FIX : ShaderNetworkAlgo : PR Feedback

Author: danieldresser-ie

src/IECoreScene/ShaderNetworkAlgo.cpp

@@ -813,12 +813,16 @@ std::pair< size_t, size_t > getEndPointDuplication( const T &basis )
 }
 
 std::tuple< const std::string*, const std::string*, const std::string*, const std::string*, const std::string* >
-lookupSplinePlugSuffixes( const std::string &shaderBaseName )
+lookupSplinePlugSuffixes( const std::string &shaderName )
 {
 	// We seem to be able to identify shaders that should use the PRMan convention by whether they start
-	// with one of the PRMan prefixes
-	if( boost::starts_with( shaderBaseName, "Pxr" ) ||  boost::starts_with( shaderBaseName, "Lama" ) )
+	// with one of the PRMan prefixes.
+	// NOTE : This will fail if a shader is loaded from an explicit path, rather than being found in the
+	// search path, because the shader name will include the full file path. We consider this an
+	// acceptable failure, because shaders should be found in the search paths.
+	if( boost::starts_with( shaderName, "Pxr" ) ||  boost::starts_with( shaderName, "Lama" ) )
 	{
+		// The convention used by the PRMan shader library.
 		static const std::string positions( "_Knots" );
 		static const std::string floatValues( "_Floats" );
 		static const std::string colorValues( "_Colors" );
@@ -828,6 +832,7 @@ lookupSplinePlugSuffixes( const std::string &shaderBaseName )
 	}
 	else
 	{
+		// The convention used by the OSL shaders that we ship with Gaffer.
 		static const std::string positions( "Positions" );
 		static const std::string values( "Values" );
 		static const std::string basis( "Basis" );
@@ -836,7 +841,7 @@ lookupSplinePlugSuffixes( const std::string &shaderBaseName )
 }
 
 template<typename Spline>
-void expandSpline( const InternedString &name, const Spline &spline, CompoundDataMap &newParameters, const std::string &shaderBaseName )
+void expandSpline( const InternedString &name, const Spline &spline, CompoundDataMap &newParameters, const std::string &shaderName )
 {
 	const char *basis = "catmull-rom";
 	if( spline.basis == Spline::Basis::bezier() )
@@ -890,7 +895,7 @@ void expandSpline( const InternedString &name, const Spline &spline, CompoundDat
 		}
 	}
 
-	auto [ positionsSuffix, floatValuesSuffix, colorValuesSuffix, basisSuffix, countSuffix ] = lookupSplinePlugSuffixes( shaderBaseName );
+	auto [ positionsSuffix, floatValuesSuffix, colorValuesSuffix, basisSuffix, countSuffix ] = lookupSplinePlugSuffixes( shaderName );
 
 	newParameters[ name.string() + *positionsSuffix ] = positionsData;
 	if constexpr( std::is_same_v< typename Spline::YType, float > )
@@ -980,10 +985,10 @@ void ensureParametersCopy(
 	}
 }
 
-IECore::ConstCompoundDataPtr collapseSplineParametersInternal( const IECore::ConstCompoundDataPtr &parametersData, const std::string &shaderBaseName )
+IECore::ConstCompoundDataPtr collapseSplineParametersInternal( const IECore::ConstCompoundDataPtr &parametersData, const std::string &shaderName )
 {
 
-	auto [ positionsSuffix, floatValuesSuffix, colorValuesSuffix, basisSuffix, countSuffix ] = lookupSplinePlugSuffixes( shaderBaseName );
+	auto [ positionsSuffix, floatValuesSuffix, colorValuesSuffix, basisSuffix, countSuffix ] = lookupSplinePlugSuffixes( shaderName );
 
 	const CompoundDataMap &parameters( parametersData->readable() );
 	CompoundDataPtr newParametersData;
@@ -1022,7 +1027,7 @@ IECore::ConstCompoundDataPtr collapseSplineParametersInternal( const IECore::Con
 			IECore::InternedString countName = prefix + *countSuffix;
 			const auto countIter = parameters.find( countName );
 			const IntData *countData = nullptr;
-			if( countIter == parameters.end() )
+			if( countIter != parameters.end() )
 			{
 				countData = runTimeCast<const IntData>( countIter->second.get() );
 			}
@@ -1166,11 +1171,6 @@ std::pair< InternedString, int > createSplineInputAdapter(
 	return std::make_pair( adapterHandle, getEndPointDuplication( splineData->readable().basis ).first );
 }
 
-std::string shaderBaseNameFromPath( const std::string shaderName )
-{
-	return std::filesystem::path( shaderName ).filename().string();
-}
-
 } // namespace
 
 void ShaderNetworkAlgo::collapseSplines( ShaderNetwork *network, std::string targetPrefix )
@@ -1195,12 +1195,12 @@ void ShaderNetworkAlgo::collapseSplines( ShaderNetwork *network, std::string tar
 		}
 
 		// For nodes which aren't spline adapters, we just need to deal with any parameters that are splines
-		ConstCompoundDataPtr collapsed = collapseSplineParametersInternal( shader->parametersData(), shaderBaseNameFromPath( shader->getName() ) );
+		ConstCompoundDataPtr collapsed = collapseSplineParametersInternal( shader->parametersData(), shader->getName() );
 		if( collapsed != shader->parametersData() )
 		{
 			// \todo - this const_cast is ugly, although safe because if the return from collapseSplineParameterInternals
 			// doesn't match the input, it is freshly allocated. Once collapseSplineParameters is fully
-			// deprecated, and no longer visible publicly, an internal version of collapseSplineParametersInternal could
+			// deprecated, and no longer visible publicly, collapseSplineParametersInternal could
 			// just return a non-const new parameter data, or nullptr if no changes are needed.
 			network->setShader( name, std::move( new Shader( shader->getName(), shader->getType(), const_cast< CompoundData *>( collapsed.get() ) ) ) );
 		}
@@ -1322,13 +1322,13 @@ void ShaderNetworkAlgo::expandSplines( ShaderNetwork *network, std::string targe
 			{
 				ensureParametersCopy( origParameters, newParametersData, newParameters );
 				newParameters->erase( name );
-				expandSpline( name, colorSpline->readable(), *newParameters, shaderBaseNameFromPath( s.second->getName() ) );
+				expandSpline( name, colorSpline->readable(), *newParameters, s.second->getName() );
 			}
 			else if( const SplineffData *floatSpline = runTimeCast<const SplineffData>( value.get() ) )
 			{
 				ensureParametersCopy( origParameters, newParametersData, newParameters );
 				newParameters->erase( name );
-				expandSpline( name, floatSpline->readable(), *newParameters, shaderBaseNameFromPath( s.second->getName() ) );
+				expandSpline( name, floatSpline->readable(), *newParameters, s.second->getName() );
 			}
 		}