Merge pull request #1571 from johnhaddon/internalConnectionIntegrity

Node : Fix disconnect-during-unparent bug.

python/GafferImageTest/ImageStatsTest.py

@@ -159,6 +159,15 @@ def testROI( self ) :
 		self.__assertColour( s["min"].getValue(), IECore.Color4f( 0.25, 0, 0, 0.5 ) )
 		self.__assertColour( s["max"].getValue(), IECore.Color4f( 0.5, 0.5, 0, 0.75 ) )
 
+	def testMin( self ) :
+
+		c = GafferImage.Constant()
+		s = GafferImage.ImageStats()
+		s["in"].setInput( c["out"] )
+		s["regionOfInterest"].setValue( c["out"]["format"].getValue().getDisplayWindow() )
+
+		self.assertEqual( s["max"]["r"].getValue(), 0 )
+
 	def __assertColour( self, colour1, colour2 ) :
 		for i in range( 0, 4 ):
 			self.assertEqual( "%.4f" % colour2[i], "%.4f" % colour1[i] )

python/GafferImageTest/ImageTestCase.py

@@ -42,7 +42,32 @@
 
 class ImageTestCase( GafferTest.TestCase ) :
 
-	def assertImagesEqual( self, imageA, imageB, maxDifference = 0, ignoreMetadata = False, ignoreDataWindow = False ) :
+	def assertImageHashesEqual( self, imageA, imageB ) :
+
+		self.assertEqual( imageA["format"].hash(), imageB["format"].hash() )
+		self.assertEqual( imageA["dataWindow"].hash(), imageB["dataWindow"].hash() )
+		self.assertEqual( imageA["metadata"].hash(), imageB["metadata"].hash() )
+		self.assertEqual( imageA["channelNames"].hash(), imageB["channelNames"].hash() )
+
+		dataWindow = imageA["dataWindow"].getValue()
+		self.assertEqual( dataWindow, imageB["dataWindow"].getValue() )
+
+		channelNames = imageA["channelNames"].getValue()
+		self.assertEqual( channelNames, imageB["channelNames"].getValue() )
+
+		tileOrigin = GafferImage.ImagePlug.tileOrigin( dataWindow.min )
+		while tileOrigin.y < dataWindow.max.y :
+			tileOrigin.x = GafferImage.ImagePlug.tileOrigin( dataWindow.min ).x
+			while tileOrigin.x < dataWindow.max.x :
+				for channelName in channelNames :
+					self.assertEqual(
+						imageA.channelDataHash( channelName, tileOrigin ),
+						imageB.channelDataHash( channelName, tileOrigin )
+					)
+				tileOrigin.x += GafferImage.ImagePlug.tileSize()
+			tileOrigin.y += GafferImage.ImagePlug.tileSize()
+
+	def assertImagesEqual( self, imageA, imageB, maxDifference = 0.0, ignoreMetadata = False, ignoreDataWindow = False ) :
 
 		self.assertEqual( imageA["format"].getValue(), imageB["format"].getValue() )
 		if not ignoreDataWindow :
