Context bug fixes

include/GafferSceneTest/TraverseScene.h

@@ -37,6 +37,8 @@
 #ifndef GAFFERSCENETEST_TRAVERSESCENE_H
 #define GAFFERSCENETEST_TRAVERSESCENE_H
 
+#include "Gaffer/Context.h"
+
 #include "GafferScene/ScenePlug.h"
 
 namespace GafferSceneTest
@@ -54,6 +56,11 @@ void traverseScene( GafferScene::ScenePlug *scenePlug );
 /// traversals will be triggered automatically by plugDirtiedSignal().
 boost::signals::connection connectTraverseSceneToPlugDirtiedSignal( const GafferScene::ConstScenePlugPtr &scene );
 
+/// Arranges for traverseScene() to be called every time the context is changed. This is useful
+/// for exposing bugs caused by things like InteractiveRender and SceneView, where threaded
+/// traversals will be triggered automatically from Context::changedSignal().
+boost::signals::connection connectTraverseSceneToContextChangedSignal( const GafferScene::ConstScenePlugPtr &scene, const Gaffer::ContextPtr &context );
+
 } // namespace GafferSceneTest
 
 #endif // GAFFERSCENETEST_TRAVERSESCENE_H

python/GafferSceneTest/InstancerTest.py

@@ -457,5 +457,63 @@ def testLoadReferenceAndGIL( self ) :
 
 			script["reference"].load( self.temporaryDirectory() + "/test.grf" )
 
+	def testContextChangedAndGIL( self ) :
+
+		script = Gaffer.ScriptNode()
+
+		script["plane"] = GafferScene.Plane()
+		script["plane"]["divisions"].setValue( IECore.V2i( 20 ) )
+
+		script["sphere"] = GafferScene.Sphere()
+
+		script["expression"] = Gaffer.Expression()
+		script["expression"].setExpression( "parent['sphere']['radius'] = context.get( 'minRadius', 0.1 ) + context.getFrame() + float( context['instancer:id'] )" )
+
+		script["instancer"] = GafferScene.Instancer()
+		script["instancer"]["in"].setInput( script["plane"]["out"] )
+		script["instancer"]["instance"].setInput( script["sphere"]["out"] )
+		script["instancer"]["parent"].setValue( "/plane" )
+
+		context = Gaffer.Context()
+		traverseConnection = Gaffer.ScopedConnection( GafferSceneTest.connectTraverseSceneToContextChangedSignal( script["instancer"]["out"], context ) )
+		with context :
+
+			context.setFrame( 10 )
+			context.setFramesPerSecond( 50 )
+			context.setTime( 1 )
+
+			context.set( "a", 1 )
+			context.set( "a", 2.0 )
+			context.set( "a", "a" )
+			context.set( "a", IECore.V2i() )
+			context.set( "a", IECore.V3i() )
+			context.set( "a", IECore.V2f() )
+			context.set( "a", IECore.V3f() )
+			context.set( "a", IECore.Color3f() )
+			context.set( "a", IECore.BoolData( True ) )
+
+			context["b"] = 1
+			context["b"] = 2.0
+			context["b"] = "b"
+			context["b"] = IECore.V2i()
+			context["b"] = IECore.V3i()
+			context["b"] = IECore.V2f()
+			context["b"] = IECore.V3f()
+			context["b"] = IECore.Color3f()
+			context["b"] = IECore.BoolData( True )
+
+			with Gaffer.BlockedConnection( traverseConnection ) :
+				# Must add it with the connection disabled, otherwise
+				# the addition causes a traversal, and then remove() gets
+				# all its results from the cache.
+				context["minRadius"] = 0.2
+
+			context.remove( "minRadius" )
+
+			with Gaffer.BlockedConnection( traverseConnection ) :
+				context["minRadius"] = 0.3
+
+			del context["minRadius"]
+
 if __name__ == "__main__":
 	unittest.main()

python/GafferTest/ContextTest.py

@@ -64,7 +64,7 @@ def testChangedSignal( self ) :
 		def f( context, name ) :
 
 			self.failUnless( context.isSame( c ) )
-			changes.append( ( name, context[name] ) )
+			changes.append( ( name, context.get( name, None ) ) )
 
 		cn = c.changedSignal().connect( f )
 
@@ -82,6 +82,16 @@ def f( context, name ) :
 		c["b"] = 1
 		self.assertEqual( changes, [ ( "a", 2 ), ( "a", 3 ), ( "b", 1 ) ] )
 
+		# Removing variables should also trigger the changed signal.
+
+		del changes[:]
+
+		c.remove( "a" )
+		self.assertEqual( changes, [ ( "a", None ) ] )
+
+		del c["b"]
+		self.assertEqual( changes, [ ( "a", None ), ( "b", None ) ] )
+
 	def testTypes( self ) :
 
 		c = Gaffer.Context()

src/Gaffer/Context.cpp

@@ -174,6 +174,10 @@ void Context::remove( const IECore::InternedString &name )
 	{
 		m_map.erase( it );
 		m_hashValid = false;
+		if( m_changedSignal )
+		{
+			(*m_changedSignal)( this, name );
+		}
 	}
 }
 

src/GafferBindings/ContextBinding.cpp

