DependencyNode::affects() is now called for output plugs too.

It's becoming a common pattern to have internal plugs which hold the result of some intermediate computation which is worth caching in its own right. This was achievable in two ways :

  - By having two internal plugs. The first, an output, holds the computed intermediate value. The second, an input, receives this value by a connection and allows its use in further computations. GafferScene::Group was using this method. This method was correct, but fiddly to set up and had extra overhead in the form of the second plug and connection.
  - By having a single internal output plug, which holds the intermediate value and is accessed directly in further computations. Because affects() was not called for this output plug, implementations had to cheat and directly have inputs affect the final outputs, bypassing the internal plug during dirty propagation. GafferOSL::OSLImage was using this method. This method was slightly dodgy, but easier to implement and understand.

By calling affects() for output plugs as well, we can now implement the simpler second mechanism without having to resort to naughtiness in affects().

Note that it is possible for lazy affects() methods which were not checking the identity of the input plug appropriately to be tripped up by this - it's worth checking any node implementations in external packages before accepting this commit.

include/Gaffer/ContextProcessor.inl

@@ -61,12 +61,15 @@ void ContextProcessor<BaseType>::affects( const Plug *input, DependencyNode::Aff
 {
 	BaseType::affects( input, outputs );
 
-	if( const ValuePlug *inputValuePlug = IECore::runTimeCast<const ValuePlug>( input ) )
+	if( input->direction() == Plug::In )
 	{
-		const ValuePlug *output = oppositePlug( inputValuePlug );
-		if( output ) 
+		if( const ValuePlug *inputValuePlug = IECore::runTimeCast<const ValuePlug>( input ) )
 		{
-			outputs.push_back( output );
+			const ValuePlug *output = oppositePlug( inputValuePlug );
+			if( output )
+			{
+				outputs.push_back( output );
+			}
 		}
 	}
 }

include/GafferScene/Group.h

@@ -90,9 +90,6 @@ class Group : public SceneProcessor
 		Gaffer::ObjectPlug *mappingPlug();
 		const Gaffer::ObjectPlug *mappingPlug() const;
 
-		Gaffer::ObjectPlug *inputMappingPlug();
-		const Gaffer::ObjectPlug *inputMappingPlug() const;
-
 		Gaffer::Behaviours::InputGenerator<ScenePlug> m_inPlugs;
 
 		static size_t g_firstPlugIndex;

python/GafferOSLTest/OSLImageTest.py

@@ -84,10 +84,11 @@ def test( self ) :
 		cs = GafferTest.CapturingSlot( image.plugDirtiedSignal() )
 		image["shader"].setInput( constant["out"] )
 
-		self.assertEqual( len( cs ), 3 )
+		self.assertEqual( len( cs ), 4 )
 		self.assertTrue( cs[0][0].isSame( image["shader"] ) )
-		self.assertTrue( cs[1][0].isSame( image["out"]["channelData"] ) )
-		self.assertTrue( cs[2][0].isSame( image["out"] ) )
+		self.assertTrue( cs[1][0].isSame( image["__shading"] ) )
+		self.assertTrue( cs[2][0].isSame( image["out"]["channelData"] ) )
+		self.assertTrue( cs[3][0].isSame( image["out"] ) )
 
 		inputImage = reader["out"].image()
 		outputImage = image["out"].image()
@@ -103,19 +104,21 @@ def test( self ) :
 
 		getGreen["parameters"]["channelName"].setValue( "R" )
 
-		self.assertEqual( len( cs ), 3 )
+		self.assertEqual( len( cs ), 4 )
 		self.assertTrue( cs[0][0].isSame( image["shader"] ) )
-		self.assertTrue( cs[1][0].isSame( image["out"]["channelData"] ) )
-		self.assertTrue( cs[2][0].isSame( image["out"] ) )
+		self.assertTrue( cs[1][0].isSame( image["__shading"] ) )
+		self.assertTrue( cs[2][0].isSame( image["out"]["channelData"] ) )
+		self.assertTrue( cs[3][0].isSame( image["out"] ) )
 
 		del cs[:]
 
 		buildColor["parameters"]["r"].setInput( getRed["out"]["channelValue"] )
 
-		self.assertEqual( len( cs ), 3 )
+		self.assertEqual( len( cs ), 4 )
 		self.assertTrue( cs[0][0].isSame( image["shader"] ) )
-		self.assertTrue( cs[1][0].isSame( image["out"]["channelData"] ) )
-		self.assertTrue( cs[2][0].isSame( image["out"] ) )
+		self.assertTrue( cs[1][0].isSame( image["__shading"] ) )
+		self.assertTrue( cs[2][0].isSame( image["out"]["channelData"] ) )
+		self.assertTrue( cs[3][0].isSame( image["out"] ) )
 
 		inputImage = reader["out"].image()
 		outputImage = image["out"].image()

python/GafferTest/ComputeNodeTest.py

@@ -318,10 +318,11 @@ def __init__( self, name="PassThrough", inputs={}, dynamicPlugs=() ) :
 			self.addChild( Gaffer.ObjectPlug( "out", Gaffer.Plug.Direction.Out, IECore.NullObject() ) )	
 
 		def affects( self, input ) :
-
-			assert( input.isSame( self["in"] ) )	
-
-			return [ self["out"] ]
+
+			if input.isSame( self["in"] ) :
+				return [ self["out"] ]
+
+			return []
 
 		def hash( self, output, context, h ) :
 

python/GafferTest/DependencyNodeTest.py

@@ -178,8 +178,10 @@ def __init__( self, name="PassThrough", inputs={}, dynamicPlugs=() ) :
 
 			def affects( self, input ) :
 
-				assert( input.isSame( self["in"] ) )	
-				return [ self["out"] ]
+				if input.isSame( self["in"] ) :
+					return [ self["out"] ]
+
+				return []
 
 		s1 = SimpleDependencyNode()
 		s2 = SimpleDependencyNode()

python/GafferTest/TimeWarpComputeNodeTest.py

@@ -100,6 +100,20 @@ def testHash( self ) :
 			with c :
 				mh = s["m"]["product"].hash()
 			self.assertEqual( wh, mh )
+
+	def testAffects( self ) :
+
+		w = Gaffer.TimeWarpComputeNode()
+		w["in"] = Gaffer.IntPlug()
+		w["out"] = Gaffer.IntPlug( direction = Gaffer.Plug.Direction.Out )
+
+		cs = GafferTest.CapturingSlot( w.plugDirtiedSignal() )
+
+		w["in"].setValue( 10 )
+
+		self.assertEqual( len( cs ), 2 )
+		self.assertTrue( cs[0][0].isSame( w["in"] ) )
+		self.assertTrue( cs[1][0].isSame( w["out"] ) )
 
 if __name__ == "__main__":
 	unittest.main()

src/Gaffer/DependencyNode.cpp

@@ -145,26 +145,23 @@ void DependencyNode::propagateDirtiness( Plug *plugToDirty )
 	// and compute() methods.
 	if( !plugToDirty->isInstanceOf( (IECore::TypeId)CompoundPlugTypeId ) )
 	{
-		if( plugToDirty->direction()==Plug::In )
+		DependencyNode *dependencyNode = IECore::runTimeCast<DependencyNode>( node );
+		if( dependencyNode )
 		{
-			DependencyNode *dependencyNode = IECore::runTimeCast<DependencyNode>( node );
-			if( dependencyNode )
-			{				
-				AffectedPlugsContainer affected;
-				dependencyNode->affects( plugToDirty, affected );
-				for( AffectedPlugsContainer::const_iterator it=affected.begin(); it!=affected.end(); it++ )
+			AffectedPlugsContainer affected;
+			dependencyNode->affects( plugToDirty, affected );
+			for( AffectedPlugsContainer::const_iterator it=affected.begin(); it!=affected.end(); it++ )
+			{
+				if( ( *it )->isInstanceOf( (IECore::TypeId)Gaffer::CompoundPlugTypeId ) )
 				{
-					if( ( *it )->isInstanceOf( (IECore::TypeId)Gaffer::CompoundPlugTypeId ) )
-					{
-						// DependencyNode::affects() implementations are only allowed to place leaf plugs in the outputs,
-						// so we helpfully report any mistakes.						
-						dirtyPlugs.clear();
-						throw IECore::Exception( "Non-leaf plug " + (*it)->fullName() + " cannot be returned by affects()" );
-					}
-					// cast is ok - AffectedPlugsContainer only holds const pointers so that
-					// affects() can be const to discourage implementations from having side effects.
-					propagateDirtiness( const_cast<Plug *>( *it ) );
+					// DependencyNode::affects() implementations are only allowed to place leaf plugs in the outputs,
+					// so we helpfully report any mistakes.
+					dirtyPlugs.clear();
+					throw IECore::Exception( "Non-leaf plug " + (*it)->fullName() + " cannot be returned by affects()" );
 				}
+				// cast is ok - AffectedPlugsContainer only holds const pointers so that
+				// affects() can be const to discourage implementations from having side effects.
+				propagateDirtiness( const_cast<Plug *>( *it ) );
 			}
 		}
 

src/GafferOSL/OSLImage.cpp

@@ -190,17 +190,11 @@ void OSLImage::affects( const Gaffer::Plug *input, AffectedPlugsContainer &outpu
 
 	if( input == shaderPlug() )
 	{
-		/// \todo We should really be pushing shadingPlug()
-		/// here, and then pushing channelDataPlug() in the
-		/// affects for shadingPlug(). However, currently
-		/// affects() isn't called for shadingPlug() because
-		/// it's an output. We could do the output->input trick
-		/// that we're using in GafferScene::Group, but it
-		/// seems worth considering allowing affects() to be
-		/// called for outputs, to avoid forcing lots of 
-		/// implementations to use extra plugs when they're not
-		/// really needed.
-		outputs.push_back( outPlug()->channelDataPlug() );
+		outputs.push_back( shadingPlug() );
+	}
+	else if( input == shadingPlug() )
+	{
+		outputs.push_back( outPlug()->channelDataPlug()	);
 	}
 }
 

src/GafferScene/Group.cpp

@@ -69,8 +69,6 @@ Group::Group( const std::string &name )
 	addChild( new TransformPlug( "transform" ) );
 
 	addChild( new Gaffer::ObjectPlug( "__mapping", Gaffer::Plug::Out, new CompoundObject() ) );
-	addChild( new Gaffer::ObjectPlug( "__inputMapping", Gaffer::Plug::In, new CompoundObject(), Gaffer::Plug::Default & ~Gaffer::Plug::Serialisable ) );
-	inputMappingPlug()->setInput( mappingPlug() );
 }
 
 Group::~Group()
@@ -97,6 +95,16 @@ const Gaffer::TransformPlug *Group::transformPlug() const
 	return getChild<TransformPlug>( g_firstPlugIndex + 1 );
 }
 
+Gaffer::ObjectPlug *Group::mappingPlug()
+{
+	return getChild<Gaffer::ObjectPlug>( g_firstPlugIndex + 2 );
+}
+
+const Gaffer::ObjectPlug *Group::mappingPlug() const
+{
+	return getChild<Gaffer::ObjectPlug>( g_firstPlugIndex + 2 );
+}
+
 void Group::affects( const Plug *input, AffectedPlugsContainer &outputs ) const
 {
 	SceneProcessor::affects( input, outputs );
@@ -113,21 +121,25 @@ void Group::affects( const Plug *input, AffectedPlugsContainer &outputs ) const
 		outputs.push_back( outPlug()->transformPlug() );
 		outputs.push_back( outPlug()->boundPlug() );
 	}
-	else if( input == inputMappingPlug() )
+	else if( const ScenePlug *s = input->parent<ScenePlug>() )
 	{
-		for( ValuePlugIterator it( outPlug() ); it != it.end(); it++ )
+		if( s->direction() == Plug::In )
 		{
-			outputs.push_back( it->get() );
+			// all input scene plugs children affect the corresponding output child
+			outputs.push_back( outPlug()->getChild<ValuePlug>( input->getName() ) );		
+			// and the names also affect the mapping
+			if( input == s->childNamesPlug() )
+			{
+				outputs.push_back( mappingPlug() );
+			}
 		}
 	}
-	else if( const ScenePlug *s = input->parent<ScenePlug>() )
+	else if( input == mappingPlug() )
 	{
-		// all input scene plugs affect the output
-		outputs.push_back( outPlug()->getChild<ValuePlug>( input->getName() ) )
-		;		
-		if( input == s->childNamesPlug() )
+		// the mapping affects everything about the output
+		for( ValuePlugIterator it( outPlug() ); it != it.end(); it++ )
 		{
-			outputs.push_back( mappingPlug() );
+			outputs.push_back( it->get() );
 		}
 	}
 
@@ -240,7 +252,7 @@ void Group::hashChildNames( const ScenePath &path, const Gaffer::Context *contex
 	else if( path.size() == 1 ) // "/group"
 	{
 		SceneProcessor::hashChildNames( path, context, parent, h );
-		inputMappingPlug()->hash( h );
+		mappingPlug()->hash( h );
 	}
 	else // "/group/..."
 	{
@@ -260,7 +272,7 @@ void Group::hashGlobals( const Gaffer::Context *context, const ScenePlug *parent
 	{
 		(*it)->globalsPlug()->hash( h );
 	}
-	inputMappingPlug()->hash( h );
+	mappingPlug()->hash( h );
 	namePlug()->hash( h );
 }
 
@@ -432,7 +444,7 @@ IECore::ConstInternedStringVectorDataPtr Group::computeChildNames( const ScenePa
 	}
 	else if( path.size() == 1 )
 	{
-		ConstCompoundObjectPtr mapping = staticPointerCast<const CompoundObject>( inputMappingPlug()->getValue() );
+		ConstCompoundObjectPtr mapping = staticPointerCast<const CompoundObject>( mappingPlug()->getValue() );
 		return mapping->member<InternedStringVectorData>( "__GroupChildNames" );
 	}
 	else
@@ -449,7 +461,7 @@ IECore::ConstCompoundObjectPtr Group::computeGlobals( const Gaffer::Context *con
 
 	std::string groupName = namePlug()->getValue();
 
-	ConstCompoundObjectPtr mapping = staticPointerCast<const CompoundObject>( inputMappingPlug()->getValue() );
+	ConstCompoundObjectPtr mapping = staticPointerCast<const CompoundObject>( mappingPlug()->getValue() );
 	const ObjectVector *forwardMappings = mapping->member<ObjectVector>( "__GroupForwardMappings", true /* throw if missing */ );
 
 	IECore::CompoundDataPtr forwardDeclarations = new IECore::CompoundData;
@@ -487,7 +499,7 @@ SceneNode::ScenePath Group::sourcePath( const ScenePath &outputPath, const std::
 {		
 	const InternedString mappedChildName = outputPath[1];
 
-	ConstCompoundObjectPtr mapping = staticPointerCast<const CompoundObject>( inputMappingPlug()->getValue() );
+	ConstCompoundObjectPtr mapping = staticPointerCast<const CompoundObject>( mappingPlug()->getValue() );
 	const CompoundObject *entry = mapping->member<CompoundObject>( mappedChildName );
 	if( !entry )
 	{
@@ -502,23 +514,3 @@ SceneNode::ScenePath Group::sourcePath( const ScenePath &outputPath, const std::
 	result.insert( result.end(), outputPath.begin() + 2, outputPath.end() );
 	return result;
 }
-
-Gaffer::ObjectPlug *Group::mappingPlug()
-{
-	return getChild<Gaffer::ObjectPlug>( g_firstPlugIndex + 2 );
-}
-
-const Gaffer::ObjectPlug *Group::mappingPlug() const
-{
-	return getChild<Gaffer::ObjectPlug>( g_firstPlugIndex + 2 );
-}
-
-Gaffer::ObjectPlug *Group::inputMappingPlug()
-{
-	return getChild<Gaffer::ObjectPlug>( g_firstPlugIndex + 3 );
-}
-
-const Gaffer::ObjectPlug *Group::inputMappingPlug() const
-{
-	return getChild<Gaffer::ObjectPlug>( g_firstPlugIndex + 3 );
-}