CollectImages : Add addLayerPrefix plug

This is motivated by the discussion at https://groups.google.com/u/1/g/gaffer-dev/c/kJQ5fuKIHP8, where folks are trying to collect light group outputs from Arnold, where they are inevitably already prefixed with the layer name in the EXR file.

Changes.md

@@ -13,6 +13,7 @@ Improvements
 
 - LightTool : Changed spot light and quad light edge tool tip locations so that they follow the cone and edge during drag.
 - Arnold : Improved speed of translation of encapsulated scenes when using many threads.
+- CollectImages : Added `addLayerPrefix` plug, to allow the layer prefix to be omitted in the case that the input images are already prefixed.
 
 Fixes
 -----

include/GafferImage/CollectImages.h

@@ -59,6 +59,9 @@ class GAFFERIMAGE_API CollectImages : public ImageProcessor
 		Gaffer::StringPlug *layerVariablePlug();
 		const Gaffer::StringPlug *layerVariablePlug() const;
 
+		Gaffer::BoolPlug *addLayerPrefixPlug();
+		const Gaffer::BoolPlug *addLayerPrefixPlug() const;
+
 		Gaffer::BoolPlug *mergeMetadataPlug();
 		const Gaffer::BoolPlug *mergeMetadataPlug() const;
 

python/GafferImageTest/CollectImagesTest.py

@@ -151,10 +151,24 @@ def testLayerMapping( self ) :
 		self.assertEqual( sampler["color"].getValue(), imath.Color4f( 0.1, 0.2, 0.3, 0.4 ) )
 
 		# Test simple duplicate
+
+		def assertExpectedMessages( messageHandler, layerName, channelNames ) :
+
+			self.assertEqual( [ m.level for m in messageHandler.messages ], [ IECore.Msg.Level.Warning ] * len( channelNames ) )
+			self.assertEqual( [ m.context for m in messageHandler.messages ], [ "CollectImages" ] * len( channelNames ) )
+
+			self.assertEqual(
+				[ m.message for m in messageHandler.messages ],
+				[ "Ignoring duplicate channel \"{}\" from layer \"{}\"".format( c, layerName ) for c in channelNames ]
+			)
+
 		collect["rootLayers"].setValue( IECore.StringVectorData( [ 'A', 'A' ] ) )
 
