Merge pull request #1860 from johnhaddon/substitutionsFix

StringPlug substitutions fix

doc/source/ScriptingReference/index.md → doc/source/Reference/ScriptingReference/CommonOperations/index.md

@@ -1,5 +1,5 @@
-Scripting Reference
-===================
+Common Operations
+=================
 
 Node Graphs
 -----------

doc/source/Reference/ScriptingReference/StringSubstitutionSyntax/index.md

@@ -0,0 +1,15 @@
+String Substitution Syntax
+==========================
+
+```eval_rst
+======================== ============= ================= ===============
+Substitution             Syntax        Example           Result
+======================== ============= ================= ===============
+Frame number             \#            image.#.exr       image.1.exr
+Padded frame number      \####         image.####.exr    image.0001.exr
+Home directory           ~             ~/gaffer/projects /home/stanley/gaffer/projects
+Context variable         ${name}       ${scene:path}     /world/sphere
+Environment variable     ${name}       /disk1/${USER}    /disk1/stanley
+Escape special character \\			   \\$               $
+======================== ============= ================= ===============
+```

doc/source/Reference/ScriptingReference/index.md

@@ -0,0 +1,5 @@
+Scripting Reference
+===================
+
+- [Common Operations](CommonOperations/index.md)
+- [String Substitution Syntax](StringSubstitutionSyntax/index.md)

doc/source/Reference/index.md

@@ -0,0 +1,7 @@
+Reference
+=========
+
+- [UI Reference](UIReference/index.md)
+- [Node Reference](NodeReference/index.md)
+- [Scripting Reference](ScriptingReference/index.md)
+- [Command Line Reference](CommandLineReference/index.md)

doc/source/index.md

@@ -9,8 +9,5 @@ In addition to itself being highly extensible, Gaffer's underlying frameworks ar
 
 - [Installation](Installation/index.md)
 - [Tutorials](Tutorials/index.md)
-- [UI Reference](UIReference/index.md)
-- [Node Reference](NodeReference/index.md)
-- [Scripting Reference](ScriptingReference/index.md)
-- [Command Line Reference](CommandLineReference/index.md)
+- [Reference](Reference/index.md)
 - [Appendices](Appendices/index.md)

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

@@ -38,6 +38,7 @@
 from __future__ import with_statement
 
 import os
+import inspect
 import unittest
 
 import IECore
@@ -228,5 +229,119 @@ def testSubstitutionsCounterpart( self ) :
 		p2 = p.createCounterpart( "p2", p.Direction.In )
 		self.assertEqual( p.substitutions(), p2.substitutions() )
 
