Merge pull request #1428 from johnhaddon/usdSetAndBoundFixes

USDScene fixes

Changes

@@ -1,7 +1,12 @@
 10.5.x.x (relative to 10.5.9.1)
 ========
 
+Fixes
+-----
 
+- USDScene :
+  - Fixed round-tripping of colons in set names.
+  - Fixed `hash()` to consider animation on ModelAPI extents when hashing the bound.
 
 10.5.9.1 (relative to 10.5.9.0)
 ========

contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp

@@ -131,6 +131,22 @@ SceneInterface::Path fromUSDWithoutPrefix( const pxr::SdfPath &path, size_t pref
 	return result;
 }
 
+/// \todo USD 24.03 introduces support for UTF8 in prim and property names,
+/// (while retaining the requirement that they mustn't start with a digit). But
+/// `TfMakeValidIdentifier()` remains unchanged and still doesn't allow unicode
+/// characters, so we will need to do something else when we update to 24.03.
+///
+/// Note also : This is a one-way transformation, when we really want to be able to
+/// round-trip names fully. This would be possible with this proposal :
+///
+/// https://github.com/PixarAnimationStudios/OpenUSD-proposals/tree/main/proposals/transcoding_invalid_identifiers
+///
+/// A similar proposal means that leading digits and medial hyphens wouldn't
+/// need transcoding :
+///
+/// https://github.com/NVIDIA-Omniverse/USD-proposals/tree/extended_unicode_identifiers/proposals/extended_unicode_identifiers
+///
+/// Hopefully one or both of those come along to save us before too long.
 pxr::TfToken validName( const std::string &name )
 {
 	// `TfMakeValidIdentifier` _almost_ does what we want, but in Gaffer
@@ -149,6 +165,37 @@ pxr::TfToken validName( const std::string &name )
 	}
 }
 
+// As for `validName()`, but allowing ':' and ensuring that each token
+// between ':' is also a valid name. This is necessary to meet the requirements
+// of `SdfPath::IsValidNamespacedIdentifier()`.
+pxr::TfToken validNamespacedName( const std::string &name )
+{
+	std::string result;
+
+	size_t index = 0, size = name.size();
+	while( index < size )
+	{
+		const size_t prevIndex = index;
+		index = name.find( ':', index );
+		index = index == std::string::npos ? size : index;
+		if( result.size() )
+		{
+			result += ":";
+		}
+		result += validName( name.substr( prevIndex, index - prevIndex ) );
+		index++;
+	}
+
+	if( result.size() )
+	{
+		return pxr::TfToken( result );
+	}
+	else
+	{
+		return validName( name );
+	}
+}
+
 template<typename T>
 T *reportedCast( const IECore::RunTimeTyped *v, const char *context, const char *name )
 {
@@ -201,7 +248,7 @@ void writeSetInternal( const pxr::UsdPrim &prim, const pxr::TfToken &name, const
 				continue;
 			}
 			pxr::UsdPrim childPrim = prim.GetStage()->DefinePrim( USDScene::toUSD( *it ) );
-			writeSetInternal( childPrim, validName( name ), set.subTree( *it ) );
+			writeSetInternal( childPrim, name, set.subTree( *it ) );
 			it.prune(); // Only visit children of root
 		}
 		return;
