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;
@@ -1003,35 +1008,26 @@ IECore::ConstCompoundDataPtr collapseSplineParametersInternal( const IECore::Con
 
 
 		std::string prefix = maybeBasis.first.string().substr( 0, maybeBasis.first.string().size() - basisSuffix->size() );
-		IECore::InternedString positionsName = prefix + *positionsSuffix;
-		const auto positionsIter = parameters.find( positionsName );
-		const FloatVectorData *floatPositions = nullptr;
-
-		if( positionsIter != parameters.end() )
-		{
-			floatPositions = runTimeCast<const FloatVectorData>( positionsIter->second.get() );
-		}
-
+		const IECore::InternedString positionsName = prefix + *positionsSuffix;
+		const FloatVectorData *floatPositions = parametersData->member<const FloatVectorData>( positionsName );
 		if( !floatPositions )
 		{
 			continue;
 		}
 
+		IECore::InternedString countName;
+		const IntData *countData = nullptr;
+
 		if( countSuffix )
 		{
-			IECore::InternedString countName = prefix + *countSuffix;
-			const auto countIter = parameters.find( countName );
-			const IntData *countData = nullptr;
-			if( countIter == parameters.end() )
-			{
-				countData = runTimeCast<const IntData>( countIter->second.get() );
-			}
+			countName = prefix + *countSuffix;
+			countData = parametersData->member<const IntData>( countName );
 
 			if( !countData )
 			{
 				IECore::msg(
 					Msg::Error, "ShaderNetworkAlgo",
-					"Using spline format that expects count plug, but no int count plug found matching \"" + countName.string() + "\""
+					"Using spline format that expects count parameter, but no int count parameter found matching \"" + countName.string() + "\""
 				);
 			}
 			else
@@ -1046,39 +1042,39 @@ IECore::ConstCompoundDataPtr collapseSplineParametersInternal( const IECore::Con
 			}
 		}
 
-		IECore::InternedString floatValuesName = prefix + *floatValuesSuffix;
-		auto valuesIter = parameters.find( floatValuesName );
-		if( valuesIter == parameters.end() )
+		IECore::InternedString valuesName = prefix + *floatValuesSuffix;
+		IECore::DataPtr foundSpline;
+		if( const FloatVectorData *floatValues = parametersData->member<const FloatVectorData>( valuesName ) )
 		{
-			IECore::InternedString colorValuesName = prefix + *colorValuesSuffix;
-			valuesIter = parameters.find( colorValuesName );
+			foundSpline = loadSpline<SplineffData>( basis, floatPositions, floatValues );
 		}
-
-
-		IECore::DataPtr foundSpline;
-		if( valuesIter != parameters.end() )
+		else
 		{
-			if( const FloatVectorData *floatValues = runTimeCast<const FloatVectorData>( valuesIter->second.get() ) )
-			{
-				foundSpline = loadSpline<SplineffData>( basis, floatPositions, floatValues );
-			}
-			else if( const Color3fVectorData *color3Values = runTimeCast<const Color3fVectorData>( valuesIter->second.get() ) )
+			valuesName = prefix + *colorValuesSuffix;
+			if( const Color3fVectorData *color3Values = parametersData->member<const Color3fVectorData>( valuesName ) )
 			{
 				foundSpline = loadSpline<SplinefColor3fData>( basis, floatPositions, color3Values );
 			}
-			else if( const Color4fVectorData *color4Values = runTimeCast<const Color4fVectorData>( valuesIter->second.get() ) )
+			else if( const Color4fVectorData *color4Values = parametersData->member<const Color4fVectorData>( valuesName ) )
 			{
 				foundSpline = loadSpline<SplinefColor4fData>( basis, floatPositions, color4Values );
 			}
+
 		}
 
 		if( foundSpline )
 		{
 			ensureParametersCopy( parameters, newParametersData, newParameters );
-			(*newParameters)[prefix] = foundSpline;
 			newParameters->erase( maybeBasis.first );
 			newParameters->erase( positionsName );
-			newParameters->erase( valuesIter->first );
+			newParameters->erase( valuesName );
+			if( countData )
+			{
+				newParameters->erase( countName );
+			}
+
+			(*newParameters)[prefix] = foundSpline;
+
 		}
 	}
 
@@ -1166,11 +1162,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 +1186,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 +1313,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() );
 			}
 		}