StringPlug : Rationalise substitutions logic.

See comments for further details.

include/Gaffer/StringPlug.h

@@ -44,6 +44,41 @@
 namespace Gaffer
 {
 
+/// Plug for providing string values.
+///
+/// Substitutions
+/// =============
+///
+/// Substitutions allow the user to enter values containing
+/// frame numbers and the values of context variables, and
+/// have the appropriate values substituted in automatically
+/// during computation.
+///
+/// e.g. "~/images/${name}.####.exr" -> "/home/bob/beauty.0001.exr"
+///
+/// Substitutions are performed transparently when `getValue()`
+/// is called for an input plug from within a current `Process`,
+/// so no specific action is required on the part of the Node
+/// developer to support them.
+///
+///	If a node needs to deal with sequences directly, or otherwise
+/// access unsubstituted values, the `substitutions` constructor
+/// argument may be used to disable specific substitutions.
+///
+/// > Note : This feature does not affect the values passed
+///	> internally between string plugs - substitutions are only
+/// > applied to the return value generated for `getValue()`.
+/// > This is important, since it allows a downstream node to
+/// > access an unsubstituted value from its input, even if
+/// > an intermediate upstream plug has substitutions enabled
+/// > for other purposes.
+/// >
+/// > In other words, substitutions could just as well be
+/// > implemented using an explicit `getSubstitutedValue()`
+/// > method or by performing a manual substitution after using
+/// > `getValue()`. However, in practice, it was determined to
+/// > be too error prone to remember to do this for every
+/// > value access in every node.
 class StringPlug : public ValuePlug
 {
 

python/GafferTest/StringPlugTest.py

@@ -242,7 +242,19 @@ def testSubstitutionsFromExpressionInput( self ) :
 		# The third case is trickier. The "in" plug on the node
 		# itself requests no substitutions, but it receives its
 		# input via an indirect connection with substitutions
-		# turned on.
+		# turned on. We resolve this by defining substitutions
+		# to occur only when observing a value inside a compute,
+		# and to always be determined by the plug used to access
+		# the value. A chain of connections can be thought of as
+		# carrying an unsubstituted string all the way along
+		# internally, with each plug along the way determining
+		# the substitutions applied when peeking in to see the value
+		# at that point.
+		#
+		# In practice this works best because typically it is only
+		# nodes that know when a substitution is relevant, and the
+		# user shouldn't be burdened with the job of thinking about
+		# them when making intermediate connections to that node.
 		s["substitionsOnIndirectly"] = GafferTest.StringInOutNode( substitutions = Gaffer.Context.Substitutions.NoSubstitutions )
 		s["substitionsOnIndirectly"]["user"]["in"] = Gaffer.StringPlug()
 		s["substitionsOnIndirectly"]["in"].setInput( s["substitionsOnIndirectly"]["user"]["in"] )
@@ -286,12 +298,14 @@ def testSubstitutionsFromExpressionInput( self ) :
 			self.assertEqual( s["substitionsOff"]["out"].getValue( _precomputedHash = substitutionsOffHash1 ), "test.#.exr" )
 			self.assertNotEqual( substitutionsOnHash1, substitutionsOffHash1 )
 
-			# We should get frame numbers out of the third node, because the intermediate
-			# plug performs the substitutions before they even get to the node.
+			# We shouldn't get frame numbers out of the third node, because the
+			# requirements of the node (no substitutions) trump any upstream opinions.
+			# Substitutions are performed by the plug during value access, and do not
+			# affect the actual data flow.
 
-			self.assertEqual( s["substitionsOnIndirectly"]["out"].getValue(), "test.1.exr" )
+			self.assertEqual( s["substitionsOnIndirectly"]["out"].getValue(), "test.#.exr" )
 			substitionsOnIndirectlyHash1 = s["substitionsOnIndirectly"]["out"].hash()
-			self.assertEqual( s["substitionsOnIndirectly"]["out"].getValue( _precomputedHash = substitionsOnIndirectlyHash1 ), "test.1.exr" )
+			self.assertEqual( s["substitionsOnIndirectly"]["out"].getValue( _precomputedHash = substitionsOnIndirectlyHash1 ), "test.#.exr" )
 
 			# Frame 2
 			#########
@@ -322,13 +336,12 @@ def testSubstitutionsFromExpressionInput( self ) :
 			self.assertEqual( substitutionsOffHash1, substitutionsOffHash2 )
 			self.assertNotEqual( substitutionsOnHash2, substitutionsOffHash2 )
 
-			# The third node should still be substituting, also with an updated hash
-			# and frame number.
+			# The third node should still be non-substituting.
 
-			self.assertEqual( s["substitionsOnIndirectly"]["out"].getValue(), "test.2.exr" )
+			self.assertEqual( s["substitionsOnIndirectly"]["out"].getValue(), "test.#.exr" )
 			substitionsOnIndirectlyHash2 = s["substitionsOnIndirectly"]["out"].hash()
-			self.assertEqual( s["substitionsOnIndirectly"]["out"].getValue( _precomputedHash = substitionsOnIndirectlyHash2 ), "test.2.exr" )
-			self.assertNotEqual( substitionsOnIndirectlyHash2, substitionsOnIndirectlyHash1 )
+			self.assertEqual( s["substitionsOnIndirectly"]["out"].getValue( _precomputedHash = substitionsOnIndirectlyHash2 ), "test.#.exr" )
+			self.assertEqual( substitionsOnIndirectlyHash2, substitionsOnIndirectlyHash1 )
 
 if __name__ == "__main__":
 	unittest.main()

src/Gaffer/StringPlug.cpp

@@ -42,40 +42,6 @@
 using namespace IECore;
 using namespace Gaffer;
 
-//////////////////////////////////////////////////////////////////////////
-// Internal utilities
-//////////////////////////////////////////////////////////////////////////
-
-namespace
-{
-
-const StringPlug *sourceAndSubstitutions( const StringPlug *plug, unsigned &substitutions )
-{
-	substitutions = Context::NoSubstitutions;
-	const StringPlug *input = NULL;
-	do
-	{
-		if( ( plug->direction() == Plug::In ) && ( plug->getFlags() & Plug::PerformsSubstitutions ) )
-		{
-			substitutions |= plug->substitutions();
-		}
-
-		input = plug->getInput<StringPlug>();
-		if( input )
-		{
-			plug = input;
-		}
-	} while( input );
-
-	return plug;
-}
-
-} // namespace
-
-//////////////////////////////////////////////////////////////////////////
-// StringPlug
-//////////////////////////////////////////////////////////////////////////
-
 IE_CORE_DEFINERUNTIMETYPED( StringPlug );
 
 StringPlug::StringPlug(
@@ -128,18 +94,22 @@ void StringPlug::setValue( const std::string &value )
 
 std::string StringPlug::getValue( const IECore::MurmurHash *precomputedHash ) const
 {
-	unsigned substitutions;
-	const StringPlug *p = sourceAndSubstitutions( this, substitutions );
-
-	IECore::ConstObjectPtr o = p->getObjectValue( precomputedHash );
+	IECore::ConstObjectPtr o = getObjectValue( precomputedHash );
 	const IECore::StringData *s = IECore::runTimeCast<const IECore::StringData>( o.get() );
 	if( !s )
 	{
 		throw IECore::Exception( "StringPlug::getObjectValue() didn't return StringData - is the hash being computed correctly?" );
 	}
 
-	const bool performSubstitutions = substitutions && Process::current() && Context::hasSubstitutions( s->readable() );
-	return performSubstitutions ? Context::current()->substitute( s->readable(), substitutions ) : s->readable();
+	const bool performSubstitutions =
+		m_substitutions &&
+		direction() == In &&
+		getFlags( PerformsSubstitutions ) &&
+		Process::current() &&
+		Context::hasSubstitutions( s->readable() )
+	;
+
+	return performSubstitutions ? Context::current()->substitute( s->readable(), m_substitutions ) : s->readable();
 }
 
 void StringPlug::setFrom( const ValuePlug *other )
@@ -157,12 +127,15 @@ void StringPlug::setFrom( const ValuePlug *other )
 
 IECore::MurmurHash StringPlug::hash() const
 {
-	unsigned substitutions;
-	const StringPlug *p = sourceAndSubstitutions( this, substitutions );
+	const bool performSubstitutions =
+		m_substitutions &&
+		direction() == In &&
+		getFlags( PerformsSubstitutions )
+	;
 
-	if( substitutions )
+	if( performSubstitutions )
 	{
-		IECore::ConstObjectPtr o = p->getObjectValue();
+		IECore::ConstObjectPtr o = getObjectValue();
 		const IECore::StringData *s = IECore::runTimeCast<const IECore::StringData>( o.get() );
 		if( !s )
 		{
@@ -172,11 +145,11 @@ IECore::MurmurHash StringPlug::hash() const
 		if( Context::hasSubstitutions( s->readable() ) )
 		{
 			IECore::MurmurHash result;
-			result.append( Context::current()->substitute( s->readable(), substitutions ) );
+			result.append( Context::current()->substitute( s->readable(), m_substitutions ) );
 			return result;
 		}
 	}
 
 	// no substitutions
-	return p->ValuePlug::hash();
+	return ValuePlug::hash();
 }