Merge pull request #1394 from johnhaddon/internedStringSubstitution

ericmehl · web-flow · commit a2935e32e3af · 2023-11-09T09:15:47.000-05:00
ShaderNetwork : Support InternedString attributes in substitutions
diff --git a/Changes b/Changes
@@ -1,8 +1,18 @@
-10.5.x.x (relative to 10.5.3.0)
+10.5.x.x (relative to 10.5.4.0)
 ========
 
 
 
+10.5.4.0 (relative to 10.5.3.0)
+========
+
+Improvements
+------------
+
+- ShaderNetwork : Added support for InternedStringData attributes in string parameter substitutions.
+- MurmurHash : Added specialisation for `std::hash`, among other things allowing the use of MurmurHash as a key in `unordered_map`.
+- CompoundObject : Defaulted template argument to `Object` in `member()` methods.
+
 10.5.3.0 (relative to 10.5.2.1)
 ========
 
diff --git a/SConstruct b/SConstruct
@@ -56,7 +56,7 @@ SConsignFile()
 
 ieCoreMilestoneVersion = 10 # for announcing major milestones - may contain all of the below
 ieCoreMajorVersion = 5 # backwards-incompatible changes
-ieCoreMinorVersion = 3 # new backwards-compatible features
+ieCoreMinorVersion = 4 # new backwards-compatible features
 ieCorePatchVersion = 0 # bug fixes
 ieCoreVersionSuffix = "" # used for alpha/beta releases. Example: "a1", "b2", etc.
 
@@ -1226,7 +1226,7 @@ else:
 				"/Fd${TARGET}.pdb",
 			],
 		)
-	
+
 	# Reorder build commands so that `/external:I` includes come after `/I` includes.
 	# Otherwise we'll pick up the Gaffer includes from the build directory, and not
 	# the ones in the source tree.
diff --git a/include/IECore/CompoundObject.h b/include/IECore/CompoundObject.h
@@ -64,17 +64,17 @@ class IECORE_API CompoundObject : public Object
 		/// or doesn't match the type specified as the template argument, behavior
 		/// is defined by the throwExceptions parameter. When this parameter is true a descriptive
 		/// Exception is thrown, and when false 0 is returned.
-		template<typename T>
+		template<typename T = Object>
 		T *member( const InternedString &name, bool throwExceptions = false );
-		template<typename T>
+		template<typename T = Object>
 		const T *member( const InternedString &name, bool throwExceptions = false ) const;
 
 		/// A Convenience function to find an object in members().
 		/// If the named object doesn't exist, if createIfMissing is true, an object will be added
 		/// with the type's object factory create method. If false, or the named entry does not match the
 		/// type specified as the template argument, behavior is defined by the throwExceptions parameter.
 		/// When this parameter is true a descriptive Exception is thrown, and when false 0 is returned.
-		template<typename T>
+		template<typename T = Object>
 		T *member( const InternedString &name, bool throwExceptions, bool createIfMissing );
 
 		/// Returns an instance of CompoundObject which can be shared by everyone - for instance a procedural
diff --git a/src/IECoreMaya/ParameterisedHolderModificationCmd.cpp b/src/IECoreMaya/ParameterisedHolderModificationCmd.cpp
@@ -323,7 +323,7 @@ void ParameterisedHolderModificationCmd::storeParametersWithNewValues( const IEC
 			{
 				childParameterPath = it->first;
 			}
-			storeParametersWithNewValues( it->second.get(), newCompound->member<Object>( it->first ), childParameterPath );
+			storeParametersWithNewValues( it->second.get(), newCompound->member( it->first ), childParameterPath );
 		}
 
 		const CompoundObject::ObjectMap &newChildren = static_cast<const CompoundObject *>( newValue )->members();
diff --git a/src/IECoreNuke/ParameterisedHolder.cpp b/src/IECoreNuke/ParameterisedHolder.cpp
@@ -188,7 +188,7 @@ int ParameterisedHolder<BaseType>::knob_changed( DD::Image::Knob *knob )
 			ParameterisedInterface *parameterisedInterface = dynamic_cast<ParameterisedInterface *>( g_getParameterisedResult.get() );
 			// apply current state
 			ConstCompoundObjectPtr classSpecifier = runTimeCast<const CompoundObject>( m_classSpecifierKnob->getValue() );