+	def testSubstitutionsFromExpressionInput( self ) :
+
+		s = Gaffer.ScriptNode()
+
+		# Should output a substituted version of the input.
+		s["substitionsOn"] = GafferTest.StringInOutNode()
+
+		# Should pass through the input directly, without substitutions.
+		s["substitionsOff"] = GafferTest.StringInOutNode( substitutions = Gaffer.Context.Substitutions.NoSubstitutions )
+
+		# 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. 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"] )
+
+		# All three nodes above receive their input from this expression
+		# which outputs a sequence value to be substituted (or not).
+
+		s["e"] = Gaffer.Expression()
+		s["e"].setExpression( inspect.cleandoc(
+			"""
+			parent["substitionsOn"]["in"] = "test.#.exr"
+			parent["substitionsOff"]["in"] = "test.#.exr"
+			parent["substitionsOnIndirectly"]["user"]["in"] = "test.#.exr"
+			"""
+		) )
+
+		with Gaffer.Context() as c :
+
+			# Frame 1
+			#########
+
+			c.setFrame( 1 )
+
+			# The output of the expression itself is not substituted.
+			# Substitutions occur only on input plugs.
+
+			self.assertEqual( s["substitionsOn"]["in"].getInput().getValue(), "test.#.exr" )
+			self.assertEqual( s["substitionsOff"]["in"].getInput().getValue(), "test.#.exr" )
+			self.assertEqual( s["substitionsOnIndirectly"]["user"]["in"].getInput().getValue(), "test.#.exr" )
+
+			# We should get frame numbers out of the substituting node.
+
+			self.assertEqual( s["substitionsOn"]["out"].getValue(), "test.1.exr" )
+			substitutionsOnHash1 = s["substitionsOn"]["out"].hash()
+			self.assertEqual( s["substitionsOn"]["out"].getValue( _precomputedHash = substitutionsOnHash1 ), "test.1.exr" )
+
+			# We should get sequences out of the non-substituting node.
+
+			self.assertEqual( s["substitionsOff"]["out"].getValue(), "test.#.exr" )
+			substitutionsOffHash1 = s["substitionsOff"]["out"].hash()
+			self.assertEqual( s["substitionsOff"]["out"].getValue( _precomputedHash = substitutionsOffHash1 ), "test.#.exr" )
+			self.assertNotEqual( substitutionsOnHash1, substitutionsOffHash1 )
+
+			# 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.#.exr" )
+			substitionsOnIndirectlyHash1 = s["substitionsOnIndirectly"]["out"].hash()
+			self.assertEqual( s["substitionsOnIndirectly"]["out"].getValue( _precomputedHash = substitionsOnIndirectlyHash1 ), "test.#.exr" )
+
+			# Frame 2
+			#########
+
+			c.setFrame( 2 )
+
+			# The output of the expression itself is not substituted.
+			# Substitutions occur only on input plugs.
+
+			self.assertEqual( s["substitionsOn"]["in"].getInput().getValue(), "test.#.exr" )
+			self.assertEqual( s["substitionsOff"]["in"].getInput().getValue(), "test.#.exr" )
+			self.assertEqual( s["substitionsOnIndirectly"]["user"]["in"].getInput().getValue(), "test.#.exr" )
+
+			# We should get frame numbers out of the substituting node.
+			# The hash must has changed to make this possible.
+
+			self.assertEqual( s["substitionsOn"]["out"].getValue(), "test.2.exr" )
+			substitutionsOnHash2 = s["substitionsOn"]["out"].hash()
+			self.assertEqual( s["substitionsOn"]["out"].getValue( _precomputedHash = substitutionsOnHash2 ), "test.2.exr" )
+			self.assertNotEqual( substitutionsOnHash2, substitutionsOnHash1 )
+
+			# We should still get sequences out of the non-substituting node,
+			# and it should have the same output hash as it had on frame 1.
+
+			self.assertEqual( s["substitionsOff"]["out"].getValue(), "test.#.exr" )
+			substitutionsOffHash2 = s["substitionsOff"]["out"].hash()
+			self.assertEqual( s["substitionsOff"]["out"].getValue( _precomputedHash = substitutionsOffHash2 ), "test.#.exr" )
+			self.assertEqual( substitutionsOffHash1, substitutionsOffHash2 )
+			self.assertNotEqual( substitutionsOnHash2, substitutionsOffHash2 )
+
+			# The third node should still be non-substituting.
+
+			self.assertEqual( s["substitionsOnIndirectly"]["out"].getValue(), "test.#.exr" )
+			substitionsOnIndirectlyHash2 = s["substitionsOnIndirectly"]["out"].hash()
+			self.assertEqual( s["substitionsOnIndirectly"]["out"].getValue( _precomputedHash = substitionsOnIndirectlyHash2 ), "test.#.exr" )
+			self.assertEqual( substitionsOnIndirectlyHash2, substitionsOnIndirectlyHash1 )
+
 if __name__ == "__main__":
 	unittest.main()

src/Gaffer/StringPlug.cpp

@@ -101,14 +101,15 @@ std::string StringPlug::getValue( const IECore::MurmurHash *precomputedHash ) co
 		throw IECore::Exception( "StringPlug::getObjectValue() didn't return StringData - is the hash being computed correctly?" );
 	}
 
-	bool performSubstitution =
+	const bool performSubstitutions =
 		m_substitutions &&
-		direction()==Plug::In &&
+		direction() == In &&
+		getFlags( PerformsSubstitutions ) &&
 		Process::current() &&
-		Plug::getFlags( Plug::PerformsSubstitutions ) &&
-		Context::hasSubstitutions( s->readable() );
+		Context::hasSubstitutions( s->readable() )
+	;
 
-	return performSubstitution ? Context::current()->substitute( s->readable(), m_substitutions ) : s->readable();
+	return performSubstitutions ? Context::current()->substitute( s->readable(), m_substitutions ) : s->readable();
 }
 
 void StringPlug::setFrom( const ValuePlug *other )
@@ -126,12 +127,15 @@ void StringPlug::setFrom( const ValuePlug *other )
 
 IECore::MurmurHash StringPlug::hash() const
 {
-	const StringPlug *p = source<StringPlug>();
+	const bool performSubstitutions =
+		m_substitutions &&
+		direction() == In &&
+		getFlags( PerformsSubstitutions )
+	;
 
-	const bool performSubstitution = m_substitutions && p->direction()==Plug::In && p->getFlags( Plug::PerformsSubstitutions );
-	if( performSubstitution )
+	if( performSubstitutions )
 	{
-		IECore::ConstObjectPtr o = p->getObjectValue();
+		IECore::ConstObjectPtr o = getObjectValue();
 		const IECore::StringData *s = IECore::runTimeCast<const IECore::StringData>( o.get() );
 		if( !s )
 		{
@@ -147,5 +151,5 @@ IECore::MurmurHash StringPlug::hash() const
 	}
 
 	// no substitutions
-	return p->ValuePlug::hash();
+	return ValuePlug::hash();
 }