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

@@ -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)
 ========

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

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()