diff --git a/Changes b/Changes index f4aab10712..6e7f1feb44 100644 --- a/Changes +++ b/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) ======== diff --git a/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp b/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp index ff8ff848b7..745637b2ee 100644 --- a/contrib/IECoreUSD/src/IECoreUSD/AttributeAlgo.cpp +++ b/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 g_shaderTypes = { "surface", "displacement", "light", "volume" }; bool writeConformantRenderManAttributes() { @@ -68,8 +71,93 @@ 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 ) { 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; + } + } } } diff --git a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py index 8842332481..9d80390d53 100644 --- a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py +++ b/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()