Fix IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES bugs

Changes

@@ -1,7 +1,13 @@
 10.6.x.x (relative to 10.6.7.0)
 ========
 
+Fixes
+-----
 
+- USDScene :
+  - Fixed bug writing shader assignments with `IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES=1`.
+  - Fixed bug reading shader assignments with `IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES=1`.
+  - Fixed bug reading legacy attributes with `IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES=1`.
 
 10.6.7.0 (relative to 10.6.6.0)
 ========

contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp

@@ -36,6 +36,8 @@
 #include "boost/algorithm/string/erase.hpp"
 #include "boost/algorithm/string/replace.hpp"
 #include "boost/algorithm/string/predicate.hpp"
+#include "boost/container/flat_set.hpp"
+#include "boost/container/small_vector.hpp"
 
 IECORE_PUSH_DEFAULT_VISIBILITY
 #include "pxr/usd/usdGeom/primvar.h"
@@ -50,14 +52,15 @@ using namespace pxr;
 namespace
 {
 
-static const pxr::TfToken g_cortexPrimitiveVariableMetadataToken( "cortex_isConstantPrimitiveVariable" );
-static const pxr::TfToken g_cortexPrimitiveVariableMetadataTokenDeprecated( "IECOREUSD_CONSTANT_PRIMITIVE_VARIABLE" );
-static const std::string g_primVarPrefix = "primvars:";
-static const std::string g_primVarUserPrefix = "primvars:user:";
-static const std::string g_renderPrefix = "render:";
-static const std::string g_riPrefix = "ri:";
-static const std::string g_riAttributesPrefix = "ri:attributes:";
-static const std::string g_userPrefix = "user:";
+const pxr::TfToken g_cortexPrimitiveVariableMetadataToken( "cortex_isConstantPrimitiveVariable" );
+const pxr::TfToken g_cortexPrimitiveVariableMetadataTokenDeprecated( "IECOREUSD_CONSTANT_PRIMITIVE_VARIABLE" );
+const std::string g_primVarPrefix = "primvars:";
+const std::string g_primVarUserPrefix = "primvars:user:";
+const std::string g_renderPrefix = "render:";
+const std::string g_riPrefix = "ri:";
+const std::string g_riAttributesPrefix = "ri:attributes:";
+const std::string g_userPrefix = "user:";
+const boost::container::flat_set<std::string> g_shaderTypes = { "surface", "displacement", "light", "volume" };
 
 bool writeConformantRenderManAttributes()
 {
@@ -68,8 +71,93 @@ bool writeConformantRenderManAttributes()
 	return false;
 }
 
+using Names = boost::container::small_vector<AttributeAlgo::Name, 2>;
+
+// Given a Cortex attribute name, return the ways it might be represented in
+// USD. This is a one-to-many mapping, to account for legacy representations
+// in old files. The first name returned should be used for writing new files,
+// and the other names are used as fallbacks when reading.
+Names usdNames( std::string name )
+{
+	if( boost::starts_with( name, g_riPrefix ) )
+	{
+		const std::string potentialShaderType = name.substr( g_riPrefix.size() );
+		if( g_shaderTypes.count( potentialShaderType ) )
+		{
+			return { { pxr::TfToken( name ), true } };
+		}
+		const AttributeAlgo::Name conformantName = { pxr::TfToken( g_riAttributesPrefix + name.substr( g_riPrefix.size() ) ), true };
+		const AttributeAlgo::Name legacyName = { pxr::TfToken( name ), false };
+		if( writeConformantRenderManAttributes() )
+		{
+			return { conformantName, legacyName };
+		}
+		else
+		{
+			return { legacyName, conformantName };
+		}
+	}
+
+	bool isPrimvar = false;
+
+	// The long term plan is to convert only "render:" prefixed attributes to primvars, and it will
+	// be the client's responsibility to ensure everything important gets prefixed with "render:".
+	// But for the moment, Gaffer doesn't do this yet, so we support the two most important prefixes
+	// for Gaffer currently:  "user:" and "ai:".
+	/// \todo I don't think the `render:` plan is working out - it may well be better to just map
+	/// all Cortex attributes to primvars.
+	if( boost::starts_with( name, "render:" ) || boost::starts_with( name, "user:" ) || boost::starts_with( name, "ai:" ) )
+	{
+		isPrimvar = true;
+
+		// Strip the "render:" prefix from when writing attributes as primitive variables
+		if( boost::starts_with( name, g_renderPrefix ) )
+		{
+			name = name.substr( 7 );
+		}
+	}
+
+	if( name == "ai:disp_map" )
+	{
+		// Special case where the whole name is different, not just prefix
+		name = "arnold:displacement";
+	}
+	else
+	{
+		size_t colonPos = name.find( ":" );
+		if( colonPos != std::string::npos )
+		{
+			std::string prefix = name.substr( 0, colonPos );
+			std::string newPrefix;
+			// Translate prefixes.  Currently ai -> arnold is the only mapping supported
+			if( prefix == "ai" )
+			{
+				newPrefix = "arnold";
+			}
+
+			if( newPrefix.size() )
+			{
+				name = newPrefix + name.substr( colonPos );
+			}
+		}
+	}
+
+	Names result;
+	if( isPrimvar )
+	{
+		result.push_back( { TfToken( name ), true } );
+	}
+	if( name.find( ':' ) != std::string::npos )
+	{
+		// We add this one even when the primary version is a primvar, as a
+		// fallback to legacy files from a time when we wrote attributes.
+		result.push_back( { TfToken( name ), false } );
+	}
+	return result;
 }
 
+} // namespace
+
 bool IECoreUSD::AttributeAlgo::isCortexAttribute( const pxr::UsdGeomPrimvar &primVar )
 {
 	if( primVar.GetInterpolation() != pxr::UsdGeomTokens->constant )
@@ -133,56 +221,16 @@ pxr::TfToken IECoreUSD::AttributeAlgo::cortexPrimitiveVariableMetadataTokenDepre
 
 IECoreUSD::AttributeAlgo::Name IECoreUSD::AttributeAlgo::nameToUSD( std::string name )
 {
-	if( boost::starts_with( name, g_riPrefix ) && writeConformantRenderManAttributes() )
-	{
-		return { pxr::TfToken( g_riAttributesPrefix + name.substr( g_riPrefix.size() ) ), true };
-	}
-
-	bool isPrimvar = false;
-
-	// The long term plan is to convert only "render:" prefixed attributes to primvars, and it will
-	// be the client's responsibility to ensure everything important gets prefixed with "render:".
-	// But for the moment, Gaffer doesn't do this yet, so we support the two most important prefixes
-	// for Gaffer currently:  "user:" and "ai:".
-	/// \todo I don't think the `render:` plan is working out - it may well be better to just map
-	/// all Cortex attributes to primvars.
-	if( boost::starts_with( name, "render:" ) || boost::starts_with( name, "user:" ) || boost::starts_with( name, "ai:" ) )
+	const Names names = usdNames( name );
+	if( names.size() )
 	{
-		isPrimvar = true;
-
-		// Strip the "render:" prefix from when writing attributes as primitive variables
-		if( boost::starts_with( name, g_renderPrefix ) )
-		{
-			name = name.substr( 7 );
-		}
+		return names.front();
 	}
-
-	if( name == "ai:disp_map" )
-	{
-		// Special case where the whole name is different, not just prefix
-		name = "arnold:displacement";
-	}
-	else
-	{
-		size_t colonPos = name.find( ":" );
-		if( colonPos != std::string::npos )
-		{
-			std::string prefix = name.substr( 0, colonPos );
-			std::string newPrefix;
-			// Translate prefixes.  Currently ai -> arnold is the only mapping supported
-			if( prefix == "ai" )
-			{
-				newPrefix = "arnold";
-			}
-
-			if( newPrefix.size() )
-			{
-				name = newPrefix + name.substr( colonPos );
-			}
-		}
-	}
-
-	return { TfToken( name ), isPrimvar };
+	// This is necessary because `USDScene` calls `nameToUSD()` when writing
+	// materials, and `surface` etc aren't handled by the above.
+	/// \todo It would likely be better if the material-writing code wasn't
+	/// mixed up with this.
+	return { pxr::TfToken( name ), false };
 }
 
 IECore::InternedString IECoreUSD::AttributeAlgo::nameFromUSD( IECoreUSD::AttributeAlgo::Name name )
@@ -234,27 +282,27 @@ IECore::InternedString IECoreUSD::AttributeAlgo::nameFromUSD( IECoreUSD::Attribu
 
 UsdAttribute IECoreUSD::AttributeAlgo::findUSDAttribute( const pxr::UsdPrim &prim, std::string cortexName )
 {
-	AttributeAlgo::Name n = AttributeAlgo::nameToUSD( cortexName );
-	if( n.isPrimvar )
+	for( const auto &n : usdNames( cortexName ) )
 	{
-		if( pxr::UsdGeomPrimvar primvar = pxr::UsdGeomPrimvarsAPI( prim ).GetPrimvar( n.name ) )
+		if( n.isPrimvar )
 		{
-			if( isCortexAttribute( primvar ) )
+			if( pxr::UsdGeomPrimvar primvar = pxr::UsdGeomPrimvarsAPI( prim ).GetPrimvar( n.name ) )
 			{
-				return primvar.GetAttr();
+				if( isCortexAttribute( primvar ) )
+				{
+					return primvar.GetAttr();
+				}
 			}
 		}
-	}
-
-	// In theory, this should be able to be an else.  But for the moment, for attributes that should be written
-	// to a primvar, we try reading them from an attribute if we can't find them in a primvar.  This provides
-	// some backwards compatibility with files from before we started writing to primvars, and might provide
-	// compatibility with other USD authors, maybe?
-	if( pxr::UsdAttribute attribute = prim.GetAttribute( n.name ) )
-	{
-		if ( attribute.GetName().GetString().find( ":" ) != std::string::npos && attribute.IsCustom() )
+		else
 		{
-			return attribute;
+			if( pxr::UsdAttribute attribute = prim.GetAttribute( n.name ) )
+			{
+				if( attribute.IsCustom() )
+				{
+					return attribute;
+				}
+			}
 		}
 	}
 

contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py

@@ -4868,28 +4868,42 @@ def testOSLShaderForHDPrman( self ) :
 					self.assertEqual( loadedShaderNetwork.getShader( "scale" ).name, "floatAttribute" )
 					self.assertEqual( loadedShaderNetwork.getShader( "scale" ).type, "osl:shader" )
 
-	def testRenderManAttributeRoundTrip( self ) :
+	def testWriteConformantRenderManAttributes( self ) :
 
 		self.addCleanup( os.environ.__delitem__, "IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES" )
 
-		for conformant in True, False :
-
-			with self.subTest( conformant = conformant ) :
+		for writeConformant in ( True, False ) :
 
-				os.environ["IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES"] = str( int( conformant ) )
-
-				fileName = os.path.join( self.temporaryDirectory(), f"renderManAttributes{conformant}.usda" )
+			with self.subTest( writeConformant = writeConformant ) :
 
 				# Test writing to USD.
 
+				os.environ["IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES"] = str( int( writeConformant ) )
+
+				fileName = os.path.join( self.temporaryDirectory(), f"renderManAttributes{writeConformant}.usda" )
 				scene = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Write )
 				child = scene.createChild( "test" )
 				child.writeAttribute( "ri:trace:maxdiffusedepth", IECore.IntData( 2 ), 0 )
+
+				surface = IECoreScene.ShaderNetwork(
+					shaders = { "constant" : IECoreScene.Shader( "PxrDiffuse" ) },
+					output = ( "constant", "out_bxdf" )
+				)
+				child.writeAttribute( "ri:surface", surface, 0.0 )
+
+				surfaceFull = IECoreScene.ShaderNetwork(
+					shaders = { "constant" : IECoreScene.Shader( "PxrSurface" ) },
+					output = ( "constant", "out_bxdf" )
+				)
+				child.writeAttribute( "ri:surface:full", surfaceFull, 0.0 )
+
 				del scene, child
 
 				stage = pxr.Usd.Stage.Open( fileName )
 
-				if conformant :
+				# The attribute is written differently depending on IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES.
+
+				if writeConformant :
 					primVars = pxr.UsdGeom.PrimvarsAPI( stage.GetPrimAtPath( "/test" ) )
 					diffuseDepth = primVars.GetPrimvar( "ri:attributes:trace:maxdiffusedepth" )
 					self.assertTrue( diffuseDepth.IsDefined() )
@@ -4900,12 +4914,30 @@ def testRenderManAttributeRoundTrip( self ) :
 					diffuseDepth = stage.GetPrimAtPath( "/test" ).GetAttribute( "ri:trace:maxdiffusedepth" )
 					self.assertEqual( diffuseDepth.Get( 0 ), 2 )
 
+				# But the materials should be the same either way, with an `ri:surface` terminal.
+
+				for purpose in [ "", "full" ] :
+					material = pxr.UsdShade.MaterialBindingAPI( stage.GetPrimAtPath( "/test" ) ).ComputeBoundMaterial( purpose )[0]
+					output = material.GetOutput( "ri:surface" )
+					self.assertTrue( output )
+					self.assertEqual( output.GetConnectedSource()[1], "out_bxdf" )
+
 				# Test loading back to Cortex.
 
-				scene = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Read )
-				child = scene.child( "test" )
-				self.assertEqual( child.attributeNames(), [ "ri:trace:maxdiffusedepth" ] )
-				self.assertEqual( child.readAttribute( "ri:trace:maxdiffusedepth", 0 ), IECore.IntData( 2 ) )
+				for readConformant in ( True, False ) :
+
+					with self.subTest( readConformant = readConformant ) :
+
+						os.environ["IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES"] = str( int( readConformant ) )
+
+						fileName = os.path.join( self.temporaryDirectory(), f"renderManAttributes{writeConformant}.usda" )
+
+						scene = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Read )
+						child = scene.child( "test" )
+						self.assertEqual( set( child.attributeNames() ), { "ri:trace:maxdiffusedepth", "ri:surface", "ri:surface:full" } )
+						self.assertEqual( child.readAttribute( "ri:trace:maxdiffusedepth", 0 ), IECore.IntData( 2 ) )
+						self.assertEqual( child.readAttribute( "ri:surface", 0 ), surface )
+						self.assertEqual( child.readAttribute( "ri:surface:full", 0 ), surfaceFull )
 
 if __name__ == "__main__":
 	unittest.main()