-			ConstObjectPtr handlerState = classSpecifier->member<Object>( "handlerState" );
+			ConstObjectPtr handlerState = classSpecifier->member( "handlerState" );
 			if( handlerState )
 			{
 				m_parameterHandler->setState( parameterisedInterface->parameters(), handlerState.get() );
@@ -525,7 +525,7 @@ void ParameterisedHolder<BaseType>::updateParameterised( bool reload )
 		// apply the previously stored handler state
 
 		ConstCompoundObjectPtr classSpecifier = runTimeCast<const CompoundObject>( m_classSpecifierKnob->getValue() );
-		ConstObjectPtr handlerState = classSpecifier->member<Object>( "handlerState" );
+		ConstObjectPtr handlerState = classSpecifier->member( "handlerState" );
 		if( handlerState )
 		{
 			m_parameterHandler->setState( parameterisedInterface->parameters(), handlerState.get() );
diff --git a/src/IECoreScene/ShaderNetwork.cpp b/src/IECoreScene/ShaderNetwork.cpp
@@ -66,10 +66,14 @@ struct ReplaceFunctor
 	std::string operator()( const boost::smatch & match )
 	{
 		// Search for attribute matching token
-		const StringData *sourceAttribute = m_attributes->member<StringData>( match[1].str() );
-		if( sourceAttribute )
+		const Object *sourceAttribute = m_attributes->member( match[1].str() );
+		if( auto stringData = runTimeCast<const StringData>( sourceAttribute ) )
 		{
-			return sourceAttribute->readable();
+			return stringData->readable();
+		}
+		else if( auto internedStringData = runTimeCast<const InternedStringData>( sourceAttribute ) )
+		{
+			return internedStringData->readable().string();
 		}
 		else
 		{
@@ -86,7 +90,7 @@ boost::regex attributeRegex()
 	// Extract ATTR_NAME from the pattern <attr:ATTR_NAME>
 	// Only match if the angle brackets haven't been escaped with a backslash
 	static boost::regex r( "(?<!\\\\)<attr:([^>]*[^\\\\>])>" );
-	return r;	
+	return r;
 }
 
 bool stringFindSubstitutions( const std::string &target, std::unordered_set< InternedString > &requestedAttributes )
@@ -428,10 +432,14 @@ class ShaderNetwork::Implementation
 			update();
 			for( const auto &a : m_neededSubstitutions )
 			{
-				const StringData *sourceAttribute = attributes->member<StringData>( a );
-				if( sourceAttribute )
+				const Object *sourceAttribute = attributes->member( a );
+				if( auto stringData = runTimeCast<const StringData>( sourceAttribute ) )
+				{
+					h.append( stringData->readable() );
+				}
+				else if( auto internedStringData = runTimeCast<const InternedStringData>( sourceAttribute ) )
 				{
-					h.append( sourceAttribute->readable() );
+					h.append( internedStringData->readable().string() );
 				}
 				else
 				{
@@ -737,7 +745,7 @@ class ShaderNetwork::Implementation
 						}
 					}
 
-					m_parmsNeedingSubstitution[ node.handle ] = parmsNeedingSub;	
+					m_parmsNeedingSubstitution[ node.handle ] = parmsNeedingSub;
 				}
 
 				m_hash.append( m_output.shader );
diff --git a/test/IECoreScene/ShaderNetworkTest.py b/test/IECoreScene/ShaderNetworkTest.py
@@ -513,23 +513,25 @@ def testUniqueHandles( self ) :
 			{ "test" } | { "test{0}".format( x ) for x in range( 1, 20 ) }
 		)
 
-	def testSubstitutions( self ):
-		def runSubstitutionTest( shader, attributes ):
-			n = IECoreScene.ShaderNetwork( shaders = { "s" : s } )
-			a = IECore.CompoundObject( attributes )
-			h = IECore.MurmurHash()
-			n.hashSubstitutions( a, h )
-			nSubst = n.copy()
-			nSubst.applySubstitutions( a )
-			return ( h, nSubst.getShader("s") )
+	def __hashAndSubstitution( self, shader, attributes ) :
+
+		n = IECoreScene.ShaderNetwork( shaders = { "s" : shader } )
+		a = IECore.CompoundObject( attributes )
+		h = IECore.MurmurHash()
+		n.hashSubstitutions( a, h )
+		nSubst = n.copy()
+		nSubst.applySubstitutions( a )
+		return ( h, nSubst.getShader( "s" ) )
+
+	def testSubstitutions( self ) :
 
 		s = IECoreScene.Shader( "test", "surface",IECore.CompoundData( {
 			"a" : IECore.StringData( "foo" ),
 			"b" : IECore.FloatData( 42.42 ),
 			"c" : IECore.StringVectorData( [ "foo", "bar" ] ),
 		} ) )
 
-		( h, sSubst ) = runSubstitutionTest( s, { "unused" : IECore.StringData( "blah" ) } )
+		( h, sSubst ) = self.__hashAndSubstitution( s, { "unused" : IECore.StringData( "blah" ) } )
 		self.assertEqual( h, IECore.MurmurHash() )
 		self.assertEqual( s, sSubst )
 
@@ -538,7 +540,7 @@ def runSubstitutionTest( shader, attributes ):
 			"b" : IECore.FloatData( 42.42 ),
 			"c" : IECore.StringVectorData( [ "<attr:bob>", "pre<attr:carol>", "<attr:fred>post", "<attr:bob><attr:carol> <attr:fred>" ] ),
 		} ) )
