Merge pull request #5378 from johnhaddon/instancerContextSanitisation

Instancer : Fix rare `scene:path` context "leak"

Changes.md

@@ -1,6 +1,10 @@
 1.2.x.x (relative to 1.2.9.0)
 =======
 
+Fixes
+-----
+
+- Instancer : Fixed bug that could cause the `scene:path` context variable to be leaked into the evaluation of `propotypes.set` in rare circumstances.
 
 1.2.9.0 (relative to 1.2.8.0)
 =======

python/GafferSceneTest/InstancerTest.py

@@ -2225,6 +2225,36 @@ def testRootPerVertexWithEmptyPoints( self ) :
 		self.assertEqual( instancer["out"].childNames( "/points/instances" ), IECore.InternedStringVectorData() )
 		self.assertSceneValid( instancer["out"] )
 
+	def testNoScenePathInPrototypeSetContext( self ) :
+
+		plane = GafferScene.Plane()
+		planeFilter = GafferScene.PathFilter()
+		planeFilter["paths"].setValue( IECore.StringVectorData( [ "/plane" ] ) )
+
+		cube = GafferScene.Cube()
+		cube["sets"].setValue( "setA" )
+
+		instancer = GafferScene.Instancer()
+		instancer["in"].setInput( plane["out"] )
+		instancer["prototypes"].setInput( cube["out"] )
+		instancer["filter"].setInput( planeFilter["out"] )
+
+		instancer["user"]["contextVariableEnabler"] = Gaffer.BoolPlug( defaultValue = False, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic )
+
+		instancer["contextVariables"].addChild( GafferScene.Instancer.ContextVariablePlug() )
+		instancer["contextVariables"][0]["name"].setValue( "test" )
+		# Although the input has a value of `False`, this is sufficient to trick
+		# `Instancer::hashBranchSet()` into thinking there will be context
+		# variables, which causes it to call `setCollaboratePlug()->hash()`,
+		# which then accurately concludes that actually there are no context
+		# variables. It then hits a very rare code path which wasn't well tested
+		# until now.
+		instancer["contextVariables"][0]["enabled"].setInput( instancer["user"]["contextVariableEnabler"] )
+
+		# We're not asserting anything here, but the ContextSanitiser installed by `SceneTestCase.setUp()`
+		# is checking that we're not leaking `scene:path` upstream.
+		instancer["out"].set( "setA" )
+
 	def runTestContextSetPerf( self, useContexts, parallelEvaluate ):
 
 		plane = GafferScene.Plane()

src/GafferScene/Instancer.cpp

@@ -1344,7 +1344,7 @@ void Instancer::hash( const Gaffer::ValuePlug *output, const Gaffer::Context *co
 			// requested were actually invalid, then we can use the same fast approximate hash used in
 			// hashBranchSet.
 			//
-			// This is a little bit weird, because in this scenario, this plug will never be evaluated:
+			// This is a little bit weird, because in this scenario, this plug will never be computed :
 			// computeBranchSet checks the accurate hasContextVariables before evaluating setCollaboratePlug.
 			// But hashBranchSet is evaluating us, so we have to give a hash that will work for it.
 			//
@@ -1353,6 +1353,8 @@ void Instancer::hash( const Gaffer::ValuePlug *output, const Gaffer::Context *co
 			// if point locations change, unlike the engineHash which includes all changes
 			engineHash( sourcePath, context, h );
 			prototypeChildNamesHash( sourcePath, context, h );
+			Context::EditableScope scope( context );
+			scope.remove( ScenePlug::scenePathContextName );
 			prototypesPlug()->setPlug()->hash( h );
 			namePlug()->hash( h );
 			return;