Merge pull request #1385 from johnhaddon/usdShaderFix

USDScene : Don't treat unconnected material outputs as attributes

Author: johnhaddon

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

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.

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

contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp

@@ -469,7 +469,7 @@ class ShaderNetworkCache : public LRUCache<pxr::SdfPath, IECoreScene::ConstShade
 		static IECoreScene::ConstShaderNetworkPtr getter( const ShaderNetworkCacheGetterKey &key, size_t &cost )
 		{
 			IECoreScene::ConstShaderNetworkPtr result = ShaderAlgo::readShaderNetwork( key );
-			cost = result->Object::memoryUsage();
+			cost = result ? result ->Object::memoryUsage() : 0;
 			return result;
 		}
 
@@ -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
@@ -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 );
 			}

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

contrib/IECoreUSD/test/IECoreUSD/data/unconnectedMaterialOutput.usda

@@ -0,0 +1,13 @@
+#usda 1.0
+
+def Sphere "sphere" (
+    prepend apiSchemas = ["MaterialBindingAPI"]
+)
+{
+    rel material:binding = </myMaterial>
+}
+
+def Material "myMaterial"
+{
+    token outputs:cycles:surface
+}