-		( h, sSubst ) = runSubstitutionTest( s, { "unused" : IECore.StringData( "blah" ) } )
+		( h, sSubst ) = self.__hashAndSubstitution( s, { "unused" : IECore.StringData( "blah" ) } )
 		# Now that we've got substitutions, the hash should be non-default
 		self.assertNotEqual( h, IECore.MurmurHash() )
 
@@ -550,12 +552,12 @@ def runSubstitutionTest( shader, attributes ):
 		self.assertEqual( sSubst.parameters["c"][2], "post" )
 		self.assertEqual( sSubst.parameters["c"][3], " " )
 
-		( h2, sSubst2 ) = runSubstitutionTest( s, { "unused" : IECore.StringData( "blah2" ) } )
+		( h2, sSubst2 ) = self.__hashAndSubstitution( s, { "unused" : IECore.StringData( "blah2" ) } )
 		# The attribute being changed has no impact
 		self.assertEqual( h, h2 )
 		self.assertEqual( sSubst, sSubst2 )
 
-		( h3, sSubst3 ) = runSubstitutionTest( s, { "fred" : IECore.StringData( "CAT" ) } )
+		( h3, sSubst3 ) = self.__hashAndSubstitution( s, { "fred" : IECore.StringData( "CAT" ) } )
 		self.assertNotEqual( h, h3 )
 		self.assertNotEqual( s, sSubst3 )
 		self.assertEqual( sSubst3.parameters["a"].value, "preCATpost" )
@@ -564,7 +566,7 @@ def runSubstitutionTest( shader, attributes ):
 		self.assertEqual( sSubst3.parameters["c"][2], "CATpost" )
 		self.assertEqual( sSubst3.parameters["c"][3], " CAT" )
 
-		( h4, sSubst4 ) = runSubstitutionTest( s, { "fred" : IECore.StringData( "FISH" ) } )
+		( h4, sSubst4 ) = self.__hashAndSubstitution( s, { "fred" : IECore.StringData( "FISH" ) } )
 		self.assertNotEqual( h3, h4 )
 		self.assertEqual( sSubst4.parameters["c"][2], "FISHpost" )
 
@@ -573,7 +575,7 @@ def runSubstitutionTest( shader, attributes ):
 			"bob" : IECore.StringData( "CAT" ),
 			"carol" : IECore.StringData( "BIRD" )
 		}
-		( h5, sSubst5 ) = runSubstitutionTest( s, allAttributes )
+		( h5, sSubst5 ) = self.__hashAndSubstitution( s, allAttributes )
 		self.assertNotEqual( h4, h5 )
 		self.assertEqual( sSubst5.parameters["a"].value, "preFISHpost" )
 		self.assertEqual( sSubst5.parameters["c"][0], "CAT" )
@@ -587,14 +589,32 @@ def runSubstitutionTest( shader, attributes ):
 			"b" : IECore.FloatData( 42.42 ),
 			"c" : IECore.StringVectorData( [ r"\<attr:bob\>", r"\<attr:carol>", r"<attr:fred\>" ] ),
 		} ) )
-		( h6, sSubst6 ) = runSubstitutionTest( s, {} )
-		( h7, sSubst7 ) = runSubstitutionTest( s, allAttributes )
+		( h6, sSubst6 ) = self.__hashAndSubstitution( s, {} )
+		( h7, sSubst7 ) = self.__hashAndSubstitution( s, allAttributes )
 		self.assertEqual( h6, h7 )
 		self.assertEqual( sSubst6, sSubst7 )
 		self.assertEqual( sSubst6.parameters["a"].value, "pre<attr:fred>post" )
 		self.assertEqual( sSubst6.parameters["c"][0], "<attr:bob>" )
 		self.assertEqual( sSubst6.parameters["c"][1], "<attr:carol>" )
 		self.assertEqual( sSubst6.parameters["c"][2], "<attr:fred>" )
 
