Merge branch 'RB-10.4' into main

Author: johnhaddon

Changes

@@ -29,9 +29,18 @@ Breaking Changes
 - Primitive : Changed `variableIndexedView()` return type from `boost::optional` to `std::optional`.
 - MeshAlgo `distributePoints()` : Bug fixes mean subtle changes to the resulting points.
 
-10.4.x.x (relative to 10.4.9.0)
+10.4.x.x (relative to 10.4.9.1)
 ========
 
+
+10.4.9.1 (relative to 10.4.9.0)
+========
+
+Fixes
+-----
+
+- USDScene : Fixed hash clashes when the same USD file containing instanceable prims was opened a second time. Because USD doesn't assign prototypes deterministically, hashes would be shuffled between prototypes, yielding inconsistencies between the hashes for the two scenes. Hashes are now completely unique from scene-to-scene, so no locations can have clashing hashes.
+
 10.4.9.0 (relative to 10.4.8.0)
 ========
 

contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp

@@ -516,6 +516,14 @@ Imath::M44d localTransform( const pxr::UsdPrim &prim, pxr::UsdTimeCode time )
 	return result;
 }
 
+// Used to assign a unique hash to each USD file. Using a global counter rather than the file name 
+// means that we treat the same file as separate if it is closed and reopened. This means it's not
+// a problem if USD changes things when a file is reopened. USD appears to not in general guarantee
+// that anything is the same when reopening an unchanged file - things we're aware of that could
+// cause problems without this conservative uniquifying are: how instance prototype names are
+// assigned, and hashes ( indices ) of SdfPaths
+std::atomic< int > g_usdFileCounter = 0;
+
 } // namespace
 
 class USDScene::Location : public RefCounted
@@ -539,7 +547,8 @@ class USDScene::IO : public RefCounted
 			:	m_fileName( fileName ), m_openMode( openMode ), m_stage( stage ),
 				m_rootPrim( m_stage->GetPseudoRoot() ),
 				m_timeCodesPerSecond( m_stage->GetTimeCodesPerSecond() ),
-				m_shaderNetworkCache( 10 * 1024 * 1024 ) // 10Mb
+				m_shaderNetworkCache( 10 * 1024 * 1024 ), // 10Mb
+				m_uniqueId( g_usdFileCounter.fetch_add( 1, std::memory_order_relaxed ) )
 		{
 			// Although the USD API implies otherwise, we need a different
 			// cache per-purpose because `UsdShadeMaterialBindingAPI::ComputeBoundMaterial()`
@@ -656,6 +665,11 @@ class USDScene::IO : public RefCounted
 			return m_shaderNetworkCache.get( output );
 		}
 
+		inline int uniqueId()
+		{
+			return m_uniqueId;
+		}
+
 	private :
 
 		static pxr::UsdStageRefPtr makeStage( const std::string &fileName, IndexedIO::OpenMode openMode )
@@ -691,6 +705,10 @@ class USDScene::IO : public RefCounted
 
 		ShaderNetworkCache m_shaderNetworkCache;
 
+		// Used to identify a file uniquely ( including between different openings of the same filename,
+		// since closing and reopening a file may cause USD to shuffle the contents ).
+		const int m_uniqueId;
+
 };
 
 USDScene::USDScene( const std::string &fileName, IndexedIO::OpenMode openMode )
@@ -1307,7 +1325,7 @@ void USDScene::hashSet( const Name &name, IECore::MurmurHash &h ) const
 {
 	SceneInterface::hashSet( name, h );
 
-	h.append( m_root->fileName() );
+	h.append( m_root->uniqueId() );
 	append( m_location->prim.GetPath(), h );
 	h.append( name );
 }
