From 1cca93125d0f5b0ee5c375c8c1c0c340c25e488f Mon Sep 17 00:00:00 2001 From: John Haddon Date: Tue, 27 Aug 2024 13:05:20 +0100 Subject: [PATCH] USDScene : Fix round-tripping of colons in set names And make some notes on future changes we'll want to make to our name mangling. In theory we could implement round-tripping _now_ either by using metadata to store the "true name" or by implementing the first USD proposal ourselves. But I don't think round-tripping of arbitrary names is pressing enough to warrant the extra effort and ugliness if USD will provide it for us in the medium term. Note : I removed the first call to `validName()` in `writeSetInternal()` because in that case we're going to make the name valid in the recursive call we're passing the name to. --- Changes | 3 ++ contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp | 53 +++++++++++++++++-- .../IECoreUSD/test/IECoreUSD/USDSceneTest.py | 31 +++++++++++ 3 files changed, 84 insertions(+), 3 deletions(-) diff --git a/Changes b/Changes index 700537783d..483e516a7a 100644 --- a/Changes +++ b/Changes @@ -1,7 +1,10 @@ 10.5.x.x (relative to 10.5.9.1) ======== +Fixes +----- +- USDScene : Fixed round-tripping of colons in set names. 10.5.9.1 (relative to 10.5.9.0) ======== diff --git a/contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp b/contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp index ac67cb8b90..bdbb57c4c3 100644 --- a/contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp +++ b/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 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 diff --git a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py index 553af70126..a2c2fbe4bf 100644 --- a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py +++ b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py @@ -4257,5 +4257,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()