USD ShaderAlgo : Refactor shader reading

- Simplify `readShaderNetwork()` public interface, and move "does the output have a connection?" logic from USDScene into ShaderAlgo.
- Introduce `readShaderNetworkWalk()` overload which takes an output and returns a handle for that output. This removes the repetition of the `DEFAULT_OUTPUT` special case.
- Simplify connection handling by building `vector<Connection>` directly, rather than packing and unpacking a recipe for making a connection.
- Remove unused `inputs` variable.
- Remove bogus comment - there is no such variable as `handles[0]`.

Author: johnhaddon

contrib/IECoreUSD/include/IECoreUSD/ShaderAlgo.h

@@ -53,12 +53,8 @@ namespace ShaderAlgo
 /// Write ShaderNetwork to USD, placing the shaders under the Prim `shaderContainer`
 IECOREUSD_API pxr::UsdShadeOutput writeShaderNetwork( const IECoreScene::ShaderNetwork *shaderNetwork, pxr::UsdPrim shaderContainer );
 
-/// Read ShaderNetwork from a USD node ( and its connected inputs )
-/// `anchorPath` is the ancestor path that shaders will be named relative to
-/// `outputHandle` specifies which output of the USD node is being used ( the ShaderNetwork must have
-/// a corresponding output set )
-IECoreScene::ShaderNetworkPtr readShaderNetwork( const pxr::SdfPath &anchorPath, const pxr::UsdShadeShader &outputShader, const pxr::TfToken &outputHandle );
-
+/// Reads a ShaderNetwork from a material output, typically obtained from `UsdShadeMaterial::GetOutput()`.
+IECoreScene::ShaderNetworkPtr readShaderNetwork( const pxr::UsdShadeOutput &output );
 
 
 } // namespace ShaderAlgo

contrib/IECoreUSD/src/IECoreUSD/ShaderAlgo.cpp

@@ -53,6 +53,8 @@ namespace
 
 pxr::TfToken g_adapterLabelToken( IECoreScene::ShaderNetworkAlgo::componentConnectionAdapterLabel().string() );
 