@@ -1463,7 +1481,7 @@ void USDScene::boundHash( double time, IECore::MurmurHash &h ) const
 {
 	if( pxr::UsdGeomBoundable boundable = pxr::UsdGeomBoundable( m_location->prim ) )
 	{
-		h.append( m_root->fileName() );
+		h.append( m_root->uniqueId() );
 		appendPrimOrMasterPath( m_location->prim, h );
 		if( boundable.GetExtentAttr().ValueMightBeTimeVarying() )
 		{
@@ -1476,7 +1494,7 @@ void USDScene::transformHash( double time, IECore::MurmurHash &h ) const
 {
 	if( pxr::UsdGeomXformable xformable = pxr::UsdGeomXformable( m_location->prim ) )
 	{
-		h.append( m_root->fileName() );
+		h.append( m_root->uniqueId() );
 		appendPrimOrMasterPath( m_location->prim, h );
 
 		bool mightBeTimeVarying = xformable.TransformMightBeTimeVarying();
@@ -1573,7 +1591,7 @@ void USDScene::attributesHash( double time, IECore::MurmurHash &h ) const
 
 	if( haveAttributes || haveMaterials )
 	{
-		h.append( m_root->fileName() );
+		h.append( m_root->uniqueId() );
 
 		if( haveAttributes )
 		{
@@ -1594,7 +1612,7 @@ void USDScene::objectHash( double time, IECore::MurmurHash &h ) const
 {
 	if( ObjectAlgo::canReadObject( m_location->prim ) )
 	{
-		h.append( m_root->fileName() );
+		h.append( m_root->uniqueId() );
 		appendPrimOrMasterPath( m_location->prim, h );
 		if( ObjectAlgo::objectMightBeTimeVarying( m_location->prim ) )
 		{
@@ -1604,13 +1622,13 @@ void USDScene::objectHash( double time, IECore::MurmurHash &h ) const
 }
 void USDScene::childNamesHash( double time, IECore::MurmurHash &h ) const
 {
-	h.append( m_root->fileName() );
+	h.append( m_root->uniqueId() );
 	appendPrimOrMasterPath( m_location->prim, h );
 }
 
 void USDScene::hierarchyHash( double time, IECore::MurmurHash &h ) const
 {
-	h.append( m_root->fileName() );
+	h.append( m_root->uniqueId() );
 	appendPrimOrMasterPath( m_location->prim, h );
 	h.append( time );
 }

contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py

@@ -474,6 +474,60 @@ def testInstancesShareHashes( self ) :
 				self.assertEqual( instance0.hash( hashType, 0 ), instance1.hash( hashType, 0 ) )
 				self.assertEqual( notInstance.hash( hashType, 0 ), instance0.hash( hashType, 0 ) )
 
+	def testInstancePrototypeHashesNotReused( self ) :
+
+		# The original intent of this test was to check that we assign consistent hashes to prototypes when
+		# opening and closing the same file ( which triggers USD to randomly shuffle the prototype names ).
+
+		# The solution we've ended up with is instead of assigning consistent hashes, each instance of the
+		# file gets it's own unique hashes, and is basically treated separately. This allows us to just
+		# use the prototype names in the hash, since we force the hash to be unique anyway.
+		
+		usedHashes = set()
+
+		for i in range( 100 ):
+			scene = IECoreScene.SceneInterface.create(
+				os.path.dirname( __file__ ) + "/data/severalInstances.usda",
+				IECore.IndexedIO.OpenMode.Read
+			)
+
+			instance0 = scene.child( "instance0" )
+			instance0Child = instance0.child( "world" )
+			instance10 = scene.child( "instance10" )
+			instance10Child = instance10.child( "model" ).child( "assembly" )
+
+			h1 = instance0Child.hash( scene.HashType.TransformHash, 1 )
+			h2 = instance10Child.hash( scene.HashType.TransformHash, 1 )
+
+			self.assertNotEqual( h1, h2 )
+
+			self.assertNotIn( h1, usedHashes )
+			for j in range( 1, 10 ):
+				instanceJ = scene.child( "instance%i" % j )
+				self.assertEqual( h1, instanceJ.child( "world" ).hash( scene.HashType.TransformHash, 1 ) )
+				del instanceJ
+				
+			self.assertNotIn( h2, usedHashes )
+			for j in range( 11, 20 ):
+				instanceJ = scene.child( "instance%i" % j )
+				self.assertEqual( h2, instanceJ.child( "model" ).child( "assembly" ).hash( scene.HashType.TransformHash, 1 ) )
+				del instanceJ
+
+			usedHashes.add( h1 )
+			usedHashes.add( h2 )
+
+			# We must carefully delete everything in order to reliably trigger USD randomly switching the
+			# prototype names around ( I guess waiting for the garbage collector means we might not be
+			# fully closing the file before we open it again? Weird, but seems reproducible ).
+			# This is no longer really crucial to this test, since we just force every instance of the file
+			# to get unique hashes rather than trying to keep prototypes hashing the same, but I'm trying to
+			# document as much intent as possible here, in case we consider a different solution in the future.
+			del instance0Child
+			del instance0
+			del instance10Child
+			del instance10
+			del scene
+
 	def testGeometricInterpretation( self ) :
 
 		primitive = IECoreScene.PointsPrimitive( IECore.V3fVectorData( [ imath.V3f( 0 ) ] ) )
@@ -1049,8 +1103,6 @@ def testSetHashes( self ):
 		readRoot = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Read )
 		readRoot2 = IECoreScene.SceneInterface.create( anotherFileName, IECore.IndexedIO.OpenMode.Read )
 
-		readRoot3 = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Read )
-
 		A = readRoot.child('A')
 		Ap = readRoot.child('A')
 
@@ -1064,9 +1116,6 @@ def testSetHashes( self ):
 		A2 = readRoot2.child('A')
 		self.assertNotEqual( A.hashSet("dummySetA"), A2.hashSet("dummySetA") )
 
-		A3 = readRoot3.child('A')
-		self.assertEqual( A.hashSet("dummySetA"), A3.hashSet("dummySetA") )
-
 	def testSetsAtRoot( self ) :
 
 		fileName = os.path.join( self.temporaryDirectory(), "test.usda" )

contrib/IECoreUSD/test/IECoreUSD/data/severalInstances.usda

@@ -0,0 +1,102 @@
+#usda 1.0
+
+def Xform "instance0" (
+    prepend references = @./sphere.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance1" (
+    prepend references = @./sphere.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance2" (
+    prepend references = @./sphere.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance3" (
+    prepend references = @./sphere.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance4" (
+    prepend references = @./sphere.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance5" (
+    prepend references = @./sphere.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance6" (
+    prepend references = @./sphere.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance7" (
+    prepend references = @./sphere.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance8" (
+    prepend references = @./sphere.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance9" (
+    prepend references = @./sphere.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance10" (
+    prepend references = @./kind.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance11" (
+    prepend references = @./kind.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance12" (
+    prepend references = @./kind.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance13" (
+    prepend references = @./kind.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance14" (
+    prepend references = @./kind.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance15" (
+    prepend references = @./kind.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance16" (
+    prepend references = @./kind.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance17" (
+    prepend references = @./kind.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance18" (
+    prepend references = @./kind.usda@
+    instanceable = true
+)
+{ }
+def Xform "instance19" (
+    prepend references = @./kind.usda@
+    instanceable = true
+)
+{ }