USDScene : Fix writing of UsdLux lights

We were writing them as regular material assignments before, which might have round-tripped back into Gaffer OK, but which were not what other USD clients needed. We need to make sure that the primary shader is written as a `UsdLux*Light` prim, and that any input shaders are stored as children of that prim.

We still share most of the implementation with the generic material-writing code, but with differences in orchestration. It would have been possible to continue to do this with one `writeShaderNetwork()` uber-function internally with a bunch of conditionals depending on whether or not we were writing lights. But I felt that it was clearer and more maintainable to have two small top-level functions that each made appropriate calls to a set of shared utility functions.

Author: johnhaddon

Changes

@@ -9,6 +9,7 @@ Improvements
 Fixes
 -----
 
+- USDScene : Fixed writing of lights so they are represented as appropriate `UsdLux*Light` prims in USD.
 - FrameRange : Prevented creation of FrameRanges with negative steps
 - IECore.dataTypeFromElement : Fixed support for list to Vector conversions
 - LinkedScene : Fixed bug where `linkLocations` attribute was baked incorrectly if the link target location wasn't the ROOT

contrib/IECoreUSD/include/IECoreUSD/ShaderAlgo.h

@@ -60,8 +60,10 @@ IECOREUSD_API pxr::UsdShadeOutput writeShaderNetwork( const IECoreScene::ShaderN
 IECOREUSD_API IECoreScene::ShaderNetworkPtr readShaderNetwork( const pxr::UsdShadeOutput &output );
 
 #if PXR_VERSION >= 2111
+/// Writes a UsdLuxLight from a shader network.
+IECOREUSD_API void writeLight( const IECoreScene::ShaderNetwork *shaderNetwork, pxr::UsdPrim prim );
 /// Reads a ShaderNetwork from a light.
-IECOREUSD_API IECoreScene::ShaderNetworkPtr readShaderNetwork( const pxr::UsdLuxLightAPI &light );
+IECOREUSD_API IECoreScene::ShaderNetworkPtr readLight( const pxr::UsdLuxLightAPI &light );
 #endif
 
 } // namespace ShaderAlgo

contrib/IECoreUSD/src/IECoreUSD/ShaderAlgo.cpp

@@ -43,7 +43,10 @@
 
 #if PXR_VERSION >= 2111
 #include "pxr/usd/usdLux/cylinderLight.h"
+#include "pxr/usd/usdLux/nonboundableLightBase.h"
 #include "pxr/usd/usdLux/sphereLight.h"
+
+#include "pxr/usd/usd/schemaRegistry.h"
 #endif
 
 #include "boost/algorithm/string/replace.hpp"
@@ -217,15 +220,89 @@ IECoreScene::ShaderNetwork::Parameter readShaderNetworkWalk( const pxr::SdfPath
 	}
 }
 