@@ -215,11 +262,11 @@ void writeSetInternal( const pxr::UsdPrim &prim, const pxr::TfToken &name, const
 
 #if PXR_VERSION < 2009
 
-	pxr::UsdCollectionAPI collection = pxr::UsdCollectionAPI::ApplyCollection( prim, validName( name ), pxr::UsdTokens->explicitOnly );
+	pxr::UsdCollectionAPI collection = pxr::UsdCollectionAPI::ApplyCollection( prim, validNamespacedName( name ), pxr::UsdTokens->explicitOnly );
 
 #else
 
-	pxr::UsdCollectionAPI collection = pxr::UsdCollectionAPI::Apply( prim, validName( name ) );
+	pxr::UsdCollectionAPI collection = pxr::UsdCollectionAPI::Apply( prim, validNamespacedName( name ) );
 	collection.CreateExpansionRuleAttr( pxr::VtValue( pxr::UsdTokens->explicitOnly ) );
 
 #endif
@@ -1543,11 +1590,11 @@ void USDScene::hash( SceneInterface::HashType hashType, double time, MurmurHash
 
 void USDScene::boundHash( double time, IECore::MurmurHash &h ) const
 {
-	if( pxr::UsdGeomBoundable boundable = pxr::UsdGeomBoundable( m_location->prim ) )
+	if( auto attribute = boundAttribute( m_location->prim ) )
 	{
 		h.append( m_root->uniqueId() );
 		appendPrimOrMasterPath( m_location->prim, h );
-		if( boundable.GetExtentAttr().ValueMightBeTimeVarying() )
+		if( attribute.ValueMightBeTimeVarying() )
 		{
 			h.append( time );
 		}

contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py

@@ -4235,6 +4235,31 @@ def testModelBound( self ) :
 		self.assertTrue( root.child( "withModelAPIAndExtent" ).hasBound() )
 		self.assertEqual( root.child( "withModelAPIAndExtent" ).readBound( 0 ), imath.Box3d( imath.V3d( 1, 2, 3 ), imath.V3d( 4, 5, 6 ) ) )
 
+	def testAnimatedModelBound( self ) :
+
+		fileName = os.path.join( self.temporaryDirectory(), "modelBound.usda" )
+
+		stage = pxr.Usd.Stage.CreateNew( fileName )
+		pxr.UsdGeom.Xform.Define( stage, "/model" )
+
+		pxr.UsdGeom.ModelAPI.Apply( stage.GetPrimAtPath( "/model" ) )
+		modelAPI = pxr.UsdGeom.ModelAPI.Apply( stage.GetPrimAtPath( "/model" ) )
+		modelAPI.SetExtentsHint( [ ( 1, 2, 3 ), ( 4, 5, 6 ) ], 0 )
+		modelAPI.SetExtentsHint( [ ( 2, 3, 4 ), ( 5, 6, 7 ) ], 24 )
+
+		stage.GetRootLayer().Save()
+		del stage
+
+		root = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Read )
+		self.assertTrue( root.child( "model" ).hasBound() )
+		self.assertEqual( root.child( "model" ).readBound( 0 ), imath.Box3d( imath.V3d( 1, 2, 3 ), imath.V3d( 4, 5, 6 ) ) )
+		self.assertEqual( root.child( "model" ).readBound( 1 ), imath.Box3d( imath.V3d( 2, 3, 4 ), imath.V3d( 5, 6, 7 ) ) )
+
+		self.assertNotEqual(
+			root.child( "model" ).hash( root.HashType.BoundHash, 0 ),
+			root.child( "model" ).hash( root.HashType.BoundHash, 1 )
+		)
+
 	def testPerPurposeModelBound( self ) :
 
 		fileName = os.path.join( self.temporaryDirectory(), "testPerPurposeModelBound.usda" )
@@ -4257,5 +4282,36 @@ def testPerPurposeModelBound( self ) :
 		self.assertTrue( root.child( "group" ).hasBound() )
 		self.assertEqual( root.child( "group" ).readBound( 0 ), imath.Box3d( imath.V3d( -1 ), imath.V3d( 1 ) ) )
 
+	def testSetNameValidation( self ) :
+
+		fileName = os.path.join( self.temporaryDirectory(), "test.usda" )
+		root = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Write )
+
+		expectedSetNames = {
+			"a" : "a",
+			"foo" : "foo",
+			"foo:includes" : "foo:includes",
+			"render:test" : "render:test",
+			"render:test:foo" : "render:test:foo",
+			"1" : "_1",
+			"render:2": "render:_2",
+			"" : "_",
+		}
+
+		for setIndex, setName in enumerate( expectedSetNames.keys() ) :
+			root.writeSet( setName, IECore.PathMatcher( [ f"/set{setIndex}Member" ] ) )
+
+		del root
+		root = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Read )
+
+		self.assertEqual(
+			set( root.setNames() ),
+			set( expectedSetNames.values() ) | { "__lights", "usd:pointInstancers", "__cameras" }
+		)
+
+		for setIndex, setName in enumerate( expectedSetNames.values() ) :
+			with self.subTest( setName = setName ) :
+				self.assertEqual( root.readSet( setName ), IECore.PathMatcher( [ f"/set{setIndex}Member" ] ) )
+
 if __name__ == "__main__":
 	unittest.main()