Filter : Sanitise context used to evaluate Filter.enabled

Without this, any input connections were evaluated once per location rather than just once, leading to much unnecessary pressure on the hash cache. It also provided a loophole via which you could modify the filter per location in a way that meant that ancestor and descendant matches would be inaccurate.

Changes.md

@@ -1,6 +1,10 @@
 1.3.x.x (relative to 1.3.5.0)
 =======
 
+Fixes
+-----
+
+- Filter : Fixed bug which allowed the `scene:path` context variable to "leak" upstream via the `Filter.enabled` plug. This caused unnecessary evaluations of the input, and also provided a loophole via which the filter result could be ,made inconsistent with respect to descendant and ancestor matches.
 
 1.3.5.0 (relative to 1.3.4.0)
 =======

include/GafferScene/Filter.h

@@ -113,6 +113,8 @@ class GAFFERSCENE_API Filter : public Gaffer::ComputeNode
 
 	private :
 
+		bool enabled( const Gaffer::Context *context ) const;
+
 		friend class FilterPlug;
 
 		static size_t g_firstPlugIndex;

python/GafferSceneTest/FilterTest.py

@@ -52,5 +52,28 @@ def testInputScene( self ) :
 		GafferScene.Filter.setInputScene( c, p["out"] )
 		self.assertEqual( GafferScene.Filter.getInputScene( c ), p["out"] )
 
+	def testEnabledPlugContextSanitisation( self ) :
+
+		# Make a graph where `pathFilter.enabled` reads from the
+		# scene globals.
+		plane = GafferScene.Plane()
+
+		optionQuery = GafferScene.OptionQuery()
+		optionQuery["scene"].setInput( plane["out"] )
+		query = optionQuery.addQuery( Gaffer.BoolPlug(), "test" )
+
+		pathFilter = GafferScene.PathFilter()
+		pathFilter["enabled"] .setInput( optionQuery.outPlugFromQuery( query )["value"] )
+
+		attributes = GafferScene.StandardAttributes()
+		attributes["in"].setInput( plane["out"] )
+		attributes["filter"].setInput( pathFilter["out"] )
+
+		# Trigger an evaluation of the filter. We don't need to assert anything
+		# here, because all tests run with a ContextSanitiser active, and that
+		# will cause a failure if the filter leaks a context variable like
+		# `scene:path` into the evaluation of the scene globals.
+		attributes["out"].attributes( "/plane" )
+
 if __name__ == "__main__":
 	unittest.main()

src/GafferScene/Filter.cpp

@@ -37,6 +37,7 @@
 #include "GafferScene/Filter.h"
 
 #include "GafferScene/FilterPlug.h"
+#include "GafferScene/ScenePlug.h"
 
 #include "Gaffer/Context.h"
 
@@ -117,7 +118,7 @@ void Filter::hash( const Gaffer::ValuePlug *output, const Gaffer::Context *conte
 	ComputeNode::hash( output, context, h );
 	if( output == outPlug() )
 	{
-		if( enabledPlug()->getValue() )
+		if( enabled( context ) )
 		{
 			/// \todo In SceneNode (and other enableable nodes) we
 			/// require methods like hashMatch() to call the base class
@@ -137,7 +138,7 @@ void Filter::compute( ValuePlug *output, const Context *context ) const
 	if( output == outPlug() )
 	{
 		unsigned match = IECore::PathMatcher::NoMatch;
-		if( enabledPlug()->getValue() )
+		if( enabled( context ) )
 		{
 			match = computeMatch( getInputScene( context ), context );
 		}
@@ -166,3 +167,30 @@ unsigned Filter::computeMatch( const ScenePlug *scene, const Gaffer::Context *co
 {
 	return IECore::PathMatcher::NoMatch;
 }
+
+bool Filter::enabled( const Gaffer::Context *context ) const
+{
+	const BoolPlug *plug = enabledPlug();
+	const BoolPlug *sourcePlug = plug->source<BoolPlug>();
+	if( !sourcePlug || sourcePlug->direction() == Plug::Out )
+	{
+		// Value may be computed. We use a global scope for two reasons :
+		//
+		// - Because our implementation assumes the result is constant across
+		//   the scene, and allowing it to vary by `scene:path` could produce
+		//   results where AncestorMatch and DescendantMatch are not consistent across locations.
+		// - To reduce pressure on the hash cache.
+		//
+		// > Note : `sourcePlug` will be null if the source is not a BoolPlug.
+		// > In this case we call `getValue()` on `plug` and it will perform the
+		// > appropriate type conversion.
+		ScenePlug::GlobalScope globalScope( context );
+		return sourcePlug ? sourcePlug->getValue() : plug->getValue();
+	}
+	else
+	{
+		// Value is not computed so context is irrelevant.
+		// Avoid overhead of context creation.
+		return sourcePlug->getValue();
+	}
+}