+IECoreScene::ShaderNetwork::Parameter readShaderNetworkWalk( const pxr::SdfPath &anchorPath, const pxr::UsdShadeOutput &output, IECoreScene::ShaderNetwork &shaderNetwork );
+
 IECore::InternedString readShaderNetworkWalk( const pxr::SdfPath &anchorPath, const pxr::UsdShadeShader &usdShader, IECoreScene::ShaderNetwork &shaderNetwork )
 {
 	IECore::InternedString handle( usdShader.GetPath().MakeRelativePath( anchorPath ).GetString() );
@@ -84,8 +86,7 @@ IECore::InternedString readShaderNetworkWalk( const pxr::SdfPath &anchorPath, co
 
 	IECore::CompoundDataPtr parametersData = new IECore::CompoundData();
 	IECore::CompoundDataMap &parameters = parametersData->writable();
-	std::vector< std::tuple< IECore::InternedString, pxr::UsdShadeConnectableAPI, IECore::InternedString > > connections;
-	std::vector< pxr::UsdShadeInput > inputs = usdShader.GetInputs();
+	std::vector<IECoreScene::ShaderNetwork::Connection> connections;
 	for( pxr::UsdShadeInput &i : usdShader.GetInputs() )
 	{
 		pxr::UsdShadeConnectableAPI usdSource;
@@ -95,9 +96,14 @@ IECore::InternedString readShaderNetworkWalk( const pxr::SdfPath &anchorPath, co
 		pxr::UsdAttribute valueAttribute = i;
 		if( i.GetConnectedSource( &usdSource, &usdSourceName, &usdSourceType ) )
 		{
-			if( !usdSource.IsContainer() )
+			if( usdSourceType == pxr::UsdShadeAttributeType::Output )
 			{
-				connections.push_back( { i.GetBaseName().GetString(), usdSource, usdSourceName.GetString() } );
+				const IECoreScene::ShaderNetwork::Parameter sourceHandle = readShaderNetworkWalk(
+					anchorPath, usdSource.GetOutput( usdSourceName ), shaderNetwork
+				);
+				connections.push_back( {
+					sourceHandle, { handle, IECore::InternedString( i.GetBaseName().GetString() ) }
+				} );
 			}
 			else
 			{
@@ -124,33 +130,28 @@ IECore::InternedString readShaderNetworkWalk( const pxr::SdfPath &anchorPath, co
 	}
 	shaderNetwork.addShader( handle, std::move( newShader ) );
 
+	// Can only add connections after we've added the shader.
 	for( const auto &c : connections )
 	{
-		IECore::InternedString attributeName;
-		pxr::UsdShadeConnectableAPI usdSource;
-		IECore::InternedString sourceAttributeName;
-		std::tie( attributeName, usdSource, sourceAttributeName ) = c;
-		IECore::InternedString sourceHandle = readShaderNetworkWalk( anchorPath, pxr::UsdShadeShader( usdSource.GetPrim() ), shaderNetwork );
-
-		if( sourceAttributeName == "DEFAULT_OUTPUT" )
-		{
-			shaderNetwork.addConnection( IECoreScene::ShaderNetwork::Connection(
-					{ sourceHandle, "" },
-					{ handle, attributeName }
-			) );
-		}
-		else
-		{
-			shaderNetwork.addConnection( IECoreScene::ShaderNetwork::Connection(
-					{ sourceHandle, sourceAttributeName },
-					{ handle, attributeName }
-			) );
-		}
+		shaderNetwork.addConnection( c );
 	}
 
 	return handle;
 }
 
+IECoreScene::ShaderNetwork::Parameter readShaderNetworkWalk( const pxr::SdfPath &anchorPath, const pxr::UsdShadeOutput &output, IECoreScene::ShaderNetwork &shaderNetwork )
+{
+	IECore::InternedString shaderHandle = readShaderNetworkWalk( anchorPath, pxr::UsdShadeShader( output.GetPrim() ), shaderNetwork );
+	if( output.GetBaseName() != "DEFAULT_OUTPUT" )
+	{
+		return IECoreScene::ShaderNetwork::Parameter( shaderHandle, output.GetBaseName().GetString() );
+	}
+	else
+	{
+		return IECoreScene::ShaderNetwork::Parameter( shaderHandle );
+	}
+}
+
 } // namespace
 
 pxr::UsdShadeOutput IECoreUSD::ShaderAlgo::writeShaderNetwork( const IECoreScene::ShaderNetwork *shaderNetwork, pxr::UsdPrim shaderContainer )
@@ -249,10 +250,21 @@ pxr::UsdShadeOutput IECoreUSD::ShaderAlgo::writeShaderNetwork( const IECoreScene
 	return networkOutUsd;
 }
 
-IECoreScene::ShaderNetworkPtr IECoreUSD::ShaderAlgo::readShaderNetwork( const pxr::SdfPath &anchorPath, const pxr::UsdShadeShader &outputShader, const pxr::TfToken &outputParameter )
+IECoreScene::ShaderNetworkPtr IECoreUSD::ShaderAlgo::readShaderNetwork( const pxr::UsdShadeOutput &output  )
 {
+	pxr::UsdShadeConnectableAPI usdSource;
+	pxr::TfToken usdSourceName;
+	pxr::UsdShadeAttributeType usdSourceType;
+	if(
+		!output.GetConnectedSource( &usdSource, &usdSourceName, &usdSourceType ) ||
+		usdSourceType != pxr::UsdShadeAttributeType::Output
+	)
+	{
+		return new IECoreScene::ShaderNetwork();
+	}
+
 	IECoreScene::ShaderNetworkPtr result = new IECoreScene::ShaderNetwork();
-	IECore::InternedString outputHandle = readShaderNetworkWalk( anchorPath, outputShader, *result );
+	IECoreScene::ShaderNetwork::Parameter outputHandle = readShaderNetworkWalk( usdSource.GetPrim().GetParent().GetPath(), usdSource.GetOutput( usdSourceName ), *result );
 
 	// For the output shader, set the type to "ai:surface" if it is "ai:shader".
 	// This is complete nonsense - there is nothing to suggest that this shader is
@@ -265,22 +277,15 @@ IECoreScene::ShaderNetworkPtr IECoreUSD::ShaderAlgo::readShaderNetwork( const px
 	// don't use the suffix of the shader type for anything, and we should just set
 	// everything to prefix:shader ( aside from lights, which are a bit of a
 	// different question )
-	if( result->getShader( outputHandle )->getType() == "ai:shader" )
+	const IECoreScene::Shader *outputShader = result->getShader( outputHandle.shader );
+	if( outputShader->getType() == "ai:shader" )
 	{
-		IECoreScene::ShaderPtr o = result->getShader( outputHandle )->copy();
+		IECoreScene::ShaderPtr o = outputShader->copy();
 		o->setType( "ai:surface" );
-		result->setShader( outputHandle, std::move( o ) );
+		result->setShader( outputHandle.shader, std::move( o ) );
 	}
 
-	// handles[0] is the handle of the first shader added, which is always the output shader
-	if( outputParameter.GetString() != "DEFAULT_OUTPUT" )
-	{
-		result->setOutput( IECoreScene::ShaderNetwork::Parameter( outputHandle, outputParameter.GetString() ) );
-	}
-	else
-	{
-		result->setOutput( IECoreScene::ShaderNetwork::Parameter( outputHandle ) );
-	}
+	result->setOutput( outputHandle );
 
 	IECoreScene::ShaderNetworkAlgo::removeComponentConnectionAdapters( result.get() );
 

contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp

@@ -415,26 +415,7 @@ class ShaderNetworkCache : public LRUCache<pxr::SdfPath, IECoreScene::ConstShade
 
 		static IECoreScene::ConstShaderNetworkPtr getter( const ShaderNetworkCacheGetterKey &key, size_t &cost )
 		{
-			IECoreScene::ConstShaderNetworkPtr result;
-
-			/// \todo I'm pretty sure that the `readShaderNetwork()` signature is overly complex,
-			/// and it should just be passed a single `UsdShadeOutput &` like this function.
-			/// I suspect that `writeShaderNetwork()` could take a single `UsdShadeOutput &` too,
-			/// for symmetry between the two functions.
-
-			pxr::UsdShadeConnectableAPI source;
-			pxr::TfToken sourceName;
-			pxr::UsdShadeAttributeType sourceType;
-			if( key.GetConnectedSource( &source, &sourceName, &sourceType ) )
-			{
-				pxr::UsdShadeShader s( source.GetPrim() );
-				result = ShaderAlgo::readShaderNetwork( source.GetPrim().GetParent().GetPath(), s, sourceName );
-			}
-			else
-			{
-				result = new IECoreScene::ShaderNetwork();
-			}
-
+			IECoreScene::ConstShaderNetworkPtr result = ShaderAlgo::readShaderNetwork( key );
 			cost = result->Object::memoryUsage();
 			return result;
 		}