@@ -66,14 +91,13 @@ def assertImagesEqual( self, imageA, imageB, maxDifference = 0, ignoreMetadata =
 		stats["channels"].setValue( IECore.StringVectorData( [ "R", "G", "B", "A" ] ) )
 
 		if "R" in imageA["channelNames"].getValue() :
-			self.assertLess( stats["max"]["r"].getValue(), maxDifference )
+			self.assertLessEqual( stats["max"]["r"].getValue(), maxDifference )
 
 		if "G" in imageA["channelNames"].getValue() :
-			self.assertLess( stats["max"]["g"].getValue(), maxDifference )
+			self.assertLessEqual( stats["max"]["g"].getValue(), maxDifference )
 
 		if "B" in imageA["channelNames"].getValue() :
-			self.assertLess( stats["max"]["b"].getValue(), maxDifference )
+			self.assertLessEqual( stats["max"]["b"].getValue(), maxDifference )
 
 		if "A" in imageA["channelNames"].getValue() :
-			self.assertLess( stats["max"]["a"].getValue(), maxDifference )
-
+			self.assertLessEqual( stats["max"]["a"].getValue(), maxDifference )

python/GafferImageTest/TextTest.py

@@ -151,5 +151,17 @@ def testVerticalAlignment( self ) :
 		self.assertEqual( bottomDW.size().y, centerDW.size().y )
 		self.assertEqual( centerDW.size().y, topDW.size().y )	
 
+	def testUnparenting( self ) :
+
+		t1 = GafferImage.Text()
+		t2 = GafferImage.Text()
+
+		s = Gaffer.ScriptNode()
+		s.addChild( t2 )
+		s.removeChild( t2 )
+
+		self.assertImageHashesEqual( t1["out"], t2["out"] )
+		self.assertImagesEqual( t1["out"], t2["out"] )
+
 if __name__ == "__main__":
 	unittest.main()

python/GafferTest/NodeTest.py

@@ -36,8 +36,6 @@
 ##########################################################################
 
 import unittest
-import threading
-import time
 
 import IECore
 
@@ -182,6 +180,33 @@ def testUnparentingRemovesConnections( self ) :
 
 		self.assertEqual( n2["op1"].getInput(), None )
 
+	def testUnparentingRemovesUserConnections( self ) :
+
+		s = Gaffer.ScriptNode()
+
+		n1 = GafferTest.AddNode()
+		n2 = GafferTest.AddNode()
+
+		s["n1"] = n1
+		s["n2"] = n2
+
+		s["n1"]["user"]["i1"] = Gaffer.IntPlug( flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic )
+		s["n1"]["user"]["i2"] = Gaffer.IntPlug( flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic )
+		s["n1"]["user"]["v"] = Gaffer.V2iPlug( flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic )
+
+		s["n1"]["user"]["i1"].setInput( n2["sum"] )
+		s["n2"]["op1"].setInput( s["n1"]["user"]["i2"] )
+		s["n1"]["user"]["v"][0].setInput( s["n2"]["sum"] )
+		s["n2"]["op2"].setInput( s["n1"]["user"]["v"][1] )
+
+		del s["n1"]
+		self.assertTrue( n1.parent() is None )
+
+		self.assertTrue( n1["user"]["i1"].getInput() is None )
+		self.assertTrue( n2["op1"].getInput() is None )
+		self.assertTrue( n1["user"]["v"][0].getInput() is None )
+		self.assertTrue( n2["op2"].getInput() is None )
+
 	def testOverrideAcceptsInput( self ) :
 
 		class AcceptsInputTestNode( Gaffer.Node ) :
@@ -277,5 +302,52 @@ def testUserPlugDoesntTrackChildConnections( self ) :
 		s["n1"]["user"]["p2"] = Gaffer.IntPlug()
 		self.assertEqual( len( s["n2"]["user"] ), 1 )
 
-if __name__ == "__main__":
+	def testInternalConnectionsSurviveUnparenting( self ) :
+
+		class InternalConnectionsNode( Gaffer.Node ) :
+
+			def __init__( self, name = "InternalConnectionsNode" ) :
+
+				Gaffer.Node.__init__( self, name )
+
+				self["in1"] = Gaffer.IntPlug()
+				self["in2"] = Gaffer.IntPlug()
+				self["__in"] = Gaffer.IntPlug()
+
+				self["out1"] = Gaffer.IntPlug( direction = Gaffer.Plug.Direction.Out )
+				self["out2"] = Gaffer.IntPlug( direction = Gaffer.Plug.Direction.Out )
+				self["__out"] = Gaffer.IntPlug( direction = Gaffer.Plug.Direction.Out )
+
+				self["out1"].setInput( self["in1"] )
+
+				self["__add"] = GafferTest.AddNode()
+				self["__add"]["op1"].setInput( self["in2"] )
+				self["__add"]["op2"].setInput( self["__out"] )
+				self["__in"].setInput( self["__add"]["sum"] )
+
+				self["out2"].setInput( self["__add"]["sum"] )
+
+		IECore.registerRunTimeTyped( InternalConnectionsNode )
+
+		s = Gaffer.ScriptNode()
+		n = InternalConnectionsNode()
+		s["n"] = n
+
+		def assertConnections() :
+
+			self.assertTrue( n["out1"].getInput().isSame( n["in1"] ) )
+			self.assertTrue( n["__add"]["op1"].getInput().isSame( n["in2"] ) )
+			self.assertTrue( n["__add"]["op2"].getInput().isSame( n["__out"] ) )
+			self.assertTrue( n["out2"].getInput().isSame( n["__add"]["sum"] ) )
+			self.assertTrue( n["__in"].getInput().isSame( n["__add"]["sum"] ) )
+
+		assertConnections()
+
+		s.removeChild( n )
+		assertConnections()
+
+		s.addChild( n )
+		assertConnections()
+
+if __name__ == "__main__" :
 	unittest.main()

src/Gaffer/Node.cpp

@@ -39,6 +39,7 @@
 #include "Gaffer/ScriptNode.h"
 #include "Gaffer/Metadata.h"
 
+using namespace std;
 using namespace Gaffer;
 
 size_t Node::g_firstPlugIndex;
@@ -134,29 +135,39 @@ bool Node::acceptsInput( const Plug *plug, const Plug *inputPlug ) const
 
 void Node::parentChanging( Gaffer::GraphComponent *newParent )
 {
-	// if we're losing our parent then remove all connections first.
-	// this must be done here rather than from parentChangedSignal()
+	// If we're losing our parent then remove all external connections
+	// first. This must be done here rather than from parentChangedSignal()
 	// because we need a current parent for the operation to be
 	// undoable.
 
 	if( !newParent )
 	{
-		// because the InputGenerator code removes plugs when inputs
-		// are removed, we have to take a copy of the current children
-		// and iterate over that - otherwise our iterators are invalidated
-		// and we get crashes. if we see further problems caused by the
-		// InputGenerator creating plugs whenever it sees fit, we should
-		// consider an API where they are instead asked for explicitly
-		// when needed.
-		ChildContainer frozenChildren = children();
-		for( InputPlugIterator it( frozenChildren ); it!=it.end(); it++ )
+		// Because disconnecting a plug might cause graph changes
+		// via Node::plugInputChangedSignal(), we use a two phase
+		// process to avoid such changes invalidating our
+		// iterators.
+		vector<PlugPtr> toDisconnect;
+		for( RecursivePlugIterator it( this ); it!=it.end(); ++it )
 		{
-			(*it)->setInput( 0 );
+			if( Plug *input = (*it)->getInput<Plug>() )
+			{
+				if( !this->isAncestorOf( input ) )
+				{
+					toDisconnect.push_back( *it );
+				}
+			}
+			for( Plug::OutputContainer::const_iterator oIt = (*it)->outputs().begin(), oeIt = (*it)->outputs().end(); oIt != oeIt; ++oIt )
+			{
+				if( !this->isAncestorOf( *oIt ) )
+				{
+					toDisconnect.push_back( *oIt );
+				}
+			}
 		}
 
-		for( OutputPlugIterator it( frozenChildren ); it!=it.end(); it++ )
+		for( vector<PlugPtr>::const_iterator it = toDisconnect.begin(), eIt = toDisconnect.end(); it != eIt; ++it )
 		{
-			(*it)->removeOutputs();
+			(*it)->setInput( NULL );
 		}
 	}
 }

src/GafferImage/ImageStats.cpp

@@ -36,7 +36,6 @@
 
 #include "Gaffer/TypedPlug.h"
 #include "Gaffer/BoxPlug.h"
-#include "Gaffer/Context.h"
 #include "Gaffer/ScriptNode.h"
 
 #include "GafferImage/ImageStats.h"
@@ -301,16 +300,12 @@ void ImageStats::compute( ValuePlug *output, const Context *context ) const
 
 	const int channelIndex = colorIndex( channelName );
 
-	// Set up the execution context.
-	ContextPtr tmpContext = new Context( *context, Context::Borrowed );
-	tmpContext->set( ImagePlug::channelNameContextName, channelName );
-	Context::Scope scopedContext( tmpContext.get() );
-
 	// Loop over the ROI and compute the min, max and average channel values and then set our outputs.
 	Sampler s( inPlug(), channelName, regionOfInterest );
 
-	float min = std::numeric_limits<float>::max();
-	float max = std::numeric_limits<float>::min();
+	float min = Imath::limits<float>::max();
+	float max = Imath::limits<float>::min();
+
 	float average = 0.f;
 
 	double sum = 0.;