+	def testInternedStringSubstitutions( self ) :
+
+		shader = IECoreScene.Shader(
+			"test", "surface",
+			IECore.CompoundData( {
+				"a" : IECore.StringData( "<attr:internedString>" ),
+			} )
+		)
+
+		hash1, substitutedShader = self.__hashAndSubstitution( shader, { "internedString" : IECore.InternedStringData( "a" ) } )
+		self.assertNotEqual( hash1, IECore.MurmurHash() )
+		self.assertEqual( substitutedShader.parameters["a"], IECore.StringData( "a" ) )
+
+		hash2, substitutedShader = self.__hashAndSubstitution( shader, { "internedString" : IECore.InternedStringData( "b" ) } )
+		self.assertNotEqual( hash2, IECore.MurmurHash() )
+		self.assertNotEqual( hash2, hash1 )
+		self.assertEqual( substitutedShader.parameters["a"], IECore.StringData( "b" ) )
+
 if __name__ == "__main__":
 	unittest.main()

Original file line number	Diff line number	Diff line change
`@@ -323,7 +323,7 @@ void ParameterisedHolderModificationCmd::storeParametersWithNewValues( const IEC`
`323`	`323`	`{`
`324`	`324`	`childParameterPath = it->first;`
`325`	`325`	`}`
`326`		`- storeParametersWithNewValues( it->second.get(), newCompound->member<Object>( it->first ), childParameterPath );`
	`326`	`+ storeParametersWithNewValues( it->second.get(), newCompound->member( it->first ), childParameterPath );`
`327`	`327`	`}`
`328`	`328`
`329`	`329`	`const CompoundObject::ObjectMap &newChildren = static_cast<const CompoundObject *>( newValue )->members();`
Original file line number	Diff line number	Diff line change
`@@ -66,10 +66,14 @@ struct ReplaceFunctor`
`66`	`66`	`std::string operator()( const boost::smatch & match )`
`67`	`67`	`{`
`68`	`68`	`// Search for attribute matching token`
`69`		`- const StringData *sourceAttribute = m_attributes->member<StringData>( match[1].str() );`
`70`		`- if( sourceAttribute )`
	`69`	`+ const Object *sourceAttribute = m_attributes->member( match[1].str() );`
	`70`	`+ if( auto stringData = runTimeCast<const StringData>( sourceAttribute ) )`
`71`	`71`	`{`
`72`		`- return sourceAttribute->readable();`
	`72`	`+ return stringData->readable();`
	`73`	`+ }`
	`74`	`+ else if( auto internedStringData = runTimeCast<const InternedStringData>( sourceAttribute ) )`
	`75`	`+ {`
	`76`	`+ return internedStringData->readable().string();`
`73`	`77`	`}`
`74`	`78`	`else`
`75`	`79`	`{`
`@@ -86,7 +90,7 @@ boost::regex attributeRegex()`
`86`	`90`	`// Extract ATTR_NAME from the pattern <attr:ATTR_NAME>`
`87`	`91`	`// Only match if the angle brackets haven't been escaped with a backslash`
`88`	`92`	`static boost::regex r( "(?<!\\\\)<attr:([^>]*[^\\\\>])>" );`
`89`		`- return r;`
	`93`	`+ return r;`
`90`	`94`	`}`
`91`	`95`
`92`	`96`	`bool stringFindSubstitutions( const std::string &target, std::unordered_set< InternedString > &requestedAttributes )`
`@@ -428,10 +432,14 @@ class ShaderNetwork::Implementation`
`428`	`432`	`update();`
`429`	`433`	`for( const auto &a : m_neededSubstitutions )`
`430`	`434`	`{`
`431`		`- const StringData *sourceAttribute = attributes->member<StringData>( a );`
`432`		`- if( sourceAttribute )`
	`435`	`+ const Object *sourceAttribute = attributes->member( a );`
	`436`	`+ if( auto stringData = runTimeCast<const StringData>( sourceAttribute ) )`
	`437`	`+ {`
	`438`	`+ h.append( stringData->readable() );`
	`439`	`+ }`
	`440`	`+ else if( auto internedStringData = runTimeCast<const InternedStringData>( sourceAttribute ) )`
`433`	`441`	`{`
`434`		`- h.append( sourceAttribute->readable() );`
	`442`	`+ h.append( internedStringData->readable().string() );`
`435`	`443`	`}`
`436`	`444`	`else`
`437`	`445`	`{`
`@@ -737,7 +745,7 @@ class ShaderNetwork::Implementation`
`737`	`745`	`}`
`738`	`746`	`}`
`739`	`747`
`740`		`- m_parmsNeedingSubstitution[ node.handle ] = parmsNeedingSub;`
	`748`	`+ m_parmsNeedingSubstitution[ node.handle ] = parmsNeedingSub;`
`741`	`749`	`}`
`742`	`750`
`743`	`751`	`m_hash.append( m_output.shader );`