diff --git a/Changes.md b/Changes.md index fecb436c141..81e95a75686 100644 --- a/Changes.md +++ b/Changes.md @@ -5,6 +5,9 @@ Breaking Changes ---------------- - Dispatcher : Removed `createMatching()` method. +- Process : Removed non-const variant of the `handleException()` method. +- StringPlug : Removed deprecated `precomputedHash` argument from `getValue()` method. +- OpenColorIOContext : Removed `configEnabledPlug()`, `configValuePlug()`, `workingSpaceEnabledPlug()` and `workingSpaceValuePlug()` methods. Use the OptionalValuePlug child accessors instead. 1.3.x.x (relative to 1.3.3.0) ======= diff --git a/include/Gaffer/Process.h b/include/Gaffer/Process.h index e69eb11f8b1..c2841aff26f 100644 --- a/include/Gaffer/Process.h +++ b/include/Gaffer/Process.h @@ -98,8 +98,6 @@ class GAFFER_API Process : private ThreadState::Scope /// and rethrow the exception for propagation back to /// the original caller. [[noreturn]] void handleException() const; - /// \todo This just exists for ABI compatibility. Remove it. - void handleException(); private : diff --git a/include/Gaffer/StringPlug.h b/include/Gaffer/StringPlug.h index 3a9eb61b958..2240a3593b5 100644 --- a/include/Gaffer/StringPlug.h +++ b/include/Gaffer/StringPlug.h @@ -118,10 +118,8 @@ class GAFFER_API StringPlug : public ValuePlug /// > (which would be `\` on Windows). /// \undoable void setValue( const std::filesystem::path &value ); - /// Returns the value. The `precomputedHash` argument is deprecated, and - /// will be removed in a future release. - /// \todo Remove `precomputedHash` argument. - std::string getValue( const IECore::MurmurHash *precomputedHash = nullptr ) const; + /// Returns the value. + std::string getValue() const; void setFrom( const ValuePlug *other ) override; diff --git a/include/GafferImage/OpenColorIOContext.h b/include/GafferImage/OpenColorIOContext.h index 7788982664a..185511e2733 100644 --- a/include/GafferImage/OpenColorIOContext.h +++ b/include/GafferImage/OpenColorIOContext.h @@ -41,6 +41,7 @@ #include "Gaffer/CompoundDataPlug.h" #include "Gaffer/ContextProcessor.h" +#include "Gaffer/OptionalValuePlug.h" #include "Gaffer/TypedObjectPlug.h" namespace GafferImage @@ -56,25 +57,11 @@ class GAFFERIMAGE_API OpenColorIOContext : public Gaffer::ContextProcessor explicit OpenColorIOContext( const std::string &name=GraphComponent::defaultName() ); ~OpenColorIOContext() override; - /// \todo Return OptionalValuePlug, and remove `configEnabledPlug()` and - /// `configValuePlug()` methods. Do the same for `workingSpace` plugs. - Gaffer::ValuePlug *configPlug(); - const Gaffer::ValuePlug *configPlug() const; + Gaffer::OptionalValuePlug *configPlug(); + const Gaffer::OptionalValuePlug *configPlug() const; - Gaffer::BoolPlug *configEnabledPlug(); - const Gaffer::BoolPlug *configEnabledPlug() const; - - Gaffer::StringPlug *configValuePlug(); - const Gaffer::StringPlug *configValuePlug() const; - - Gaffer::ValuePlug *workingSpacePlug(); - const Gaffer::ValuePlug *workingSpacePlug() const; - - Gaffer::BoolPlug *workingSpaceEnabledPlug(); - const Gaffer::BoolPlug *workingSpaceEnabledPlug() const; - - Gaffer::StringPlug *workingSpaceValuePlug(); - const Gaffer::StringPlug *workingSpaceValuePlug() const; + Gaffer::OptionalValuePlug *workingSpacePlug(); + const Gaffer::OptionalValuePlug *workingSpacePlug() const; Gaffer::ValuePlug *variablesPlug(); const Gaffer::ValuePlug *variablesPlug() const; diff --git a/python/GafferTest/NumericPlugTest.py b/python/GafferTest/NumericPlugTest.py index 92ab1a28992..3d092810732 100644 --- a/python/GafferTest/NumericPlugTest.py +++ b/python/GafferTest/NumericPlugTest.py @@ -450,17 +450,14 @@ def testPrecomputedHash( self ) : self.assertEqual( n.numComputeCalls, 1 ) h = n["sum"].hash() - numHashCalls = n.numHashCalls - # Accept either 1 or 2 - it would be reasonable for the ValuePlug - # to have either cached the hash or not, but that's not what we're - # testing here. - self.assertTrue( numHashCalls == 1 or numHashCalls == 2 ) + self.assertEqual( n.numHashCalls, 1 ) self.assertEqual( n.numComputeCalls, 1 ) - # What we care about is that calling getValue() with a precomputed hash - # definitely doesn't recompute the hash again. + # Calling `getValue()`` with a precomputed hash shouldn't recompute the + # hash again, even if it has been cleared from the cache. + Gaffer.ValuePlug.clearHashCache() self.assertEqual( n["sum"].getValue( _precomputedHash = h ), 30 ) - self.assertEqual( n.numHashCalls, numHashCalls ) + self.assertEqual( n.numHashCalls, 1 ) self.assertEqual( n.numComputeCalls, 1 ) def testIsSetToDefault( self ) : diff --git a/python/GafferTest/StringPlugTest.py b/python/GafferTest/StringPlugTest.py index a546733f1e0..f34edd9e708 100644 --- a/python/GafferTest/StringPlugTest.py +++ b/python/GafferTest/StringPlugTest.py @@ -304,13 +304,11 @@ def testSubstitutionsFromExpressionInput( self ) : 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 @@ -320,7 +318,6 @@ def testSubstitutionsFromExpressionInput( self ) : 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 ######### @@ -339,7 +336,6 @@ def testSubstitutionsFromExpressionInput( self ) : 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, @@ -347,7 +343,6 @@ def testSubstitutionsFromExpressionInput( self ) : 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 ) @@ -355,7 +350,6 @@ def testSubstitutionsFromExpressionInput( self ) : 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 ) def testHashUsesValue( self ) : diff --git a/python/GafferTest/TypedPlugTest.py b/python/GafferTest/TypedPlugTest.py index 6c48492492d..4cc7bca2232 100644 --- a/python/GafferTest/TypedPlugTest.py +++ b/python/GafferTest/TypedPlugTest.py @@ -200,25 +200,60 @@ def testNoChildrenAccepted( self ) : def testPrecomputedHash( self ) : - n = GafferTest.StringInOutNode() - n["in"].setValue( "hi" ) + class MatrixMultiplyNode( Gaffer.ComputeNode ) : - self.assertEqual( n["out"].getValue(), "hi" ) + def __init__( self, name = "MatrixMultiply" ) : + + Gaffer.ComputeNode.__init__( self, name ) + + self["in1"] = Gaffer.M44fPlug() + self["in2"] = Gaffer.M44fPlug() + self["out"] = Gaffer.M44fPlug( direction = Gaffer.Plug.Direction.Out ) + + self.numComputeCalls = 0 + self.numHashCalls = 0 + + def affects( self, input ) : + + outputs = Gaffer.ComputeNode.affects( self, input ) + if input.isSame( self["in1"] ) or input.isSame( self["in2"] ) : + outputs.append( self.getChild( "out" ) ) + + return outputs + + def hash( self, output, context, h ) : + + assert( output.isSame( self.getChild( "out" ) ) ) + + self["in1"].hash( h ) + self["in2"].hash( h ) + + self.numHashCalls += 1 + + def compute( self, output, context ) : + + assert( output.isSame( self.getChild( "out" ) ) ) + output.setValue( self["in1"].getValue() * self["in2"].getValue() ) + + self.numComputeCalls += 1 + + IECore.registerRunTimeTyped( MatrixMultiplyNode ) + + n = MatrixMultiplyNode() + + self.assertEqual( n["out"].getValue(), imath.M44f() ) self.assertEqual( n.numHashCalls, 1 ) self.assertEqual( n.numComputeCalls, 1 ) h = n["out"].hash() - numHashCalls = n.numHashCalls - # Accept either 1 or 2 - it would be reasonable for the ValuePlug - # to have either cached the hash or not, but that's not what we're - # testing here. - self.assertTrue( numHashCalls == 1 or numHashCalls == 2 ) + self.assertEqual( n.numHashCalls, 1 ) self.assertEqual( n.numComputeCalls, 1 ) - # What we care about is that calling getValue() with a precomputed hash - # definitely doesn't recompute the hash again. - self.assertEqual( n["out"].getValue( _precomputedHash = h ), "hi" ) - self.assertEqual( n.numHashCalls, numHashCalls ) + # Calling `getValue()` with a precomputed hash shouldn't recompute the + # hash again, even if it has been cleared from the cache. + Gaffer.ValuePlug.clearHashCache() + self.assertEqual( n["out"].getValue( _precomputedHash = h ), imath.M44f() ) + self.assertEqual( n.numHashCalls, 1 ) self.assertEqual( n.numComputeCalls, 1 ) def testBoolPlugStringConnections( self ) : diff --git a/python/GafferTest/ValuePlugTest.py b/python/GafferTest/ValuePlugTest.py index facfdb3a59b..d840c56e5df 100644 --- a/python/GafferTest/ValuePlugTest.py +++ b/python/GafferTest/ValuePlugTest.py @@ -286,22 +286,18 @@ def testPrecomputedHashOptimisation( self ) : self.assertEqual( a1, IECore.StringData( "a" ) ) self.assertEqual( n.numHashCalls, 1 ) - # We apply some leeway in our test for how many hash calls are - # made - a good ValuePlug implementation will probably avoid - # unecessary repeated calls in most cases, but it's not - # what this unit test is about. a2 = n["out"].getValue( _copy = False ) self.assertTrue( a2.isSame( a1 ) ) - self.assertTrue( n.numHashCalls == 1 or n.numHashCalls == 2 ) + self.assertEqual( n.numHashCalls, 1 ) h = n["out"].hash() - self.assertTrue( n.numHashCalls >= 1 and n.numHashCalls <= 3 ) - numHashCalls = n.numHashCalls + self.assertEqual( n.numHashCalls, 1 ) - # What we care about is that calling getValue() with a precomputed hash - # definitely doesn't recompute the hash again. + # Calling `getValue()` with a precomputed hash shouldn't recompute the + # hash again, even if it has been cleared from the cache. + Gaffer.ValuePlug.clearHashCache() a3 = n["out"].getValue( _copy = False, _precomputedHash = h ) - self.assertEqual( n.numHashCalls, numHashCalls ) + self.assertEqual( n.numHashCalls, 1 ) self.assertTrue( a3.isSame( a1 ) ) def testSerialisationOfChildValues( self ) : diff --git a/src/Gaffer/Process.cpp b/src/Gaffer/Process.cpp index 51a98d249d9..1b121befa23 100644 --- a/src/Gaffer/Process.cpp +++ b/src/Gaffer/Process.cpp @@ -161,11 +161,6 @@ void Process::handleException() const } } -void Process::handleException() -{ - const_cast( this )->handleException(); -} - void Process::emitError( const std::string &error, const Plug *source ) const { const Plug *plug = m_destinationPlug; diff --git a/src/Gaffer/StringPlug.cpp b/src/Gaffer/StringPlug.cpp index 7dc947cda3b..f03a8b22268 100644 --- a/src/Gaffer/StringPlug.cpp +++ b/src/Gaffer/StringPlug.cpp @@ -106,9 +106,9 @@ void StringPlug::setValue( const std::filesystem::path &value ) setValue( value.generic_string() ); } -std::string StringPlug::getValue( const IECore::MurmurHash *precomputedHash ) const +std::string StringPlug::getValue() const { - ConstStringDataPtr s = getObjectValue( precomputedHash ); + ConstStringDataPtr s = getObjectValue(); const bool performSubstitutions = m_substitutions && diff --git a/src/GafferImage/OpenColorIOContext.cpp b/src/GafferImage/OpenColorIOContext.cpp index 5f9c2fafc01..31165b40648 100644 --- a/src/GafferImage/OpenColorIOContext.cpp +++ b/src/GafferImage/OpenColorIOContext.cpp @@ -67,64 +67,24 @@ OpenColorIOContext::~OpenColorIOContext() { } -Gaffer::ValuePlug *OpenColorIOContext::configPlug() +Gaffer::OptionalValuePlug *OpenColorIOContext::configPlug() { - return getChild( g_firstPlugIndex ); + return getChild( g_firstPlugIndex ); } -const Gaffer::ValuePlug *OpenColorIOContext::configPlug() const +const Gaffer::OptionalValuePlug *OpenColorIOContext::configPlug() const { - return getChild( g_firstPlugIndex ); + return getChild( g_firstPlugIndex ); } -Gaffer::BoolPlug *OpenColorIOContext::configEnabledPlug() +Gaffer::OptionalValuePlug *OpenColorIOContext::workingSpacePlug() { - return configPlug()->getChild( 0 ); + return getChild( g_firstPlugIndex + 1 ); } -const Gaffer::BoolPlug *OpenColorIOContext::configEnabledPlug() const +const Gaffer::OptionalValuePlug *OpenColorIOContext::workingSpacePlug() const { - return configPlug()->getChild( 0 ); -} - -Gaffer::StringPlug *OpenColorIOContext::configValuePlug() -{ - return configPlug()->getChild( 1 ); -} - -const Gaffer::StringPlug *OpenColorIOContext::configValuePlug() const -{ - return configPlug()->getChild( 1 ); -} - -Gaffer::ValuePlug *OpenColorIOContext::workingSpacePlug() -{ - return getChild( g_firstPlugIndex + 1 ); -} - -const Gaffer::ValuePlug *OpenColorIOContext::workingSpacePlug() const -{ - return getChild( g_firstPlugIndex + 1 ); -} - -Gaffer::BoolPlug *OpenColorIOContext::workingSpaceEnabledPlug() -{ - return workingSpacePlug()->getChild( 0 ); -} - -const Gaffer::BoolPlug *OpenColorIOContext::workingSpaceEnabledPlug() const -{ - return workingSpacePlug()->getChild( 0 ); -} - -Gaffer::StringPlug *OpenColorIOContext::workingSpaceValuePlug() -{ - return workingSpacePlug()->getChild( 1 ); -} - -const Gaffer::StringPlug *OpenColorIOContext::workingSpaceValuePlug() const -{ - return workingSpacePlug()->getChild( 1 ); + return getChild( g_firstPlugIndex + 1 ); } Gaffer::ValuePlug *OpenColorIOContext::variablesPlug() @@ -197,14 +157,14 @@ void OpenColorIOContext::compute( ValuePlug *output, const Context *context ) co IECore::CompoundDataPtr resultData = new IECore::CompoundData; IECore::CompoundDataMap &result = resultData->writable(); - if( configEnabledPlug()->getValue() ) + if( configPlug()->enabledPlug()->getValue() ) { - result["ocio:config"] = new StringData( configValuePlug()->getValue() ); + result["ocio:config"] = new StringData( configPlug()->valuePlug()->getValue() ); } - if( workingSpaceEnabledPlug()->getValue() ) + if( workingSpacePlug()->enabledPlug()->getValue() ) { - result["ocio:workingSpace"] = new StringData( workingSpaceValuePlug()->getValue() ); + result["ocio:workingSpace"] = new StringData( workingSpacePlug()->valuePlug()->getValue() ); } ConstCompoundDataPtr extraVariables = extraVariablesPlug()->getValue(); diff --git a/src/GafferModule/StringPlugBinding.cpp b/src/GafferModule/StringPlugBinding.cpp index 2f8d70ba46f..f07570843df 100644 --- a/src/GafferModule/StringPlugBinding.cpp +++ b/src/GafferModule/StringPlugBinding.cpp @@ -63,12 +63,12 @@ void setPathValue( StringPlug *plug, const std::filesystem::path &value ) plug->setValue( value ); } -std::string getValue( const StringPlug *plug, const IECore::MurmurHash *precomputedHash ) +std::string getValue( const StringPlug *plug ) { // Must release GIL in case computation spawns threads which need // to reenter Python. IECorePython::ScopedGILRelease r; - return plug->getValue( precomputedHash ); + return plug->getValue(); } std::string substitutionsRepr( unsigned substitutions ) @@ -161,7 +161,7 @@ void GafferModule::bindStringPlug() // Must be registered before string-based `setValue()`, to give it weaker overloading precedence. .def( "setValue", &setPathValue ) .def( "setValue", &setValue ) - .def( "getValue", &getValue, ( boost::python::arg( "_precomputedHash" ) = object() ) ) + .def( "getValue", &getValue ) ; s.attr( "ValueType" ) = boost::python::object( boost::python::handle<>( boost::python::borrowed( &PyUnicode_Type ) ) );