From d4ccc8e03f09a68e01a5dad06608c6771fc20ce5 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Tue, 18 Jun 2024 14:07:32 +0100 Subject: [PATCH 1/2] Loop : Fix handling of empty `indexVariable` This now disables the Loop, instead of creating a context variable with an empty name. --- Changes.md | 1 + python/GafferTest/LoopTest.py | 15 +++++++++++++++ src/Gaffer/Loop.cpp | 4 ++-- 3 files changed, 18 insertions(+), 2 deletions(-) diff --git a/Changes.md b/Changes.md index ed4f458d796..12a269a6131 100644 --- a/Changes.md +++ b/Changes.md @@ -12,6 +12,7 @@ Fixes - Cycles : Fixed rendering to the Catalogue using the batch Render node (#5905). Note that rendering a mixture of Catalogue and file outputs is still not supported, and in this case any file outputs will be ignored. - CodeWidget : Fixed bug that could prevent changes from being committed while the completion menu was visible. +- Loop : Fixed handling of empty `indexVariable`. This now disables the Loop instead of creating an unnamed context variable. 1.4.7.0 (relative to 1.4.6.0) ======= diff --git a/python/GafferTest/LoopTest.py b/python/GafferTest/LoopTest.py index 0cf4d975f19..ca86650eef0 100644 --- a/python/GafferTest/LoopTest.py +++ b/python/GafferTest/LoopTest.py @@ -307,5 +307,20 @@ def testHashAliasingDeadlock( self ) : # if not handled appropriately. script["loop"]["out"].getValue() + def testEmptyLoopVariable( self ) : + + loop = self.intLoop() + + loopBody = GafferTest.AddNode() + loopBody["op1"].setInput( loop["previous"] ) + loopBody["op2"].setValue( 2 ) + loop["next"].setInput( loopBody["sum"] ) + loop["iterations"].setValue( 4 ) + + self.assertEqual( loop["out"].getValue(), 8 ) + + loop["indexVariable"].setValue( "" ) + self.assertEqual( loop["out"].getValue(), 0 ) + if __name__ == "__main__": unittest.main() diff --git a/src/Gaffer/Loop.cpp b/src/Gaffer/Loop.cpp index 7c43b2dcd8e..4d91feea70e 100644 --- a/src/Gaffer/Loop.cpp +++ b/src/Gaffer/Loop.cpp @@ -354,7 +354,7 @@ const ValuePlug *Loop::sourcePlug( const ValuePlug *output, const Context *conte if( ancestor == previousPlug() ) { const int index = context->get( indexVariable, 0 ); - if( index >= 1 && enabledPlug()->getValue() ) + if( index >= 1 && !indexVariable.string().empty() && enabledPlug()->getValue() ) { sourceLoopIndex = index - 1; return descendantPlug( nextPlug(), relativeName ); @@ -367,7 +367,7 @@ const ValuePlug *Loop::sourcePlug( const ValuePlug *output, const Context *conte else if( ancestor == outPlug() ) { const int iterations = iterationsPlug()->getValue(); - if( iterations > 0 && enabledPlug()->getValue() ) + if( iterations > 0 && !indexVariable.string().empty() && enabledPlug()->getValue() ) { sourceLoopIndex = iterations - 1; return descendantPlug( nextPlug(), relativeName ); From f84cd46edf5d9cdac6a117d95909222417f403a4 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Tue, 18 Jun 2024 14:15:12 +0100 Subject: [PATCH 2/2] Loop : Add `nextIterationContext()` method --- Changes.md | 5 +++ include/Gaffer/Loop.h | 4 +++ python/GafferTest/LoopTest.py | 33 ++++++++++++++++++++ src/Gaffer/Loop.cpp | 33 ++++++++++++++++++++ src/GafferModule/ContextProcessorBinding.cpp | 7 +++++ 5 files changed, 82 insertions(+) diff --git a/Changes.md b/Changes.md index 12a269a6131..d06b13580b9 100644 --- a/Changes.md +++ b/Changes.md @@ -14,6 +14,11 @@ Fixes - CodeWidget : Fixed bug that could prevent changes from being committed while the completion menu was visible. - Loop : Fixed handling of empty `indexVariable`. This now disables the Loop instead of creating an unnamed context variable. +API +--- + +- Loop : Added `nextIterationContext()` method. + 1.4.7.0 (relative to 1.4.6.0) ======= diff --git a/include/Gaffer/Loop.h b/include/Gaffer/Loop.h index 4a88296f16c..f0990dc296b 100644 --- a/include/Gaffer/Loop.h +++ b/include/Gaffer/Loop.h @@ -81,6 +81,10 @@ class GAFFER_API Loop : public ComputeNode Gaffer::Plug *correspondingInput( const Gaffer::Plug *output ) override; const Gaffer::Plug *correspondingInput( const Gaffer::Plug *output ) const override; + /// Returns the context that will be used to evaluate `nextPlug()` in + /// the next iteration of the loop (relative to the current context). + ContextPtr nextIterationContext() const; + void affects( const Plug *input, DependencyNode::AffectedPlugsContainer &outputs ) const override; protected : diff --git a/python/GafferTest/LoopTest.py b/python/GafferTest/LoopTest.py index ca86650eef0..d9ba6877d4b 100644 --- a/python/GafferTest/LoopTest.py +++ b/python/GafferTest/LoopTest.py @@ -322,5 +322,38 @@ def testEmptyLoopVariable( self ) : loop["indexVariable"].setValue( "" ) self.assertEqual( loop["out"].getValue(), 0 ) + def testNextLoopIteration( self ) : + + loop = Gaffer.Loop() + self.assertIsNone( loop.nextIterationContext() ) + + loop = self.intLoop() + loop["next"].setInput( loop["previous"] ) + loop["iterations"].setValue( 10 ) + + index = 0 + context = Gaffer.Context() + while context is not None : + with context : + context = loop.nextIterationContext() + if context is not None : + self.assertEqual( context["loop:index"], index ) + index += 1 + + self.assertEqual( index, loop["iterations"].getValue() ) + + loop["iterations"].setValue( 0 ) + self.assertIsNone( loop.nextIterationContext() ) + + loop["iterations"].setValue( 2 ) + self.assertIsNotNone( loop.nextIterationContext() ) + loop["enabled"].setValue( False ) + self.assertIsNone( loop.nextIterationContext() ) + + loop["enabled"].setValue( True ) + self.assertIsNotNone( loop.nextIterationContext() ) + loop["indexVariable"].setValue( "" ) + self.assertIsNone( loop.nextIterationContext() ) + if __name__ == "__main__": unittest.main() diff --git a/src/Gaffer/Loop.cpp b/src/Gaffer/Loop.cpp index 4d91feea70e..beca06e7144 100644 --- a/src/Gaffer/Loop.cpp +++ b/src/Gaffer/Loop.cpp @@ -160,6 +160,39 @@ const Gaffer::Plug *Loop::correspondingInput( const Gaffer::Plug *output ) const return output == outPlug() ? inPlug() : nullptr; } +ContextPtr Loop::nextIterationContext() const +{ + const Plug *in = inPlug(); + if( !in ) + { + return nullptr; + } + + const Context *context = Context::current(); + ContextAlgo::GlobalScope globalScope( context, in ); + + if( !enabledPlug()->getValue() ) + { + return nullptr; + } + + const IECore::InternedString indexVariable = indexVariablePlug()->getValue(); + if( indexVariable.string().empty() ) + { + return nullptr; + } + + const int index = context->get( indexVariable, -1 ); + if( index + 1 < iterationsPlug()->getValue() ) + { + ContextPtr result = new Context( *context ); + result->set( indexVariable, index + 1 ); + return result; + } + + return nullptr; +} + void Loop::affects( const Plug *input, DependencyNode::AffectedPlugsContainer &outputs ) const { ComputeNode::affects( input, outputs ); diff --git a/src/GafferModule/ContextProcessorBinding.cpp b/src/GafferModule/ContextProcessorBinding.cpp index 77d1353feb8..a83b3b1ab19 100644 --- a/src/GafferModule/ContextProcessorBinding.cpp +++ b/src/GafferModule/ContextProcessorBinding.cpp @@ -70,6 +70,12 @@ void setupLoop( Loop &n, const ValuePlug &plug ) n.setup( &plug ); } +ContextPtr nextIterationContextWrapper( Loop &loop ) +{ + IECorePython::ScopedGILRelease gilRelease; + return loop.nextIterationContext(); +} + ContextPtr inPlugContext( const ContextProcessor &n ) { IECorePython::ScopedGILRelease gilRelease; @@ -189,6 +195,7 @@ void GafferModule::bindContextProcessor() DependencyNodeClass() .def( "setup", &setupLoop ) + .def( "nextIterationContext", &nextIterationContextWrapper ) ; DependencyNodeClass()