@@ -52,6 +52,31 @@ using namespace IECore;
 namespace
 {
 
+void setFrame( Context &c, float frame )
+{
+	IECorePython::ScopedGILRelease gilRelease;
+	c.setFrame( frame );
+}
+
+void setFramesPerSecond( Context &c, float framesPerSecond )
+{
+	IECorePython::ScopedGILRelease gilRelease;
+	c.setFramesPerSecond( framesPerSecond );
+}
+
+void setTime( Context &c, float time )
+{
+	IECorePython::ScopedGILRelease gilRelease;
+	c.setTime( time );
+}
+
+template<typename T>
+void set( Context &c, const IECore::InternedString &name, const T &value )
+{
+	IECorePython::ScopedGILRelease gilRelease;
+	c.set( name, value );
+}
+
 // In the C++ API, get() returns "const Data *". Because python has no idea of constness,
 // by default we return a copy from the bindings because we don't want the unwitting Python
 // scripter to accidentally modify the internals of a Context. We do however expose the
@@ -83,6 +108,7 @@ bool contains( Context &c, const IECore::InternedString &name )
 
 void delItem( Context &context, const IECore::InternedString &name )
 {
+	IECorePython::ScopedGILRelease gilRelease;
 	context.remove( name );
 }
 
@@ -145,35 +171,35 @@ void GafferBindings::bindContext()
 	contextClass
 		.def( init<>() )
 		.def( init<const Context &, Context::Ownership>( ( arg( "other" ), arg( "ownership" ) = Context::Copied ) ) )
-		.def( "setFrame", &Context::setFrame )
+		.def( "setFrame", &setFrame )
 		.def( "getFrame", &Context::getFrame )
-		.def( "setFramesPerSecond", &Context::setFramesPerSecond )
+		.def( "setFramesPerSecond", &setFramesPerSecond )
 		.def( "getFramesPerSecond", &Context::getFramesPerSecond )
-		.def( "setTime", &Context::setTime )
+		.def( "setTime", &setTime )
 		.def( "getTime", &Context::getTime )
-		.def( "set", &Context::set<float> )
-		.def( "set", &Context::set<int> )
-		.def( "set", &Context::set<std::string> )
-		.def( "set", &Context::set<Imath::V2i> )
-		.def( "set", &Context::set<Imath::V3i> )
-		.def( "set", &Context::set<Imath::V2f> )
-		.def( "set", &Context::set<Imath::V3f> )
-		.def( "set", &Context::set<Imath::Color3f> )
-		.def( "set", &Context::set<Data *> )
-		.def( "__setitem__", &Context::set<float> )
-		.def( "__setitem__", &Context::set<int> )
-		.def( "__setitem__", &Context::set<std::string> )
-		.def( "__setitem__", &Context::set<Imath::V2i> )
-		.def( "__setitem__", &Context::set<Imath::V3i> )
-		.def( "__setitem__", &Context::set<Imath::V2f> )
-		.def( "__setitem__", &Context::set<Imath::V3f> )
-		.def( "__setitem__", &Context::set<Imath::Color3f> )
-		.def( "__setitem__", &Context::set<Data *> )
+		.def( "set", &set<float> )
+		.def( "set", &set<int> )
+		.def( "set", &set<std::string> )
+		.def( "set", &set<Imath::V2i> )
+		.def( "set", &set<Imath::V3i> )
+		.def( "set", &set<Imath::V2f> )
+		.def( "set", &set<Imath::V3f> )
+		.def( "set", &set<Imath::Color3f> )
+		.def( "set", &set<Data *> )
+		.def( "__setitem__", &set<float> )
+		.def( "__setitem__", &set<int> )
+		.def( "__setitem__", &set<std::string> )
+		.def( "__setitem__", &set<Imath::V2i> )
+		.def( "__setitem__", &set<Imath::V3i> )
+		.def( "__setitem__", &set<Imath::V2f> )
+		.def( "__setitem__", &set<Imath::V3f> )
+		.def( "__setitem__", &set<Imath::Color3f> )
+		.def( "__setitem__", &set<Data *> )
 		.def( "get", &get, arg( "_copy" ) = true )
 		.def( "get", &getWithDefault, ( arg( "defaultValue" ), arg( "_copy" ) = true ) )
 		.def( "__getitem__", &getItem )
 		.def( "__contains__", &contains )
-		.def( "remove", &Context::remove )
+		.def( "remove", &delItem )
 		.def( "__delitem__", &delItem )
 		.def( "changed", &Context::changed )
 		.def( "names", &names )

src/GafferSceneTest/TraverseScene.cpp

@@ -68,6 +68,12 @@ void traverseOnDirty( const Gaffer::Plug *dirtiedPlug, ConstScenePlugPtr scene )
 	}
 }
 
+void traverseOnChanged( ConstScenePlugPtr scene, ConstContextPtr context )
+{
+	Context::Scope scopedContext( context.get() );
+	traverseScene( scene.get() );
+}
+
 } // namespace
 
 void GafferSceneTest::traverseScene( const GafferScene::ScenePlug *scenePlug )
@@ -91,3 +97,8 @@ boost::signals::connection GafferSceneTest::connectTraverseSceneToPlugDirtiedSig
 
 	return const_cast<Node *>( node )->plugDirtiedSignal().connect( boost::bind( &traverseOnDirty, ::_1, scene ) );
 }
+
+boost::signals::connection GafferSceneTest::connectTraverseSceneToContextChangedSignal( const GafferScene::ConstScenePlugPtr &scene, const Gaffer::ContextPtr &context )
+{
+	return context->changedSignal().connect( boost::bind( &traverseOnChanged, scene, context ) );
+}

src/GafferSceneTestModule/GafferSceneTestModule.cpp

@@ -66,6 +66,7 @@ BOOST_PYTHON_MODULE( _GafferSceneTest )
 
 	def( "traverseScene", &traverseSceneWrapper );
 	def( "connectTraverseSceneToPlugDirtiedSignal", &connectTraverseSceneToPlugDirtiedSignal );
+	def( "connectTraverseSceneToContextChangedSignal", &connectTraverseSceneToContextChangedSignal );
 
 	def( "testManyStringToPathCalls", &testManyStringToPathCalls );