From 4093b7b6c3b16aa018ad654c5a2a52e9d25bc3b0 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Mon, 14 Aug 2023 13:05:03 +0100 Subject: [PATCH 1/2] IECoreUSD : Remove trailing whitespace --- contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp | 2 +- contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp b/contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp index b248c6df29..5832607d37 100644 --- a/contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp +++ b/contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp @@ -516,7 +516,7 @@ Imath::M44d localTransform( const pxr::UsdPrim &prim, pxr::UsdTimeCode time ) return result; } -// Used to assign a unique hash to each USD file. Using a global counter rather than the file name +// Used to assign a unique hash to each USD file. Using a global counter rather than the file name // means that we treat the same file as separate if it is closed and reopened. This means it's not // a problem if USD changes things when a file is reopened. USD appears to not in general guarantee // that anything is the same when reopening an unchanged file - things we're aware of that could diff --git a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py index 825f213b1a..96357464f1 100644 --- a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py +++ b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py @@ -482,7 +482,7 @@ def testInstancePrototypeHashesNotReused( self ) : # The solution we've ended up with is instead of assigning consistent hashes, each instance of the # file gets it's own unique hashes, and is basically treated separately. This allows us to just # use the prototype names in the hash, since we force the hash to be unique anyway. - + usedHashes = set() for i in range( 100 ): @@ -506,7 +506,7 @@ def testInstancePrototypeHashesNotReused( self ) : instanceJ = scene.child( "instance%i" % j ) self.assertEqual( h1, instanceJ.child( "world" ).hash( scene.HashType.TransformHash, 1 ) ) del instanceJ - + self.assertNotIn( h2, usedHashes ) for j in range( 11, 20 ): instanceJ = scene.child( "instance%i" % j ) From ac1651b80daabb2741b3acdaa19d6692aab8a244 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Mon, 14 Aug 2023 13:09:15 +0100 Subject: [PATCH 2/2] USDScene : Don't treat unconnected material outputs as attributes We were already trying to do this by testing `IsAuthored()`, but this wasn't catching renderer-specific outputs which had been created but not connected. Loading such outputs as empty ShaderNetworks was then causing downstream errors when trying to render them in Gaffer. --- Changes | 5 +++++ .../IECoreUSD/include/IECoreUSD/ShaderAlgo.h | 4 ++++ .../IECoreUSD/src/IECoreUSD/ShaderAlgo.cpp | 18 +++++++++++++++- contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp | 11 ++++++---- .../IECoreUSD/test/IECoreUSD/USDSceneTest.py | 21 +++++++++++++++---- .../data/unconnectedMaterialOutput.usda | 13 ++++++++++++ 6 files changed, 63 insertions(+), 9 deletions(-) create mode 100644 contrib/IECoreUSD/test/IECoreUSD/data/unconnectedMaterialOutput.usda diff --git a/Changes b/Changes index b621e77fdc..ce4539c8b4 100644 --- a/Changes +++ b/Changes @@ -1,6 +1,11 @@ 10.4.x.x (relative to 10.4.10.2) ======== +Fixes +----- + +- USDScene : Fixed treatment of unconnected material outputs during reading. If they were "authored" but not connected to a source, they were incorrectly being treated as valid attributes, and loading as empty ShaderNetworks which caused problems elsewhere. + 10.4.10.2 (relative to 10.4.10.1) ======== diff --git a/contrib/IECoreUSD/include/IECoreUSD/ShaderAlgo.h b/contrib/IECoreUSD/include/IECoreUSD/ShaderAlgo.h index 036a90d52f..0a00a6c475 100644 --- a/contrib/IECoreUSD/include/IECoreUSD/ShaderAlgo.h +++ b/contrib/IECoreUSD/include/IECoreUSD/ShaderAlgo.h @@ -57,7 +57,11 @@ namespace ShaderAlgo IECOREUSD_API pxr::UsdShadeOutput writeShaderNetwork( const IECoreScene::ShaderNetwork *shaderNetwork, pxr::UsdPrim shaderContainer ); /// Reads a ShaderNetwork from a material output, typically obtained from `UsdShadeMaterial::GetOutput()`. +/// Returns `nullptr` if `canReadShaderNetwork() == false`, usually because the output has no connected source. IECoreScene::ShaderNetworkPtr readShaderNetwork( const pxr::UsdShadeOutput &output ); +/// Returns true if `readShaderNetwork()` will return `nullptr`, usually because the output has no +/// connected source. +bool canReadShaderNetwork( const pxr::UsdShadeOutput &output ); #if PXR_VERSION >= 2111 /// Reads a ShaderNetwork from a light. diff --git a/contrib/IECoreUSD/src/IECoreUSD/ShaderAlgo.cpp b/contrib/IECoreUSD/src/IECoreUSD/ShaderAlgo.cpp index 3ffc012914..c0bd5f1efd 100644 --- a/contrib/IECoreUSD/src/IECoreUSD/ShaderAlgo.cpp +++ b/contrib/IECoreUSD/src/IECoreUSD/ShaderAlgo.cpp @@ -312,6 +312,22 @@ pxr::UsdShadeOutput IECoreUSD::ShaderAlgo::writeShaderNetwork( const IECoreScene return networkOutUsd; } +bool IECoreUSD::ShaderAlgo::canReadShaderNetwork( const pxr::UsdShadeOutput &output ) +{ + pxr::UsdShadeConnectableAPI usdSource; + pxr::TfToken usdSourceName; + pxr::UsdShadeAttributeType usdSourceType; + if( + !output.GetConnectedSource( &usdSource, &usdSourceName, &usdSourceType ) || + usdSourceType != pxr::UsdShadeAttributeType::Output + ) + { + return false; + } + + return true; +} + IECoreScene::ShaderNetworkPtr IECoreUSD::ShaderAlgo::readShaderNetwork( const pxr::UsdShadeOutput &output ) { pxr::UsdShadeConnectableAPI usdSource; @@ -322,7 +338,7 @@ IECoreScene::ShaderNetworkPtr IECoreUSD::ShaderAlgo::readShaderNetwork( const px usdSourceType != pxr::UsdShadeAttributeType::Output ) { - return new IECoreScene::ShaderNetwork(); + return nullptr; } IECoreScene::ShaderNetworkPtr result = new IECoreScene::ShaderNetwork(); diff --git a/contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp b/contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp index 5832607d37..b6b9536885 100644 --- a/contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp +++ b/contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp @@ -469,7 +469,7 @@ class ShaderNetworkCache : public LRUCacheObject::memoryUsage(); + cost = result ? result ->Object::memoryUsage() : 0; return result; } @@ -950,7 +950,7 @@ bool USDScene::hasAttribute( const SceneInterface::Name &name ) const { if( pxr::UsdShadeOutput o = mat.GetOutput( output ) ) { - return o.GetAttr().IsAuthored(); + return ShaderAlgo::canReadShaderNetwork( o ); } } return false; @@ -1014,6 +1014,10 @@ void USDScene::attributeNames( SceneInterface::NameList &attrs ) const { for( pxr::UsdShadeOutput &o : mat.GetOutputs( /* onlyAuthored = */ true ) ) { + if( !ShaderAlgo::canReadShaderNetwork( o ) ) + { + continue; + } InternedString attrName = AttributeAlgo::nameFromUSD( { o.GetBaseName() , false } ); if( !purpose.IsEmpty() ) { @@ -1107,8 +1111,7 @@ ConstObjectPtr USDScene::readAttribute( const SceneInterface::Name &name, double const auto &[output, purpose] = materialOutputAndPurpose( name.string() ); if( pxr::UsdShadeMaterial mat = m_root->computeBoundMaterial( m_location->prim, purpose ) ) { - pxr::UsdShadeOutput o = mat.GetOutput( output ); - if( o && o.GetAttr().IsAuthored() ) + if( pxr::UsdShadeOutput o = mat.GetOutput( output ) ) { return m_root->readShaderNetwork( o ); } diff --git a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py index 96357464f1..90ddf9343f 100644 --- a/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py +++ b/contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py @@ -2681,7 +2681,8 @@ def testShaders( self ) : oneShaderNetwork.addShader( "foo", surface ) oneShaderNetwork.setOutput( IECoreScene.ShaderNetwork.Parameter( "foo", "" ) ) - # A network with no output can be written out, but it will read back in as empty + # A network with no output can be written out, but not read back in, because + # it will not have been connected to a material output. noOutputNetwork = IECoreScene.ShaderNetwork() noOutputNetwork.addShader( "foo", surface ) @@ -2868,14 +2869,14 @@ def testShaders( self ) : root = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Read ) - self.assertEqual( set( root.child( "shaderLocation" ).attributeNames() ), set( ['ai:disp_map', 'ai:surface', 'complex:surface', 'testBad:surface', 'volume', 'componentConnection:surface', 'manualComponent:surface' ] ) ) + self.assertEqual( set( root.child( "shaderLocation" ).attributeNames() ), set( ['ai:disp_map', 'ai:surface', 'complex:surface', 'volume', 'componentConnection:surface', 'manualComponent:surface' ] ) ) self.assertEqual( root.child( "shaderLocation" ).readAttribute( "ai:surface", 0 ).outputShader().parameters, oneShaderNetwork.outputShader().parameters ) self.assertEqual( root.child( "shaderLocation" ).readAttribute( "ai:surface", 0 ).outputShader(), oneShaderNetwork.outputShader() ) self.assertEqual( root.child( "shaderLocation" ).readAttribute( "ai:surface", 0 ), oneShaderNetwork ) - self.assertTrue( root.child( "shaderLocation" ).hasAttribute( "testBad:surface" ) ) + self.assertFalse( root.child( "shaderLocation" ).hasAttribute( "testBad:surface" ) ) - self.assertEqual( root.child( "shaderLocation" ).readAttribute( "testBad:surface", 0 ), IECoreScene.ShaderNetwork() ) + self.assertEqual( root.child( "shaderLocation" ).readAttribute( "testBad:surface", 0 ), None ) self.assertEqual( root.child( "shaderLocation" ).readAttribute( "ai:disp_map", 0 ), pickOutputNetwork ) self.assertEqual( root.child( "shaderLocation" ).hasAttribute( "ai:volume" ), False ) self.assertEqual( root.child( "shaderLocation" ).readAttribute( "volume", 0 ), oneShaderNetwork ) @@ -3667,5 +3668,17 @@ def testMaterialBindingInsideInstance( self ) : shader2 = instance2.readAttribute( "surface", 0.0, _copy = False ) self.assertTrue( shader1.isSame( shader2 ) ) + def testUnconnectedMaterialOutput( self ) : + + root = IECoreScene.SceneInterface.create( + os.path.join( os.path.dirname( __file__ ), "data", "unconnectedMaterialOutput.usda" ), + IECore.IndexedIO.OpenMode.Read + ) + + sphere = root.child( "sphere" ) + self.assertFalse( sphere.hasAttribute( "cycles:surface" ) ) + self.assertNotIn( "cycles:surface", sphere.attributeNames() ) + self.assertIsNone( sphere.readAttribute( "cycles:surface", 0 ) ) + if __name__ == "__main__": unittest.main() diff --git a/contrib/IECoreUSD/test/IECoreUSD/data/unconnectedMaterialOutput.usda b/contrib/IECoreUSD/test/IECoreUSD/data/unconnectedMaterialOutput.usda new file mode 100644 index 0000000000..97a7511f87 --- /dev/null +++ b/contrib/IECoreUSD/test/IECoreUSD/data/unconnectedMaterialOutput.usda @@ -0,0 +1,13 @@ +#usda 1.0 + +def Sphere "sphere" ( + prepend apiSchemas = ["MaterialBindingAPI"] +) +{ + rel material:binding = +} + +def Material "myMaterial" +{ + token outputs:cycles:surface +}