+IECoreScene::ConstShaderNetworkPtr adaptShaderNetworkForWriting( const IECoreScene::ShaderNetwork *shaderNetwork )
+{
+	IECoreScene::ShaderNetworkPtr result = shaderNetwork->copy();
+	IECoreScene::ShaderNetworkAlgo::expandSplines( result.get() );
+	IECoreScene::ShaderNetworkAlgo::addComponentConnectionAdapters( result.get() );
+	return result;
+}
+
+pxr::UsdShadeConnectableAPI createShaderPrim( const IECoreScene::Shader *shader, const pxr::UsdStagePtr &stage, const pxr::SdfPath &path )
+{
+	pxr::UsdShadeShader usdShader = pxr::UsdShadeShader::Define( stage, path );
+	if( !usdShader )
+	{
+		throw IECore::Exception( "Could not create shader at " + path.GetAsString() );
+	}
+	const std::string type = shader->getType();
+	std::string typePrefix;
+	size_t typeColonPos = type.find( ":" );
+	if( typeColonPos != std::string::npos )
+	{
+		typePrefix = type.substr( 0, typeColonPos ) + ":";
+		if( typePrefix == "ai:" )
+		{
+			typePrefix = "arnold:";
+		}
+	}
+	usdShader.SetShaderId( pxr::TfToken( typePrefix + shader->getName() ) );
+
+	return usdShader.ConnectableAPI();
+}
+
+void writeShaderParameterValues( const IECoreScene::Shader *shader, pxr::UsdShadeConnectableAPI usdShader )
+{
+	for( const auto &p : shader->parametersData()->readable() )
+	{
+		pxr::UsdShadeInput input = usdShader.CreateInput(
+			toUSDParameterName( p.first ),
+			IECoreUSD::DataAlgo::valueTypeName( p.second.get() )
+		);
+		input.Set( IECoreUSD::DataAlgo::toUSD( p.second.get() ) );
+	}
+
+	const IECore::BoolData *adapterMeta = shader->blindData()->member<IECore::BoolData>( IECoreScene::ShaderNetworkAlgo::componentConnectionAdapterLabel() );
+	if( adapterMeta && adapterMeta->readable() )
+	{
+		usdShader.GetPrim().SetMetadata( g_adapterLabelToken, true );
+	}
+}
+
+using ShaderMap = std::unordered_map<IECore::InternedString, pxr::UsdShadeConnectableAPI>;
+void writeShaderConnections( const IECoreScene::ShaderNetwork *shaderNetwork, const ShaderMap &usdShaders )
+{
+	for( const auto &shader : shaderNetwork->shaders() )
+	{
+		pxr::UsdShadeConnectableAPI usdShader = usdShaders.at( shader.first );
+		for( const auto &c : shaderNetwork->inputConnections( shader.first ) )
+		{
+			pxr::UsdShadeInput dest = usdShader.GetInput( pxr::TfToken( c.destination.name.string() ) );
+			if( !dest )
+			{
+				dest = usdShader.CreateInput( toUSDParameterName( c.destination.name ), pxr::SdfValueTypeNames->Token );
+			}
+
+			pxr::UsdShadeShader sourceUsdShader = usdShaders.at( c.source.shader );
+			std::string sourceOutputName = c.source.name.string();
+			if( sourceOutputName.size() == 0 )
+			{
+				sourceOutputName = "DEFAULT_OUTPUT";
+			}
+			pxr::UsdShadeOutput source = sourceUsdShader.CreateOutput( pxr::TfToken( sourceOutputName ), dest.GetTypeName() );
+			dest.ConnectToSource( source );
+		}
+	}
+}
+
 } // namespace
 
 pxr::UsdShadeOutput IECoreUSD::ShaderAlgo::writeShaderNetwork( const IECoreScene::ShaderNetwork *shaderNetwork, pxr::UsdPrim shaderContainer )
 {
-	IECoreScene::ShaderNetworkPtr shaderNetworkWithAdapters = shaderNetwork->copy();
-	IECoreScene::ShaderNetworkAlgo::expandSplines( shaderNetworkWithAdapters.get() );
-	IECoreScene::ShaderNetworkAlgo::addComponentConnectionAdapters( shaderNetworkWithAdapters.get() );
+	IECoreScene::ConstShaderNetworkPtr adaptedNetwork = adaptShaderNetworkForWriting( shaderNetwork );
+	shaderNetwork = adaptedNetwork.get();
 
-	IECoreScene::ShaderNetwork::Parameter networkOutput = shaderNetworkWithAdapters->getOutput();
+	IECoreScene::ShaderNetwork::Parameter networkOutput = shaderNetwork->getOutput();
 	if( networkOutput.shader.string() == "" )
 	{
 		// This could theoretically happen, but a shader network with no output is not useful in any way
@@ -235,36 +312,14 @@ pxr::UsdShadeOutput IECoreUSD::ShaderAlgo::writeShaderNetwork( const IECoreScene
 		);
 	}
 
+	ShaderMap usdShaders;
 	pxr::UsdShadeOutput networkOutUsd;
-	for( const auto &shader : shaderNetworkWithAdapters->shaders() )
+	for( const auto &shader : shaderNetwork->shaders() )
 	{
-		pxr::SdfPath usdShaderPath = shaderContainer.GetPath().AppendChild( pxr::TfToken( pxr::TfMakeValidIdentifier( shader.first.string() ) ) );
-		pxr::UsdShadeShader usdShader = pxr::UsdShadeShader::Define( shaderContainer.GetStage(), usdShaderPath );
-		if( !usdShader )
-		{
-			throw IECore::Exception( "Could not create shader at: " + shaderContainer.GetPath().GetString() + " / " + shader.first.string() );
-		}
-		std::string type = shader.second->getType();
-		std::string typePrefix;
-		size_t typeColonPos = type.find( ":" );
-		if( typeColonPos != std::string::npos )
-		{
-			typePrefix = type.substr( 0, typeColonPos ) + ":";
-			if( typePrefix == "ai:" )
-			{
-				typePrefix = "arnold:";
-			}
-		}
-		usdShader.SetShaderId( pxr::TfToken( typePrefix + shader.second->getName() ) );
-
-		for( const auto &p : shader.second->parametersData()->readable() )
-		{
-			pxr::UsdShadeInput input = usdShader.CreateInput(
-				toUSDParameterName( p.first ),
-				DataAlgo::valueTypeName( p.second.get() )
-			);
-			input.Set( DataAlgo::toUSD( p.second.get() ) );
-		}
+		const pxr::SdfPath usdShaderPath = shaderContainer.GetPath().AppendChild( pxr::TfToken( pxr::TfMakeValidIdentifier( shader.first.string() ) ) );
+		pxr::UsdShadeConnectableAPI usdShader = createShaderPrim( shader.second.get(), shaderContainer.GetStage(), usdShaderPath );
+		writeShaderParameterValues( shader.second.get(), usdShader );
+		usdShaders[shader.first] = usdShader;
 
 		if( networkOutput.shader == shader.first )
 		{
@@ -278,36 +333,9 @@ pxr::UsdShadeOutput IECoreUSD::ShaderAlgo::writeShaderNetwork( const IECoreScene
 			// Currently, we don't really track output types in Gaffer.
 			networkOutUsd = usdShader.CreateOutput( outName, pxr::SdfValueTypeNames->Token );
 		}
-
-		const IECore::BoolData* adapterMeta = shader.second->blindData()->member<IECore::BoolData>( IECoreScene::ShaderNetworkAlgo::componentConnectionAdapterLabel() );
-		if( adapterMeta && adapterMeta->readable() )
-		{
-			usdShader.GetPrim().SetMetadata( g_adapterLabelToken, true );
-		}
 	}
 
-	for( const auto &shader : shaderNetworkWithAdapters->shaders() )
-	{
-		pxr::UsdShadeShader usdShader = pxr::UsdShadeShader::Get( shaderContainer.GetStage(), shaderContainer.GetPath().AppendChild( pxr::TfToken( pxr::TfMakeValidIdentifier( shader.first.string() ) ) ) );
-		for( const auto &c : shaderNetworkWithAdapters->inputConnections( shader.first ) )
-		{
-			pxr::UsdShadeInput dest = usdShader.GetInput( pxr::TfToken( c.destination.name.string() ) );
-			if( ! dest.GetPrim().IsValid() )
-			{
-				dest = usdShader.CreateInput( toUSDParameterName( c.destination.name ), pxr::SdfValueTypeNames->Token );
-			}
-
-			pxr::UsdShadeShader sourceUsdShader = pxr::UsdShadeShader::Get( shaderContainer.GetStage(), shaderContainer.GetPath().AppendChild( pxr::TfToken( pxr::TfMakeValidIdentifier( c.source.shader.string() ) ) ) );
-			std::string sourceOutputName = c.source.name.string();
-			if( sourceOutputName.size() == 0 )
-			{
-				sourceOutputName = "DEFAULT_OUTPUT";
-			}
-			pxr::UsdShadeOutput source = sourceUsdShader.CreateOutput( pxr::TfToken( sourceOutputName ), dest.GetTypeName() );
-			dest.ConnectToSource( source );
-		}
-
-	}
+	writeShaderConnections( shaderNetwork, usdShaders );
 
 	return networkOutUsd;
 }
@@ -357,7 +385,63 @@ IECoreScene::ShaderNetworkPtr IECoreUSD::ShaderAlgo::readShaderNetwork( const px
 
 #if PXR_VERSION >= 2111
 
-IECoreScene::ShaderNetworkPtr IECoreUSD::ShaderAlgo::readShaderNetwork( const pxr::UsdLuxLightAPI &light )
+// This is very similar to `writeShaderNetwork` but with these key differences :
+//
+// - The output shader is written as a UsdLight-derived prim rather than a UsdShadeShader.
+// - The other shaders are parented inside the light.
+// - We don't need to create a UsdShadeOutput to return.
+void IECoreUSD::ShaderAlgo::writeLight( const IECoreScene::ShaderNetwork *shaderNetwork, pxr::UsdPrim prim )
+{
+	IECoreScene::ConstShaderNetworkPtr adaptedNetwork = adaptShaderNetworkForWriting( shaderNetwork );
+	shaderNetwork = adaptedNetwork.get();
+
+	// Verify that the light shader corresponds to a valid USD light type.
+
+	const IECoreScene::Shader *outputShader = shaderNetwork->outputShader();
+	if( !outputShader )
+	{
+		IECore::msg( IECore::Msg::Warning, "ShaderAlgo::writeLight", "No output shader" );
+		return;
+	}
+
+	pxr::TfType type = pxr::UsdSchemaRegistry::GetInstance().GetTypeFromName( pxr::TfToken( outputShader->getName() ) );
+	if(
+		!type.IsA<pxr::UsdLuxBoundableLightBase>() &&
+		!type.IsA<pxr::UsdLuxNonboundableLightBase>()
+	)
+	{
+		IECore::msg( IECore::Msg::Warning, "ShaderAlgo::writeLight", boost::format( "Shader `%1%` is not a valid UsdLux light type" ) % outputShader->getName() );
+		return;
+	}
+
+	// Write the light itself onto the prim we've been given.
+
+	ShaderMap usdShaders;
+	prim.SetTypeName( pxr::TfToken( outputShader->getName() ) );
+	writeShaderParameterValues( outputShader, pxr::UsdShadeConnectableAPI( prim ) );
+	usdShaders[shaderNetwork->getOutput().shader] = pxr::UsdShadeConnectableAPI( prim );
+
+	// Then write any other shaders as child prims so they are
+	// encapsulated within the light.
+
+	for( const auto &shader : shaderNetwork->shaders() )
+	{
+		if( shader.second == outputShader )
+		{
+			continue;
+		}
+		const pxr::SdfPath usdShaderPath = prim.GetPath().AppendChild( pxr::TfToken( pxr::TfMakeValidIdentifier( shader.first.string() ) ) );
+		pxr::UsdShadeConnectableAPI usdShader = createShaderPrim( shader.second.get(), prim.GetStage(), usdShaderPath );
+		writeShaderParameterValues( shader.second.get(), usdShader );
+		usdShaders[shader.first] = usdShader;
+	}
+
+	// Finally, connect everything up.
+
+	writeShaderConnections( shaderNetwork, usdShaders );
+}
+
+IECoreScene::ShaderNetworkPtr IECoreUSD::ShaderAlgo::readLight( const pxr::UsdLuxLightAPI &light )
 {
 	IECoreScene::ShaderNetworkPtr result = new IECoreScene::ShaderNetwork();
 	IECoreScene::ShaderNetwork::Parameter lightHandle = readShaderNetworkWalk( light.GetPath().GetParentPath(), pxr::UsdShadeConnectableAPI( light ), *result );

contrib/IECoreUSD/src/IECoreUSD/USDScene.cpp

@@ -1087,7 +1087,7 @@ ConstObjectPtr USDScene::readAttribute( const SceneInterface::Name &name, double
 #if PXR_VERSION >= 2111
 	else if( name == g_lightAttributeName )
 	{
-		return ShaderAlgo::readShaderNetwork( pxr::UsdLuxLightAPI( m_location->prim ) );
+		return ShaderAlgo::readLight( pxr::UsdLuxLightAPI( m_location->prim ) );
 	}
 #endif
 	else if( name == g_kindAttributeName )
@@ -1187,8 +1187,15 @@ void USDScene::writeAttribute( const SceneInterface::Name &name, const Object *a
 	}
 	else if( const IECoreScene::ShaderNetwork *shaderNetwork = runTimeCast<const ShaderNetwork>( attribute ) )
 	{
-		const auto &[output, purpose] = materialOutputAndPurpose( name.string() );
-		m_materials[purpose][output] = shaderNetwork;
+		if( name == g_lightAttributeName )
+		{
+			ShaderAlgo::writeLight( shaderNetwork, m_location->prim );
+		}
+		else
+		{
+			const auto &[output, purpose] = materialOutputAndPurpose( name.string() );
+			m_materials[purpose][output] = shaderNetwork;
+		}
 	}
 	else if( name.string() == "gaffer:globals" )
 	{

contrib/IECoreUSD/test/IECoreUSD/USDSceneTest.py

@@ -3367,6 +3367,83 @@ def testMultipleLights( self ) :
 			self.assertIsInstance( attribute, IECoreScene.ShaderNetwork )
 			self.assertEqual( attribute.outputShader().parameters["exposure"], IECore.FloatData( exposure ) )
 
+	def testWriteLight( self ) :
+
+		fileName = os.path.join( self.temporaryDirectory(), "pointInstancePrimvars.usda" )
+		scene = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Write )
+
+		light = scene.createChild( "light" )
+		light.writeAttribute(
+			"light",
+			IECoreScene.ShaderNetwork(
+				shaders = {
+					"output" : IECoreScene.Shader(
+						"SphereLight", "light",
+						{
+							"exposure" : 3.0,
+							"radius" : 1.5,
+							"color" : imath.Color3f( 1, 0.5, 0.25 ),
+						}
+					)
+				},
+				output = "output",
+			),
+			0
+		)
+
+		del light, scene
+
+		stage = pxr.Usd.Stage.Open( fileName )
+		light = pxr.UsdLux.SphereLight( stage.GetPrimAtPath( "/light" ) )
+		self.assertTrue( light )
+
+		self.assertEqual( light.GetExposureAttr().Get(), 3.0 )
+		self.assertEqual( light.GetRadiusAttr().Get(), 1.5 )
+		self.assertEqual( light.GetColorAttr().Get(), pxr.Gf.Vec3f( 1.0, 0.5, 0.25 ) )
+		self.assertEqual( light.GetPrim().GetChildren(), [] )
+
+	def testWriteLightWithInputNetwork( self ) :
+
+		# Write to USD
+
+		fileName = os.path.join( self.temporaryDirectory(), "pointInstancePrimvars.usda" )
+		scene = IECoreScene.SceneInterface.create( fileName, IECore.IndexedIO.OpenMode.Write )
+
+		lightNetwork = IECoreScene.ShaderNetwork(
+			shaders = {
+				"output" : IECoreScene.Shader( "RectLight", "light" ),
+				"texture" : IECoreScene.Shader(
+					"UsdUVTexture", "shader",
+					{
+						"file" : "test.exr",
+					}
+				),
+			},
+			connections = [
+				( ( "texture", "rgb" ), ( "output", "color" ) )
+			],
+			output = "output",
+		)
+
+		light = scene.createChild( "light" )
+		light.writeAttribute( "light", lightNetwork, 0 )
+
+		del light, scene
+
+		# Verify via USD API
+
+		stage = pxr.Usd.Stage.Open( fileName )
+		self.assertTrue( pxr.UsdLux.RectLight( stage.GetPrimAtPath( "/light" ) ) )
+
+		light = pxr.UsdLux.LightAPI( stage.GetPrimAtPath( "/light" ) )
+		source, sourceName, sourceType = light.GetInput( "color" ).GetConnectedSource()
+		self.assertEqual( source.GetPrim().GetName(), "texture" )
+		self.assertEqual( source.GetPrim().GetParent(), light.GetPrim() )
+		self.assertEqual( sourceName, "rgb" )
+		self.assertEqual( sourceType, pxr.UsdShade.AttributeType.Output )
+
+		self.assertEqual( light.GetPrim().GetChildren(), [ source.GetPrim() ] )
+
 	def testPointInstancerPrimvars( self ) :
 
 		# Use the USD API to author a point instancer with primvars on it.