From ac5c33b07d2f63cc7037b9038344e9c06fd184de Mon Sep 17 00:00:00 2001 From: John Haddon Date: Wed, 20 Sep 2023 16:34:25 +0100 Subject: [PATCH 1/4] Process : Remove non-const `handleException()` method --- Changes.md | 1 + include/Gaffer/Process.h | 2 -- src/Gaffer/Process.cpp | 5 ----- 3 files changed, 1 insertion(+), 7 deletions(-) diff --git a/Changes.md b/Changes.md index fecb436c141..fbc3e0e39c6 100644 --- a/Changes.md +++ b/Changes.md @@ -5,6 +5,7 @@ Breaking Changes ---------------- - Dispatcher : Removed `createMatching()` method. +- Process : Removed non-const variant of the `handleException()` method. 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/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; From 9a71bece19602920748fa43d6a4526259ac04322 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 21 Sep 2023 09:36:56 +0100 Subject: [PATCH 2/4] Plug Tests : Test `_precomputedHash` more robustly Our hash caching is now so aggressive that even if the precomputed hash was ignored, we'd still be pulling the hash out of the cache. So we now clear the cache to be sure that if the precomputed hash was ignored, that would trigger another call to `ComputeNode::hash()`. --- python/GafferTest/NumericPlugTest.py | 13 +++++-------- python/GafferTest/TypedPlugTest.py | 13 +++++-------- python/GafferTest/ValuePlugTest.py | 16 ++++++---------- 3 files changed, 16 insertions(+), 26 deletions(-) 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/TypedPlugTest.py b/python/GafferTest/TypedPlugTest.py index 6c48492492d..b192e17d1fe 100644 --- a/python/GafferTest/TypedPlugTest.py +++ b/python/GafferTest/TypedPlugTest.py @@ -208,17 +208,14 @@ def testPrecomputedHash( self ) : 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. + # 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 ), "hi" ) - self.assertEqual( n.numHashCalls, numHashCalls ) + 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 ) : From bc79202d1cd124fbe8c60ed9a59039215e0f1914 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Wed, 20 Sep 2023 16:42:32 +0100 Subject: [PATCH 3/4] StringPlug : Remove deprecated `precomputedHash` argument from `getValue()` And adjust `TypedPlugTest.testPrecomputedHash()` to actually test a TypedPlug instead of a StringPlug. --- Changes.md | 1 + include/Gaffer/StringPlug.h | 6 ++-- python/GafferTest/StringPlugTest.py | 6 ---- python/GafferTest/TypedPlugTest.py | 46 +++++++++++++++++++++++--- src/Gaffer/StringPlug.cpp | 4 +-- src/GafferModule/StringPlugBinding.cpp | 6 ++-- 6 files changed, 50 insertions(+), 19 deletions(-) diff --git a/Changes.md b/Changes.md index fbc3e0e39c6..0764aaa0aef 100644 --- a/Changes.md +++ b/Changes.md @@ -6,6 +6,7 @@ Breaking Changes - Dispatcher : Removed `createMatching()` method. - Process : Removed non-const variant of the `handleException()` method. +- StringPlug : Removed deprecated `precomputedHash` argument from `getValue()` method. 1.3.x.x (relative to 1.3.3.0) ======= 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/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 b192e17d1fe..4cc7bca2232 100644 --- a/python/GafferTest/TypedPlugTest.py +++ b/python/GafferTest/TypedPlugTest.py @@ -200,10 +200,48 @@ 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 ) @@ -214,7 +252,7 @@ def testPrecomputedHash( self ) : # 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 ), "hi" ) + self.assertEqual( n["out"].getValue( _precomputedHash = h ), imath.M44f() ) self.assertEqual( n.numHashCalls, 1 ) self.assertEqual( n.numComputeCalls, 1 ) 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/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 ) ) ); From bc31bfb6a4b7ac2659f8e5a6a9b085c0e673c00d Mon Sep 17 00:00:00 2001 From: John Haddon Date: Wed, 20 Sep 2023 16:51:19 +0100 Subject: [PATCH 4/4] OpenColorIOContext : Don't duplicate OptionalValuePlug API --- Changes.md | 1 + include/GafferImage/OpenColorIOContext.h | 23 ++------- src/GafferImage/OpenColorIOContext.cpp | 64 +++++------------------- 3 files changed, 18 insertions(+), 70 deletions(-) diff --git a/Changes.md b/Changes.md index 0764aaa0aef..81e95a75686 100644 --- a/Changes.md +++ b/Changes.md @@ -7,6 +7,7 @@ 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/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/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();