-		self.assertEqual( list(collect["out"]["channelNames"].getValue()), [ "A.R", "A.G", "A.B", "A.A" ] )
-		self.assertEqual( sampler["color"].getValue(), imath.Color4f( 0.1, 0.2, 0.3, 0.4 ) )
+		with IECore.CapturingMessageHandler() as mh :
+			self.assertEqual( list(collect["out"]["channelNames"].getValue()), [ "A.R", "A.G", "A.B", "A.A" ] )
+			self.assertEqual( sampler["color"].getValue(), imath.Color4f( 0.1, 0.2, 0.3, 0.4 ) )
+
+		assertExpectedMessages( mh, "A", [ "A.R", "A.G", "A.B", "A.A" ] )
 
 		collect["rootLayers"].setValue( IECore.StringVectorData( [ 'A', 'B' ] ) )
 		self.assertEqual( list(collect["out"]["channelNames"].getValue()), [
@@ -165,13 +179,24 @@ def testLayerMapping( self ) :
 		self.assertEqual( sampler["color"].getValue(), imath.Color4f( 0.2, 0.4, 0.6, 0.8 ) )
 
 		# Test overlapping names take the first layer
+
 		constant1["layer"].setValue( "B" )
 		collect["rootLayers"].setValue( IECore.StringVectorData( [ 'A', 'A.B' ] ) )
 		sampler["channels"].setValue( IECore.StringVectorData( [ "A.B.R", "A.B.G","A.B.B","A.B.A" ] ) )
-		self.assertEqual( list(collect["out"]["channelNames"].getValue()), [ "A.B.R", "A.B.G", "A.B.B", "A.B.A" ] )
+
+		with IECore.CapturingMessageHandler() as mh :
+			self.assertEqual( list(collect["out"]["channelNames"].getValue()), [ "A.B.R", "A.B.G", "A.B.B", "A.B.A" ] )
+
+		assertExpectedMessages( mh, "A.B", [ "A.B.R", "A.B.G", "A.B.B", "A.B.A" ] )
+
 		self.assertEqual( sampler["color"].getValue(), imath.Color4f( 0.1, 0.2, 0.3, 0.4 ) )
 		collect["rootLayers"].setValue( IECore.StringVectorData( [ 'A.B', 'A' ] ) )
-		self.assertEqual( list(collect["out"]["channelNames"].getValue()), [ "A.B.R", "A.B.G", "A.B.B", "A.B.A" ] )
+
+		with IECore.CapturingMessageHandler() as mh :
+			self.assertEqual( list(collect["out"]["channelNames"].getValue()), [ "A.B.R", "A.B.G", "A.B.B", "A.B.A" ] )
+
+		assertExpectedMessages( mh, "A", [ "A.B.R", "A.B.G", "A.B.B", "A.B.A" ] )
+
 		self.assertEqual( sampler["color"].getValue(), imath.Color4f( 0.2, 0.4, 0.6, 0.8 ) )
 
 	def testDeep( self ) :
@@ -273,5 +298,48 @@ def testHighLayerCountPerformance( self ) :
 		with GafferTest.TestRunner.PerformanceScope() :
 			GafferImageTest.processTiles( collect["out"] )
 
+	def testLayerPrefix( self ) :
+
+		# By default, we add a layer name prefix to all channel names.
+
+		constant = GafferImage.Constant()
+
+		collect = GafferImage.CollectImages()
+		collect["in"].setInput( constant["out"] )
+		collect["rootLayers"].setValue( IECore.StringVectorData( [ "diffuse", "specular" ] ) )
+
+		self.assertEqual(
+			collect["out"].channelNames(),
+			IECore.StringVectorData( [
+				"diffuse.R", "diffuse.G", "diffuse.B", "diffuse.A",
+				"specular.R", "specular.G", "specular.B", "specular.A",
+			] )
+		)
+
+		# But that is inconvenient if you've already got the layer name in the
+		# channel name.
+
+		constant["layer"].setValue( "${collect:layerName}" )
+
+		self.assertEqual(
+			collect["out"].channelNames(),
+			IECore.StringVectorData( [
+				"diffuse.diffuse.R", "diffuse.diffuse.G", "diffuse.diffuse.B", "diffuse.diffuse.A",
+				"specular.specular.R", "specular.specular.G", "specular.specular.B", "specular.specular.A",
+			] )
+		)
+
+		# So we let people turn it off.
+
+		collect["addLayerPrefix"].setValue( False )
+
+		self.assertEqual(
+			collect["out"].channelNames(),
+			IECore.StringVectorData( [
+				"diffuse.R", "diffuse.G", "diffuse.B", "diffuse.A",
+				"specular.R", "specular.G", "specular.B", "specular.A",
+			] )
+		)
+
 if __name__ == "__main__":
 	unittest.main()

python/GafferImageUI/CollectImagesUI.py

@@ -81,6 +81,17 @@
 
 		],
 
+		"addLayerPrefix" : [
+
+			"description",
+			"""
+			When on, the output channel names are automatically prefixed with
+			the name of the layer being collected. Should be turned off when
+			the input channel names already contain the layer name.
+			""",
+
+		],
+
 		"mergeMetadata" : [
 
 			"description",

src/GafferImage/CollectImages.cpp

@@ -85,27 +85,38 @@ class MappingData : public IECore::Data
 
 	public :
 
-		MappingData()
-			:	m_outputChannelNames( new StringVectorData )
+		MappingData( bool addLayerPrefix )
+			:	m_addLayerPrefix( addLayerPrefix ), m_outputChannelNames( new StringVectorData )
 		{
 		}
 
 		void addLayer( const string &layerName, const vector<string> &channelNames )
 		{
 			for( const auto &channelName : channelNames )
 			{
-				const string outputChannelName = ImageAlgo::channelName( layerName, channelName );
+				const string outputChannelName = m_addLayerPrefix ? ImageAlgo::channelName( layerName, channelName ) : channelName;
 				const Input input = { layerName, channelName };
-				// Duplicate channel names could arise because either :
-				//
-				// - The user entered the same layer name twice. In this case we ignore the second.
-				// - Name overlap due to complex hierachical naming, such as a layer named `A` with
-				//   a channel named `B.R` and a layer named `A.B` with a channel named `R`.
-				//   In this unlikely case, we just take the channel from the first layer.
 				if( m_mapping.try_emplace( outputChannelName, input ).second )
 				{
 					m_outputChannelNames->writable().push_back( outputChannelName );
 				}
+				else
+				{
+					// Duplicate channel names could arise for several reasons :
+					//
+					// - The user entered the same layer name twice.
+					// - Names overlap due to complex hierachical naming, such as a layer named `A` with
+					//   a channel named `B.R` and a layer named `A.B` with a channel named `R`.
+					// - `addLayerPrefix` is off, but the input channels do not have a suitable
+					//   prefix of their own.
+					//
+					// In all cases we take the first channel and ignore the duplicate, but we
+					// also emit a warning so the user can fix the setup.
+					msg(
+						Msg::Warning, "CollectImages",
+						fmt::format( "Ignoring duplicate channel \"{}\" from layer \"{}\"", outputChannelName, layerName )
+					);
+				}
 			}
 		}
 
@@ -129,6 +140,7 @@ class MappingData : public IECore::Data
 
 	private :
 
+		const bool m_addLayerPrefix;
 		StringVectorDataPtr m_outputChannelNames;
 
 		using Map = unordered_map<string, Input>;
@@ -155,6 +167,7 @@ CollectImages::CollectImages( const std::string &name )
 
 	addChild( new StringVectorDataPlug( "rootLayers", Plug::In, new StringVectorData ) );
 	addChild( new StringPlug( "layerVariable", Plug::In, "collect:layerName" ) );
+	addChild( new BoolPlug( "addLayerPrefix", Plug::In, true ) );
 	addChild( new BoolPlug( "mergeMetadata", Plug::In ) );
 	addChild( new ObjectPlug( "__mapping", Plug::Out, NullObject::defaultNullObject() ) );
 }
@@ -183,31 +196,42 @@ const Gaffer::StringPlug *CollectImages::layerVariablePlug() const
 	return getChild<Gaffer::StringPlug>( g_firstPlugIndex + 1 );
 }
 
