From a3e35843b9563eaca5e3e5226c12898532a9106f Mon Sep 17 00:00:00 2001 From: John Haddon Date: Mon, 20 Jul 2026 09:57:06 +0100 Subject: [PATCH 1/3] USD AttributeAlgo : Remove redundant `static` keywords We're already in an anonymous namespace, so we don't need these. --- .../IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp b/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp index ff8ff848b7..4252e0281f 100644 --- a/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp +++ b/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp @@ -50,14 +50,14 @@ 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:"; bool writeConformantRenderManAttributes() { @@ -68,7 +68,7 @@ bool writeConformantRenderManAttributes() return false; } -} +} // namespace bool IECoreUSD::AttributeAlgo::isCortexAttribute( const pxr::UsdGeomPrimvar &primVar ) { From 5ac4a70ce79e6a218d9fd9dd632c01d8eee57e44 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Mon, 20 Jul 2026 12:00:37 +0100 Subject: [PATCH 2/3] USD AttributeAlgo : Fix handling of RenderMan shader assignments We don't need to add the `ri:attributes` prefix for these, as they're not written as primvars. --- Changes | 3 +++ .../IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp | 7 +++++ .../IECoreUSD/test/IECoreUSD/USDSceneTest.py | 26 ++++++++++++++++++- 3 files changed, 35 insertions(+), 1 deletion(-) diff --git a/Changes b/Changes index f4aab10712..b23daeae96 100644 --- a/Changes +++ b/Changes @@ -1,7 +1,10 @@ 10.6.x.x (relative to 10.6.7.0) ======== +Fixes +----- +- USDScene : Fixed bug writing shader assignments with `IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES=1`. 10.6.7.0 (relative to 10.6.6.0) ======== diff --git a/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp b/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp index 4252e0281f..763a455713 100644 --- a/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp +++ b/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp @@ -36,6 +36,7 @@ #include "boost/algorithm/string/erase.hpp" #include "boost/algorithm/string/replace.hpp" #include "boost/algorithm/string/predicate.hpp" +#include "boost/container/flat_set.hpp" IECORE_PUSH_DEFAULT_VISIBILITY #include "pxr/usd/usdGeom/primvar.h" @@ -58,6 +59,7 @@ 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 g_shaderTypes = { "surface", "displacement", "light", "volume" }; bool writeConformantRenderManAttributes() { @@ -135,6 +137,11 @@ IECoreUSD::AttributeAlgo::Name IECoreUSD::AttributeAlgo::nameToUSD( std::string { if( boost::starts_with( name, g_riPrefix ) && writeConformantRenderManAttributes() ) { + const std::string potentialShaderType = name.substr( g_riPrefix.size() ); + if( g_shaderTypes.count( potentialShaderType ) ) + { + return { pxr::TfToken( name ), true }; + } return { pxr::TfToken( g_riAttributesPrefix + name.substr( g_riPrefix.size() ) ), true }; } diff --git a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py index 8842332481..094fadd775 100644 --- a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py +++ b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py @@ -4885,10 +4885,25 @@ def testRenderManAttributeRoundTrip( self ) : 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 ) + # The attribute is written differently depending on IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES. + if conformant : primVars = pxr.UsdGeom.PrimvarsAPI( stage.GetPrimAtPath( "/test" ) ) diffuseDepth = primVars.GetPrimvar( "ri:attributes:trace:maxdiffusedepth" ) @@ -4900,12 +4915,21 @@ 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( 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() From c839fd68fda31de45e7b08727929cc5e81da8483 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Mon, 20 Jul 2026 12:15:19 +0100 Subject: [PATCH 3/3] USD AttributeAlgo : Fix IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES Don't let it dictate what attribute names we use for _reading_. Otherwise we can't read attributes unless the env-var matches the value used to write them. --- Changes | 5 +- .../IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp | 177 +++++++++++------- .../IECoreUSD/test/IECoreUSD/USDSceneTest.py | 36 ++-- 3 files changed, 135 insertions(+), 83 deletions(-) diff --git a/Changes b/Changes index b23daeae96..6e7f1feb44 100644 --- a/Changes +++ b/Changes @@ -4,7 +4,10 @@ Fixes ----- -- USDScene : Fixed bug writing shader assignments with `IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES=1`. +- 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) ======== diff --git a/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp b/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp index 763a455713..745637b2ee 100644 --- a/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp +++ b/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp @@ -37,6 +37,7 @@ #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" @@ -70,6 +71,91 @@ bool writeConformantRenderManAttributes() return false; } +using Names = boost::container::small_vector; + +// 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 ) @@ -135,61 +221,16 @@ pxr::TfToken IECoreUSD::AttributeAlgo::cortexPrimitiveVariableMetadataTokenDepre IECoreUSD::AttributeAlgo::Name IECoreUSD::AttributeAlgo::nameToUSD( std::string name ) { - if( boost::starts_with( name, g_riPrefix ) && writeConformantRenderManAttributes() ) - { - const std::string potentialShaderType = name.substr( g_riPrefix.size() ); - if( g_shaderTypes.count( potentialShaderType ) ) - { - return { pxr::TfToken( name ), true }; - } - 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:" ) ) - { - 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" ) + const Names names = usdNames( name ); + if( names.size() ) { - // Special case where the whole name is different, not just prefix - name = "arnold:displacement"; + return names.front(); } - 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 ) @@ -241,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; + } + } } } diff --git a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py index 094fadd775..9d80390d53 100644 --- a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py +++ b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py @@ -4868,20 +4868,19 @@ 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 ) @@ -4904,7 +4903,7 @@ def testRenderManAttributeRoundTrip( self ) : # The attribute is written differently depending on IECOREUSD_WRITE_CONFORMANT_RENDERMAN_ATTRIBUTES. - if conformant : + if writeConformant : primVars = pxr.UsdGeom.PrimvarsAPI( stage.GetPrimAtPath( "/test" ) ) diffuseDepth = primVars.GetPrimvar( "ri:attributes:trace:maxdiffusedepth" ) self.assertTrue( diffuseDepth.IsDefined() ) @@ -4916,6 +4915,7 @@ def testRenderManAttributeRoundTrip( self ) : 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" ) @@ -4924,12 +4924,20 @@ def testRenderManAttributeRoundTrip( self ) : # Test loading back to Cortex. - 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 ) + 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()