-Gaffer::BoolPlug *CollectImages::mergeMetadataPlug()
+Gaffer::BoolPlug *CollectImages::addLayerPrefixPlug()
 {
 	return getChild<Gaffer::BoolPlug>( g_firstPlugIndex + 2 );
 }
 
-const Gaffer::BoolPlug *CollectImages::mergeMetadataPlug() const
+const Gaffer::BoolPlug *CollectImages::addLayerPrefixPlug() const
 {
 	return getChild<Gaffer::BoolPlug>( g_firstPlugIndex + 2 );
 }
 
+Gaffer::BoolPlug *CollectImages::mergeMetadataPlug()
+{
+	return getChild<Gaffer::BoolPlug>( g_firstPlugIndex + 3 );
+}
+
+const Gaffer::BoolPlug *CollectImages::mergeMetadataPlug() const
+{
+	return getChild<Gaffer::BoolPlug>( g_firstPlugIndex + 3 );
+}
+
 Gaffer::ObjectPlug *CollectImages::mappingPlug()
 {
-	return getChild<Gaffer::ObjectPlug>( g_firstPlugIndex + 3 );
+	return getChild<Gaffer::ObjectPlug>( g_firstPlugIndex + 4 );
 }
 
 const Gaffer::ObjectPlug *CollectImages::mappingPlug() const
 {
-	return getChild<Gaffer::ObjectPlug>( g_firstPlugIndex + 3 );
+	return getChild<Gaffer::ObjectPlug>( g_firstPlugIndex + 4 );
 }
 
 void CollectImages::affects( const Gaffer::Plug *input, AffectedPlugsContainer &outputs ) const
 {
 	ImageProcessor::affects( input, outputs );
 
 	if(
+		input == addLayerPrefixPlug() ||
 		input == layerVariablePlug() ||
 		input == rootLayersPlug() ||
 		input == inPlug()->channelNamesPlug()
@@ -283,6 +307,8 @@ void CollectImages::hash( const Gaffer::ValuePlug *output, const Gaffer::Context
 	{
 		ImageProcessor::hash( output, context, h );
 
+		addLayerPrefixPlug()->hash( h );
+
 		const std::string layerVariable = layerVariablePlug()->getValue();
 		Context::EditableScope layerScope( context );
 
@@ -304,7 +330,7 @@ void CollectImages::compute( Gaffer::ValuePlug *output, const Gaffer::Context *c
 {
 	if( output == mappingPlug() )
 	{
-		MappingDataPtr mapping = new MappingData;
+		MappingDataPtr mapping = new MappingData( addLayerPrefixPlug()->getValue() );
 
 		const std::string layerVariable = layerVariablePlug()->getValue();
 		Context::EditableScope layerScope( context );