From 5ab2ff209f943d8e68af3bf565ac7dcca5d31526 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 23 Aug 2024 09:25:35 +0100 Subject: [PATCH 01/10] ArrayPlug : Fix `resize()` bugs - Fix error when resizing removed a plug with an input connection. In this case, the "resize when inputs change" behaviour was kicking in during the outer resize and messing everything up. - Fix failure to resize output plugs, due to attempting to add an input plug as a child. --- Changes.md | 3 +++ python/GafferTest/ArrayPlugTest.py | 16 ++++++++++++++++ src/Gaffer/ArrayPlug.cpp | 3 ++- 3 files changed, 21 insertions(+), 1 deletion(-) diff --git a/Changes.md b/Changes.md index 6d65a583e15..a3da856dc6a 100644 --- a/Changes.md +++ b/Changes.md @@ -35,6 +35,9 @@ Fixes - Fixed update of custom context-sensitive labels on Dot nodes. - GafferCortexUI : Removed usage of legacy PlugValueWidget API. - Dispatcher : Fixed crashes caused by a dispatcher's `SetupPlugsFn` attempting to access the TaskNode it was being called for. Dispatchers may now introspect the TaskNode and add different plugs based on type (#915). +- ArrayPlug : + - Fixed error when `resize()` removed plugs with input connections. + - Fixed error when `resize()` was used on an output plug. API --- diff --git a/python/GafferTest/ArrayPlugTest.py b/python/GafferTest/ArrayPlugTest.py index 4c75d9881e6..9d418db7adf 100644 --- a/python/GafferTest/ArrayPlugTest.py +++ b/python/GafferTest/ArrayPlugTest.py @@ -494,6 +494,22 @@ def testResize( self ) : with self.assertRaises( RuntimeError ) : p.resize( p.maxSize() + 1 ) + def testRemoveInputDuringResize( self ) : + + node = Gaffer.Node() + node["user"]["p"] = Gaffer.IntPlug() + node["user"]["array"] = Gaffer.ArrayPlug( element = Gaffer.IntPlug(), resizeWhenInputsChange = True ) + node["user"]["array"].resize( 4 ) + node["user"]["array"][2].setInput( node["user"]["p"] ) + + node["user"]["array"].resize( 1 ) + self.assertEqual( len( node["user"]["array"] ), 1 ) + + def testResizeOutputPlug( self ) : + + array = Gaffer.ArrayPlug( element = Gaffer.IntPlug( direction = Gaffer.Plug.Direction.Out ), direction = Gaffer.Plug.Direction.Out ) + array.resize( 2 ) + def testSerialisationUsesIndices( self ) : s = Gaffer.ScriptNode() diff --git a/src/Gaffer/ArrayPlug.cpp b/src/Gaffer/ArrayPlug.cpp index dd5bced0c3a..18165777072 100644 --- a/src/Gaffer/ArrayPlug.cpp +++ b/src/Gaffer/ArrayPlug.cpp @@ -153,12 +153,13 @@ void ArrayPlug::resize( size_t size ) while( size > children().size() ) { - PlugPtr p = getChild( 0 )->createCounterpart( getChild( 0 )->getName(), Plug::In ); + PlugPtr p = getChild( 0 )->createCounterpart( getChild( 0 )->getName(), direction() ); p->setFlags( Gaffer::Plug::Dynamic, true ); addChild( p ); MetadataAlgo::copyColors( getChild( 0 ) , p.get() , /* overwrite = */ false ); } + Gaffer::Signals::BlockedConnection blockedInputChange( m_inputChangedConnection ); while( children().size() > size ) { removeChild( children().back() ); From f580a6b6fd70f86e4d74344ab89ae9c0d582511e Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 23 Aug 2024 11:25:45 +0100 Subject: [PATCH 02/10] Switch : Fix ArrayPlug minimum size A minimum size of 0 is not currently allowed, and will be silently clamped to 1. But we're about to allow it, and for backwards compatibility we need to keep `Switch` creating arrays of the same size. --- src/Gaffer/Switch.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/Gaffer/Switch.cpp b/src/Gaffer/Switch.cpp index 4d4c1c151ac..702815e5145 100644 --- a/src/Gaffer/Switch.cpp +++ b/src/Gaffer/Switch.cpp @@ -150,7 +150,7 @@ void Switch::setup( const Plug *plug ) g_inPlugsName, Plug::In, inElement, - 0, + 1, std::numeric_limits::max() ); addChild( in ); From 38d25ffcf056c16b37c40534e0d46795839cb0f6 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 23 Aug 2024 10:36:03 +0100 Subject: [PATCH 03/10] CreateViews : Don't serialise internal connections Also remove unused imports from CreateViewsTest. --- Changes.md | 1 + python/GafferImageTest/CreateViewsTest.py | 8 ++++++-- src/GafferImage/CreateViews.cpp | 1 + 3 files changed, 8 insertions(+), 2 deletions(-) diff --git a/Changes.md b/Changes.md index a3da856dc6a..6b3dac95cc9 100644 --- a/Changes.md +++ b/Changes.md @@ -38,6 +38,7 @@ Fixes - ArrayPlug : - Fixed error when `resize()` removed plugs with input connections. - Fixed error when `resize()` was used on an output plug. +- CreateViews : Fixed redundant serialisation of internal connections. API --- diff --git a/python/GafferImageTest/CreateViewsTest.py b/python/GafferImageTest/CreateViewsTest.py index 3db8b0252c5..820f0a4173c 100644 --- a/python/GafferImageTest/CreateViewsTest.py +++ b/python/GafferImageTest/CreateViewsTest.py @@ -37,12 +37,10 @@ import unittest import imath import inspect -import os import IECore import Gaffer -import GafferTest import GafferImage import GafferImageTest @@ -224,5 +222,11 @@ def testInputToExpressionDrivingEnabledPlug( self ) : # for this particular view. self.assertFalse( script["constant"]["enabled"].getValue() ) + def testNoRedundantSerialisation( self ) : + + script = Gaffer.ScriptNode() + script["createViews"] = GafferImage.CreateViews() + self.assertNotIn( "setInput", script.serialise() ) + if __name__ == "__main__": unittest.main() diff --git a/src/GafferImage/CreateViews.cpp b/src/GafferImage/CreateViews.cpp index 88f12474284..f9fabbde361 100644 --- a/src/GafferImage/CreateViews.cpp +++ b/src/GafferImage/CreateViews.cpp @@ -86,6 +86,7 @@ CreateViews::CreateViews( const std::string &name ) s->indexPlug()->setInput( indexPlug() ); ImagePlug *switchOut = runTimeCast< ImagePlug >( s->outPlug() ); + outPlug()->setFlags( Plug::Serialisable, false ); outPlug()->formatPlug()->setInput( switchOut->formatPlug() ); outPlug()->dataWindowPlug()->setInput( switchOut->dataWindowPlug() ); outPlug()->metadataPlug()->setInput( switchOut->metadataPlug() ); From b7edc7a96fd27e40ddd732ee6a5f678b880a81bf Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 23 Aug 2024 12:05:33 +0100 Subject: [PATCH 04/10] ArrayPlug : Improve exception message --- src/Gaffer/ArrayPlug.cpp | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/src/Gaffer/ArrayPlug.cpp b/src/Gaffer/ArrayPlug.cpp index 18165777072..82f39fe8561 100644 --- a/src/Gaffer/ArrayPlug.cpp +++ b/src/Gaffer/ArrayPlug.cpp @@ -148,7 +148,12 @@ void ArrayPlug::resize( size_t size ) { if( size > m_maxSize || size < m_minSize ) { - throw IECore::Exception( "Invalid size" ); + throw IECore::Exception( + fmt::format( + "Invalid size {} requested for `{}` (minSize={}, maxSize={})", + size, fullName(), m_minSize, m_maxSize + ) + ); } while( size > children().size() ) From 6bcff1b57fed20d902b6c7f2bb01cacfa5c28d03 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 23 Aug 2024 12:45:00 +0100 Subject: [PATCH 05/10] ArrayPlug : Allow zero-length arrays We achieve this by explicitly storing a prototype for the elements rather than using the first child as the prototype. At the same time, we require all clients to pass the prototype to the constructor, so that we are always in a valid state following construction. The serialisation now also correctly passes the prototype on construction, and uses `resize()` to add any required children instead of using a series of `addChild()` calls as before. This reflects how we want people to use the API, and makes for a more compact serialisation too. The one wrinkle in this is support for legacy serialisations, which didn't provide an element prototype to the constructor, and used `addChild()` instead of `resize()`. We continue to support these with some special cases internally, whereby `m_elementPrototype` isn't initialised until the first child is added. --- Changes.md | 6 ++ include/Gaffer/ArrayPlug.h | 20 ++-- python/GafferTest/ArrayPlugTest.py | 93 ++++++++++++++++++- .../GafferTest/scripts/arrayPlug-1.4.10.0.gfr | 26 ++++++ src/Gaffer/ArrayPlug.cpp | 91 +++++++++++------- src/GafferModule/ArrayPlugBinding.cpp | 65 +++++++++---- startup/Gaffer/arrayPlugCompatibility.py | 53 +++++++++++ 7 files changed, 295 insertions(+), 59 deletions(-) create mode 100644 python/GafferTest/scripts/arrayPlug-1.4.10.0.gfr create mode 100644 startup/Gaffer/arrayPlugCompatibility.py diff --git a/Changes.md b/Changes.md index 6b3dac95cc9..ad3a0fdf9d2 100644 --- a/Changes.md +++ b/Changes.md @@ -55,6 +55,9 @@ API - PathColumn : - Added `contextMenuSignal()`, allowing the creation of custom context menus. - Added `instanceCreatedSignal()`, providing an opportunity to connect to the signals on _any_ column, no matter how it is created. +- ArrayPlug : + - It is now legal to construct an ArrayPlug with a minimum size of 0. Previously the minimum size was 1. + - Added `elementPrototype()` method. Breaking Changes ---------------- @@ -73,6 +76,9 @@ Breaking Changes - Deprecated `getContext()` methods. Use `context()` instead. - Loop : Removed `nextIterationContext()` method. - NodeGadget, ConnectionGadget : Removed `activeForFocusNode()` virtual methods. Override `updateFromContextTracker()` instead. +- ArrayPlug : + - Renamed `element` constructor argument to `elementPrototype`. + - Deprecated the passing of `element = nullptr` to the constructor. 1.4.x.x (relative to 1.4.11.0) ======= diff --git a/include/Gaffer/ArrayPlug.h b/include/Gaffer/ArrayPlug.h index e5624b4b17a..16b65504283 100644 --- a/include/Gaffer/ArrayPlug.h +++ b/include/Gaffer/ArrayPlug.h @@ -49,17 +49,17 @@ class GAFFER_API ArrayPlug : public Plug public : - /// The element plug is used as the first array element, - /// and all new array elements are created by calling - /// element->createCounterpart(). Currently the element - /// names are derived from the name of the first element, - /// but this may change in the future. It is strongly - /// recommended that ArrayPlug children are only accessed - /// through numeric indexing and never via names. + /// All array elements are created by calling + /// `elementPrototype->createCounterpart()`. Currently the element names + /// are derived from the name of the prototype, but this may change in + /// the future. It is strongly recommended that ArrayPlug children are + /// only accessed through numeric indexing and never via names. explicit ArrayPlug( const std::string &name = defaultName(), Direction direction = In, - PlugPtr element = nullptr, + /// > Caution : `elementPrototype` should not be null. It only defaults + /// > that way to support the loading of legacy serialisations. + ConstPlugPtr elementPrototype = nullptr, size_t minSize = 1, size_t maxSize = std::numeric_limits::max(), unsigned flags = Default, @@ -75,8 +75,10 @@ class GAFFER_API ArrayPlug : public Plug void setInput( PlugPtr input ) override; PlugPtr createCounterpart( const std::string &name, Direction direction ) const override; + const Plug *elementPrototype() const; size_t minSize() const; size_t maxSize() const; + /// Resizes the array. This should be preferred to `addChild()`. void resize( size_t size ); bool resizeWhenInputsChange() const; /// Returns an unconnected element at the end of the array, adding one @@ -91,7 +93,9 @@ class GAFFER_API ArrayPlug : public Plug private : void inputChanged( Gaffer::Plug *plug ); + void childAdded(); + ConstPlugPtr m_elementPrototype; size_t m_minSize; size_t m_maxSize; bool m_resizeWhenInputsChange; diff --git a/python/GafferTest/ArrayPlugTest.py b/python/GafferTest/ArrayPlugTest.py index 9d418db7adf..d9d01d3db53 100644 --- a/python/GafferTest/ArrayPlugTest.py +++ b/python/GafferTest/ArrayPlugTest.py @@ -35,7 +35,7 @@ ########################################################################## import unittest -import gc +import pathlib import imath import IECore @@ -475,6 +475,14 @@ def testNext( self ) : self.assertEqual( n["a2"].next(), None ) + def testNextWithZeroLengthArray( self ) : + + plug = Gaffer.ArrayPlug( elementPrototype = Gaffer.IntPlug(), minSize = 0 ) + element = plug.next() + self.assertIsInstance( element, Gaffer.IntPlug ) + self.assertEqual( len( plug ), 1 ) + self.assertTrue( element.parent().isSame( plug ) ) + def testResize( self ) : p = Gaffer.ArrayPlug( element = Gaffer.IntPlug(), minSize = 1, maxSize = 3, resizeWhenInputsChange = False ) @@ -531,5 +539,88 @@ def testSerialisationUsesIndices( self ) : self.assertEqual( s2["n"]["in"][0].getInput(), s2["a"]["sum"] ) self.assertEqual( s2["n"]["in"][1].getInput(), s2["a"]["sum"] ) + def testCreateCounterpart( self ) : + + p1 = Gaffer.ArrayPlug( "p1", elementPrototype = Gaffer.IntPlug(), minSize = 2, maxSize = 4, resizeWhenInputsChange = False ) + p1.resize( 3 ) + + p2 = p1.createCounterpart( "p2", Gaffer.Plug.Direction.In ) + self.assertEqual( len( p2 ), len( p1 ) ) + self.assertTrue( p1.elementPrototype( _copy = False ).isSame( p2.elementPrototype( _copy = False ) ) ) + + def testZeroLength( self ) : + + s = Gaffer.ScriptNode() + + s["n"] = Gaffer.Node() + s["n"]["user"]["p"] = Gaffer.ArrayPlug( + elementPrototype = Gaffer.IntPlug( "e0" ), minSize = 0, + flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic + ) + + self.assertEqual( len( s["n"]["user"]["p"] ), 0 ) + + s["n"]["user"]["p"].resize( 2 ) + self.assertEqual( len( s["n"]["user"]["p"] ), 2 ) + for element in s["n"]["user"]["p"] : + self.assertIsInstance( element, Gaffer.IntPlug ) + + s["n"]["user"]["p"].resize( 0 ) + self.assertEqual( len( s["n"]["user"]["p"] ), 0 ) + + s["n"]["user"]["p"].resize( 10 ) + self.assertEqual( len( s["n"]["user"]["p"] ), 10 ) + for element in s["n"]["user"]["p"] : + self.assertIsInstance( element, Gaffer.IntPlug ) + + def testZeroLengthSerialisation( self ) : + + s = Gaffer.ScriptNode() + + s["n"] = Gaffer.Node() + s["n"]["user"]["p"] = Gaffer.ArrayPlug( + elementPrototype = Gaffer.IntPlug( "e0" ), minSize = 0, + flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic + ) + + self.assertEqual( len( s["n"]["user"]["p"] ), 0 ) + + s2 = Gaffer.ScriptNode() + s2.execute( s.serialise() ) + + self.assertEqual( len( s2["n"]["user"]["p"] ), 0 ) + + s2["n"]["user"]["p"].resize( 2 ) + self.assertEqual( len( s2["n"]["user"]["p"] ), 2 ) + for element in s2["n"]["user"]["p"] : + self.assertIsInstance( element, Gaffer.IntPlug ) + + def testLoadFromVersion1_4( self ) : + + s = Gaffer.ScriptNode() + s["fileName"].setValue( + pathlib.Path( __file__ ).parent / "scripts" / "arrayPlug-1.4.10.0.gfr" + ) + s.load() + + self.assertEqual( len( s["n"]["user"]["p"] ), 4 ) + for element in s["n"]["user"]["p"] : + self.assertIsInstance( element, Gaffer.IntPlug ) + + self.assertEqual( s["n"]["user"]["p"][0].getValue(), 0 ) + self.assertEqual( s["n"]["user"]["p"][1].getValue(), 1 ) + self.assertTrue( s["n"]["user"]["p"][2].getInput().isSame( s["a"]["sum"] ) ) + self.assertEqual( s["n"]["user"]["p"][3].getValue(), 3 ) + self.assertIsInstance( s["n"]["user"]["p"].elementPrototype(), Gaffer.IntPlug ) + + s["n"]["user"]["p"].resize( 1 ) + self.assertEqual( len( s["n"]["user"]["p"] ), 1 ) + self.assertIsInstance( s["n"]["user"]["p"][0], Gaffer.IntPlug ) + + s["n"]["user"]["p"].resize( 2 ) + self.assertEqual( len( s["n"]["user"]["p"] ), 2 ) + for element in s["n"]["user"]["p"] : + self.assertIsInstance( element, Gaffer.IntPlug ) + if __name__ == "__main__": unittest.main() diff --git a/python/GafferTest/scripts/arrayPlug-1.4.10.0.gfr b/python/GafferTest/scripts/arrayPlug-1.4.10.0.gfr new file mode 100644 index 00000000000..9705ab27367 --- /dev/null +++ b/python/GafferTest/scripts/arrayPlug-1.4.10.0.gfr @@ -0,0 +1,26 @@ +import Gaffer +import GafferTest + +Gaffer.Metadata.registerValue( parent, "serialiser:milestoneVersion", 1, persistent=False ) +Gaffer.Metadata.registerValue( parent, "serialiser:majorVersion", 4, persistent=False ) +Gaffer.Metadata.registerValue( parent, "serialiser:minorVersion", 10, persistent=False ) +Gaffer.Metadata.registerValue( parent, "serialiser:patchVersion", 0, persistent=False ) + +__children = {} + +__children["n"] = Gaffer.Node( "n" ) +parent.addChild( __children["n"] ) +__children["n"]["user"].addChild( Gaffer.ArrayPlug( "p", flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) +__children["n"]["user"]["p"].addChild( Gaffer.IntPlug( "e0", defaultValue = 0, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) +__children["n"]["user"]["p"].addChild( Gaffer.IntPlug( "e1", defaultValue = 0, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) +__children["n"]["user"]["p"].addChild( Gaffer.IntPlug( "e2", defaultValue = 0, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) +__children["n"]["user"]["p"].addChild( Gaffer.IntPlug( "e3", defaultValue = 0, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) +__children["a"] = GafferTest.AddNode( "a" ) +parent.addChild( __children["a"] ) +__children["n"]["user"]["p"][1].setValue( 1 ) +__children["n"]["user"]["p"][2].setInput( __children["a"]["sum"] ) +__children["n"]["user"]["p"][3].setValue( 3 ) + + +del __children + diff --git a/src/Gaffer/ArrayPlug.cpp b/src/Gaffer/ArrayPlug.cpp index 82f39fe8561..5476f46b781 100644 --- a/src/Gaffer/ArrayPlug.cpp +++ b/src/Gaffer/ArrayPlug.cpp @@ -69,27 +69,19 @@ bool hasInput( const Plug *p ) GAFFER_PLUG_DEFINE_TYPE( ArrayPlug ) -ArrayPlug::ArrayPlug( const std::string &name, Direction direction, PlugPtr element, size_t minSize, size_t maxSize, unsigned flags, bool resizeWhenInputsChange ) - : Plug( name, direction, flags ), m_minSize( std::max( minSize, size_t( 1 ) ) ), m_maxSize( std::max( maxSize, m_minSize ) ), m_resizeWhenInputsChange( resizeWhenInputsChange ) +ArrayPlug::ArrayPlug( const std::string &name, Direction direction, ConstPlugPtr elementPrototype, size_t minSize, size_t maxSize, unsigned flags, bool resizeWhenInputsChange ) + : Plug( name, direction, flags ), m_elementPrototype( elementPrototype ), m_minSize( minSize ), m_maxSize( std::max( maxSize, m_minSize ) ), m_resizeWhenInputsChange( resizeWhenInputsChange ) { - if( element ) + if( !m_elementPrototype ) { - // If we're dynamic ourselves, then serialisations will include a constructor - // for us, but it will have element==None. In this case we make sure the first - // element is dynamic, so that it too will have a constructor written out. But - // if we're not dynamic, we expect to be passed the element again upon reconstruction, - // so we don't need a constructor to be serialised for the element, and therefore - // we must set it to be non-dynamic. - element->setFlags( Gaffer::Plug::Dynamic, getFlags( Gaffer::Plug::Dynamic ) ); - addChild( element ); - - for( size_t i = 1; i < m_minSize; ++i ) - { - PlugPtr p = element->createCounterpart( element->getName(), Plug::In ); - addChild( p ); - MetadataAlgo::copyColors( element.get() , p.get() , /* overwrite = */ false ); - } + // We're being constructed during execution of a legacy serialisation + // (nobody else is allowed to pass a null `elementPrototype`). Arrange + // to recover our protoype when the first element is added. + childAddedSignal().connect( boost::bind( &ArrayPlug::childAdded, this ) ); + return; } + + resize( m_minSize ); } ArrayPlug::~ArrayPlug() @@ -103,7 +95,16 @@ bool ArrayPlug::acceptsChild( const GraphComponent *potentialChild ) const return false; } - return children().size() == 0 || potentialChild->typeId() == children()[0]->typeId(); + if( !m_elementPrototype ) + { + // Special case to support loading of legacy serialisations. We accept + // the first child we are given and then in `childAdded()` we initialise + // `m_elementPrototype` from it. + assert( children().size() == 0 ); + return true; + } + + return potentialChild->typeId() == m_elementPrototype->typeId(); } bool ArrayPlug::acceptsInput( const Plug *input ) const @@ -126,14 +127,19 @@ void ArrayPlug::setInput( PlugPtr input ) PlugPtr ArrayPlug::createCounterpart( const std::string &name, Direction direction ) const { - ArrayPlugPtr result = new ArrayPlug( name, direction, nullptr, m_minSize, m_maxSize, getFlags(), resizeWhenInputsChange() ); - for( Plug::Iterator it( this ); !it.done(); ++it ) + ArrayPlugPtr result = new ArrayPlug( name, direction, m_elementPrototype, m_minSize, m_maxSize, getFlags(), resizeWhenInputsChange() ); + if( m_elementPrototype ) { - result->addChild( (*it)->createCounterpart( (*it)->getName(), direction ) ); + result->resize( children().size() ); } return result; } +const Plug *ArrayPlug::elementPrototype() const +{ + return m_elementPrototype.get(); +} + size_t ArrayPlug::minSize() const { return m_minSize; @@ -156,12 +162,21 @@ void ArrayPlug::resize( size_t size ) ); } + if( !m_elementPrototype ) + { + throw IECore::Exception( + fmt::format( + "ArrayPlug `{}` was constructed without the required `elementPrototype`", + fullName() + ) + ); + } + while( size > children().size() ) { - PlugPtr p = getChild( 0 )->createCounterpart( getChild( 0 )->getName(), direction() ); - p->setFlags( Gaffer::Plug::Dynamic, true ); + PlugPtr p = m_elementPrototype->createCounterpart( m_elementPrototype->getName(), direction() ); addChild( p ); - MetadataAlgo::copyColors( getChild( 0 ) , p.get() , /* overwrite = */ false ); + MetadataAlgo::copyColors( m_elementPrototype.get(), p.get() , /* overwrite = */ false ); } Gaffer::Signals::BlockedConnection blockedInputChange( m_inputChangedConnection ); @@ -178,10 +193,13 @@ bool ArrayPlug::resizeWhenInputsChange() const Gaffer::Plug *ArrayPlug::next() { - Plug *last = static_cast( children().back().get() ); - if( !hasInput( last ) ) + if( children().size() ) { - return last; + Plug *last = static_cast( children().back().get() ); + if( !hasInput( last ) ) + { + return last; + } } if( children().size() >= m_maxSize ) @@ -189,11 +207,8 @@ Gaffer::Plug *ArrayPlug::next() return nullptr; } - PlugPtr p = getChild( 0 )->createCounterpart( getChild( 0 )->getName(), Plug::In ); - p->setFlags( Gaffer::Plug::Dynamic, true ); - addChild( p ); - MetadataAlgo::copyColors( getChild( 0 ) , p.get() , /* overwrite = */ false ); - return p.get(); + resize( children().size() + 1 ); + return static_cast( children().back().get() ); } void ArrayPlug::parentChanged( GraphComponent *oldParent ) @@ -265,3 +280,13 @@ void ArrayPlug::inputChanged( Gaffer::Plug *plug ) } } } + +void ArrayPlug::childAdded() +{ + if( !m_elementPrototype ) + { + // First child being added from a legacy serialisation. Initialise prototype. + const Plug *firstElement = getChild( 0 ); + m_elementPrototype = firstElement->createCounterpart( firstElement->getName(), firstElement->direction() ); + } +} diff --git a/src/GafferModule/ArrayPlugBinding.cpp b/src/GafferModule/ArrayPlugBinding.cpp index dbb73eb3fcd..d5e16ff0a8a 100644 --- a/src/GafferModule/ArrayPlugBinding.cpp +++ b/src/GafferModule/ArrayPlugBinding.cpp @@ -54,7 +54,7 @@ using namespace Gaffer; namespace { -std::string repr( const ArrayPlug *plug ) +std::string constructor( const ArrayPlug *plug, Serialisation *serialisation = nullptr ) { std::string result = Serialisation::classPath( plug ) + "( \"" + plug->getName().string() + "\", "; @@ -63,6 +63,12 @@ std::string repr( const ArrayPlug *plug ) result += "direction = " + PlugSerialiser::directionRepr( plug->direction() ) + ", "; } + if( serialisation && plug->elementPrototype() ) + { + const Serialisation::Serialiser *plugSerialiser = Serialisation::acquireSerialiser( plug->elementPrototype() ); + result += fmt::format( "elementPrototype = {}, ", plugSerialiser->constructor( plug->elementPrototype(), *serialisation ) ); + } + if( plug->minSize() != 1 ) { result += fmt::format( "minSize = {}, ", plug->minSize() ); @@ -90,6 +96,11 @@ std::string repr( const ArrayPlug *plug ) } +std::string repr( const ArrayPlug &plug ) +{ + return constructor( &plug ); +} + class ArrayPlugSerialiser : public PlugSerialiser { @@ -97,29 +108,48 @@ class ArrayPlugSerialiser : public PlugSerialiser bool childNeedsConstruction( const Gaffer::GraphComponent *child, const Serialisation &serialisation ) const override { - auto arrayPlug = static_cast( child->parent() ); - if( arrayPlug->direction() == Plug::Out && arrayPlug->parent() ) - { - // BoxIO serialisation is different than most nodes, in that - // the internal connection is made _before_ children are added - // to the input ArrayPlug. The existence of the connection means - // that when a child is added to the input plug, an equivalent - // child is added to the output plug automatically. Hence we don't - // need to serialise an explicit constructor for this child. - /// \todo Figure out how we can do this cleanly, without the - /// ArrayPlugSerialiser needing knowledge of BoxIO. - return false; - } - return PlugSerialiser::childNeedsConstruction( child, serialisation ); + // We'll call `resize()` in our `postConstructor()` to create all + // the child elements. + return false; } std::string constructor( const Gaffer::GraphComponent *graphComponent, Serialisation &serialisation ) const override { - return ::repr( static_cast( graphComponent ) ); + return ::constructor( static_cast( graphComponent ), &serialisation ); + } + + std::string postConstructor( const Gaffer::GraphComponent *graphComponent, const std::string &identifier, Serialisation &serialisation ) const override + { + std::string result = PlugSerialiser::postConstructor( graphComponent, identifier, serialisation ); + + auto arrayPlug = static_cast( graphComponent ); + if( arrayPlug->children().size() != arrayPlug->minSize() ) + { + if( result.size() ) + { + result += "\n"; + } + + result += fmt::format( "{}.resize( {} )\n", identifier, arrayPlug->children().size() ); + } + + return result; } }; +PlugPtr elementPrototype( ArrayPlug &p, bool copy ) +{ + // By default we copy, because allowing an unsuspecting Python + // user to modify the prototype would lead to arrays with inconsistent + // elements. We're protected by `const` in C++ but not so in Python. + if( !copy || !p.elementPrototype() ) + { + return const_cast( p.elementPrototype() ); + } + return p.elementPrototype()->createCounterpart( p.elementPrototype()->getName(), p.elementPrototype()->direction() ); +} + void resize( ArrayPlug &p, size_t size ) { IECorePython::ScopedGILRelease gilRelease; @@ -142,7 +172,7 @@ void GafferModule::bindArrayPlug() ( arg( "name" ) = GraphComponent::defaultName(), arg( "direction" ) = Plug::In, - arg( "element" ) = PlugPtr(), + arg( "elementPrototype" ) = PlugPtr(), arg( "minSize" ) = 1, arg( "maxSize" ) = std::numeric_limits::max(), arg( "flags" ) = Plug::Default, @@ -150,6 +180,7 @@ void GafferModule::bindArrayPlug() ) ) ) + .def( "elementPrototype", &elementPrototype, ( arg( "_copy" ) = true ) ) .def( "minSize", &ArrayPlug::minSize ) .def( "maxSize", &ArrayPlug::maxSize ) .def( "resize", &resize ) diff --git a/startup/Gaffer/arrayPlugCompatibility.py b/startup/Gaffer/arrayPlugCompatibility.py new file mode 100644 index 00000000000..c7e16cd3796 --- /dev/null +++ b/startup/Gaffer/arrayPlugCompatibility.py @@ -0,0 +1,53 @@ +########################################################################## +# +# Copyright (c) 2024, Cinesite VFX Ltd. All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above +# copyright notice, this list of conditions and the following +# disclaimer. +# +# * Redistributions in binary form must reproduce the above +# copyright notice, this list of conditions and the following +# disclaimer in the documentation and/or other materials provided with +# the distribution. +# +# * Neither the name of John Haddon nor the names of +# any other contributors to this software may be used to endorse or +# promote products derived from this software without specific prior +# written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS +# IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR +# PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, +# EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, +# PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR +# PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF +# LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING +# NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS +# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +# +########################################################################## + +import Gaffer + +def __initWrapper( originalInit ) : + + def init( self, *args, **kw ) : + + if "element" in kw : + + # We renamed `element` to `elementPrototype` in Gaffer 1.5. + kw["elementPrototype"] = kw["element"] + del kw["element"] + + originalInit( self, *args, **kw ) + + return init + +Gaffer.ArrayPlug.__init__ = __initWrapper( Gaffer.ArrayPlug.__init__ ) From 02d9fccde24c99703d7785e4833e0f714ce54c6e Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 23 Aug 2024 13:00:16 +0100 Subject: [PATCH 06/10] BoxIO/PlugAlgo : Remove special case for ArrayPlug `ArrayPlugSerialiser::childNeedsConstruction()` return false now, so the existence of the Dynamic flag on its children is irrelevant. We're one step closer to eventually getting rid of the flag entirely. --- src/Gaffer/BoxIO.cpp | 2 +- src/Gaffer/PlugAlgo.cpp | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/src/Gaffer/BoxIO.cpp b/src/Gaffer/BoxIO.cpp index 56d4f8c1767..4605cfe135f 100644 --- a/src/Gaffer/BoxIO.cpp +++ b/src/Gaffer/BoxIO.cpp @@ -125,7 +125,7 @@ void applyDynamicFlag( Plug *plug ) { plug->setFlags( Plug::Dynamic, true ); - auto compoundTypes = { PlugTypeId, ValuePlugTypeId, ArrayPlugTypeId }; + auto compoundTypes = { PlugTypeId, ValuePlugTypeId }; if( find( begin( compoundTypes ), end( compoundTypes ), (Gaffer::TypeId)plug->typeId() ) != end( compoundTypes ) ) { for( Plug::RecursiveIterator it( plug ); !it.done(); ++it ) diff --git a/src/Gaffer/PlugAlgo.cpp b/src/Gaffer/PlugAlgo.cpp index 9bfc3ba3ebc..c73932b40f6 100644 --- a/src/Gaffer/PlugAlgo.cpp +++ b/src/Gaffer/PlugAlgo.cpp @@ -1436,7 +1436,7 @@ void applyDynamicFlag( Plug *plug ) // for types like CompoundNumericPlug that create children in their constructors. // Or, even better, abolish the Dynamic flag entirely and deal with everything // via serialisers. - std::array compoundTypes = { PlugTypeId, ValuePlugTypeId, ArrayPlugTypeId, CompoundDataPlugTypeId }; + std::array compoundTypes = { PlugTypeId, ValuePlugTypeId, CompoundDataPlugTypeId }; if( find( compoundTypes.begin(), compoundTypes.end(), (Gaffer::TypeId)plug->typeId() ) != compoundTypes.end() ) { for( Plug::RecursiveIterator it( plug ); !it.done(); ++it ) From 83f865baaa795ff16fbc84a72595e94af995f99a Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 23 Aug 2024 13:05:28 +0100 Subject: [PATCH 07/10] ArrayPlug clients : Rename `element` to `elementPrototype` Our `startup/Gaffer/arrayPlugCompatibility.py` shim was keeping these working, but we want to provide a good example. --- python/GafferDispatchTest/DebugDispatcher.py | 2 +- python/GafferSceneUI/SceneEditor.py | 2 +- python/GafferTest/ArrayPlugNode.py | 2 +- python/GafferTest/ArrayPlugTest.py | 28 ++++++++++---------- python/GafferTest/DotTest.py | 4 +-- python/GafferTest/ReferenceTest.py | 2 +- python/GafferTest/SwitchTest.py | 6 ++--- 7 files changed, 23 insertions(+), 23 deletions(-) diff --git a/python/GafferDispatchTest/DebugDispatcher.py b/python/GafferDispatchTest/DebugDispatcher.py index 02657a3287b..a67a8ee5f86 100644 --- a/python/GafferDispatchTest/DebugDispatcher.py +++ b/python/GafferDispatchTest/DebugDispatcher.py @@ -111,7 +111,7 @@ def __createNode( name ) : node = Gaffer.Node() node.setName( name.replace( ".", "_" ) ) - node["preTasks"] = Gaffer.ArrayPlug( element = Gaffer.Plug( "preTask0" ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) + node["preTasks"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.Plug( "preTask0" ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) node["task"] = Gaffer.Plug( direction = Gaffer.Plug.Direction.Out, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) node["node"] = Gaffer.StringPlug( flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) node["frames"] = Gaffer.StringPlug( flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) diff --git a/python/GafferSceneUI/SceneEditor.py b/python/GafferSceneUI/SceneEditor.py index 0a957f4e8b3..6e4cea1f6ea 100644 --- a/python/GafferSceneUI/SceneEditor.py +++ b/python/GafferSceneUI/SceneEditor.py @@ -55,7 +55,7 @@ def __init__( self, numInputs = 1 ) : if numInputs == 1 : self["in"] = GafferScene.ScenePlug() else : - self["in"] = Gaffer.ArrayPlug( element = GafferScene.ScenePlug(), minSize = numInputs, maxSize = numInputs ) + self["in"] = Gaffer.ArrayPlug( elementPrototype = GafferScene.ScenePlug(), minSize = numInputs, maxSize = numInputs ) IECore.registerRunTimeTyped( Settings, typeName = "GafferSceneUI::SceneEditor::Settings" ) diff --git a/python/GafferTest/ArrayPlugNode.py b/python/GafferTest/ArrayPlugNode.py index b940d01e278..9f671d29bae 100644 --- a/python/GafferTest/ArrayPlugNode.py +++ b/python/GafferTest/ArrayPlugNode.py @@ -47,7 +47,7 @@ def __init__( self, name = "ArrayPlugNode" ) : self.addChild( Gaffer.ArrayPlug( "in", - element = Gaffer.IntPlug( "e1", minValue=0, maxValue=10 ), + elementPrototype = Gaffer.IntPlug( "e1", minValue=0, maxValue=10 ), maxSize = 6 ) ) diff --git a/python/GafferTest/ArrayPlugTest.py b/python/GafferTest/ArrayPlugTest.py index d9d01d3db53..599031e67b4 100644 --- a/python/GafferTest/ArrayPlugTest.py +++ b/python/GafferTest/ArrayPlugTest.py @@ -187,7 +187,7 @@ def testMinimumInputs( self ) : a = GafferTest.AddNode() n = Gaffer.Node() - n["in"] = Gaffer.ArrayPlug( "in", element = Gaffer.IntPlug( "e1" ), minSize=3 ) + n["in"] = Gaffer.ArrayPlug( "in", elementPrototype = Gaffer.IntPlug( "e1" ), minSize=3 ) self.assertEqual( len( n["in"] ), 3 ) @@ -291,7 +291,7 @@ def testFixedLengthDynamic( self ) : s["a"] = GafferTest.AddNode() s["n"] = Gaffer.Node() - s["n"]["a"] = Gaffer.ArrayPlug( "a", element = Gaffer.IntPlug(), minSize = 4, maxSize = 4, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) + s["n"]["a"] = Gaffer.ArrayPlug( "a", elementPrototype = Gaffer.IntPlug(), minSize = 4, maxSize = 4, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) s["n"]["a"][1].setInput( s["a"]["sum"] ) s["n"]["a"][2].setInput( s["a"]["sum"] ) @@ -327,7 +327,7 @@ def createCounterpart( self, name, direction ) : return PythonElement( name, direction, self.getFlags() ) n = Gaffer.Node() - n["a"] = Gaffer.ArrayPlug( element = PythonElement() ) + n["a"] = Gaffer.ArrayPlug( elementPrototype = PythonElement() ) self.assertEqual( len( n["a"] ), 1 ) self.assertTrue( isinstance( n["a"][0], PythonElement ) ) @@ -342,8 +342,8 @@ def testTopLevelConnection( self ) : n = Gaffer.Node() - n["a"] = Gaffer.ArrayPlug( element = Gaffer.IntPlug() ) - n["b"] = Gaffer.ArrayPlug( element = Gaffer.IntPlug() ) + n["a"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.IntPlug() ) + n["b"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.IntPlug() ) n["b"].setInput( n["a"] ) def assertInput( plug, input ) : @@ -383,7 +383,7 @@ def testArrayPlugCopiesColors( self ) : Gaffer.Metadata.registerValue( element, "connectionGadget:color", connectionColor ) Gaffer.Metadata.registerValue( element, "nodule:color", noodleColor ) - n["a"] = Gaffer.ArrayPlug( element = element ) + n["a"] = Gaffer.ArrayPlug( elementPrototype = element ) n["a"][0].setInput(n2["test"]) self.assertEqual( Gaffer.Metadata.value( n["a"][1], "connectionGadget:color" ), connectionColor ) @@ -391,20 +391,20 @@ def testArrayPlugCopiesColors( self ) : def testOnlyOneChildType( self ) : - p = Gaffer.ArrayPlug( element = Gaffer.IntPlug() ) + p = Gaffer.ArrayPlug( elementPrototype = Gaffer.IntPlug() ) self.assertTrue( p.acceptsChild( Gaffer.IntPlug() ) ) self.assertFalse( p.acceptsChild( Gaffer.FloatPlug() ) ) def testDenyInputFromNonArrayPlugs( self ) : - a = Gaffer.ArrayPlug( element = Gaffer.IntPlug() ) + a = Gaffer.ArrayPlug( elementPrototype = Gaffer.IntPlug() ) p = Gaffer.V2iPlug() self.assertFalse( a.acceptsInput( p ) ) def testPartialConnections( self ) : n = Gaffer.Node() - n["p"] = Gaffer.ArrayPlug( element = Gaffer.V3fPlug( "e" ) ) + n["p"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.V3fPlug( "e" ) ) self.assertEqual( len( n["p"] ), 1 ) p = Gaffer.FloatPlug() @@ -432,7 +432,7 @@ def testResizeWhenInputsChange( self ) : s["a"] = GafferTest.AddNode() s["n"] = Gaffer.Node() - s["n"]["user"]["p"] = Gaffer.ArrayPlug( element = Gaffer.IntPlug(), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, resizeWhenInputsChange = False ) + s["n"]["user"]["p"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.IntPlug(), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, resizeWhenInputsChange = False ) self.assertEqual( s["n"]["user"]["p"].resizeWhenInputsChange(), False ) self.assertEqual( len( s["n"]["user"]["p"] ), 1 ) @@ -449,8 +449,8 @@ def testNext( self ) : a = GafferTest.AddNode() n = Gaffer.Node() - n["a1"] = Gaffer.ArrayPlug( element = Gaffer.IntPlug() ) - n["a2"] = Gaffer.ArrayPlug( element = Gaffer.IntPlug(), maxSize = 3, resizeWhenInputsChange = False ) + n["a1"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.IntPlug() ) + n["a2"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.IntPlug(), maxSize = 3, resizeWhenInputsChange = False ) self.assertEqual( len( n["a1"] ), 1 ) self.assertEqual( len( n["a2"] ), 1 ) @@ -485,7 +485,7 @@ def testNextWithZeroLengthArray( self ) : def testResize( self ) : - p = Gaffer.ArrayPlug( element = Gaffer.IntPlug(), minSize = 1, maxSize = 3, resizeWhenInputsChange = False ) + p = Gaffer.ArrayPlug( elementPrototype = Gaffer.IntPlug(), minSize = 1, maxSize = 3, resizeWhenInputsChange = False ) self.assertEqual( len( p ), p.minSize() ) p.resize( 2 ) @@ -506,7 +506,7 @@ def testRemoveInputDuringResize( self ) : node = Gaffer.Node() node["user"]["p"] = Gaffer.IntPlug() - node["user"]["array"] = Gaffer.ArrayPlug( element = Gaffer.IntPlug(), resizeWhenInputsChange = True ) + node["user"]["array"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.IntPlug(), resizeWhenInputsChange = True ) node["user"]["array"].resize( 4 ) node["user"]["array"][2].setInput( node["user"]["p"] ) diff --git a/python/GafferTest/DotTest.py b/python/GafferTest/DotTest.py index 3a7376eabc7..2c2aecf34b8 100644 --- a/python/GafferTest/DotTest.py +++ b/python/GafferTest/DotTest.py @@ -143,10 +143,10 @@ def testDeletion( self ) : def testArrayPlug( self ) : n1 = Gaffer.Node() - n1["a"] = Gaffer.ArrayPlug( element = Gaffer.IntPlug() ) + n1["a"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.IntPlug() ) n2 = Gaffer.Node() - n2["a"] = Gaffer.ArrayPlug( element = Gaffer.IntPlug() ) + n2["a"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.IntPlug() ) d = Gaffer.Dot() d.setup( n1["a"] ) diff --git a/python/GafferTest/ReferenceTest.py b/python/GafferTest/ReferenceTest.py index dd6aa200613..402caf704d6 100644 --- a/python/GafferTest/ReferenceTest.py +++ b/python/GafferTest/ReferenceTest.py @@ -971,7 +971,7 @@ def testReloadWithNestedInputConnections( self ) : s = Gaffer.ScriptNode() s["b"] = Gaffer.Box() - s["b"]["array"] = Gaffer.ArrayPlug( element = Gaffer.IntPlug(), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) + s["b"]["array"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.IntPlug(), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) s["b"]["color"] = Gaffer.Color3fPlug( flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) s["b"].exportForReference( self.temporaryDirectory() / "test.grf" ) diff --git a/python/GafferTest/SwitchTest.py b/python/GafferTest/SwitchTest.py index 808d18a043d..252fe2889af 100644 --- a/python/GafferTest/SwitchTest.py +++ b/python/GafferTest/SwitchTest.py @@ -262,7 +262,7 @@ def testIndexExpression( self ) : def testPassThroughWhenIndexConstant( self ) : n = Gaffer.Switch() - n["in"] = Gaffer.ArrayPlug( element = Gaffer.Plug(), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) + n["in"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.Plug(), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) n["out"] = Gaffer.Plug( direction = Gaffer.Plug.Direction.Out, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) self.assertTrue( n["out"].source().isSame( n["in"][0] ) ) @@ -309,7 +309,7 @@ def testIndexInputAcceptance( self ) : def testConnectedIndex( self ) : n = Gaffer.Switch() - n["in"] = Gaffer.ArrayPlug( element = Gaffer.Plug(), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) + n["in"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.Plug(), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) n["out"] = Gaffer.Plug( direction = Gaffer.Plug.Direction.Out, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) input0 = Gaffer.Plug() @@ -344,7 +344,7 @@ def testAcceptsNoneInputs( self ) : def testIndirectInputsToIndex( self ) : n = Gaffer.Switch() - n["in"] = Gaffer.ArrayPlug( element = Gaffer.Plug(), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) + n["in"] = Gaffer.ArrayPlug( elementPrototype = Gaffer.Plug(), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) n["out"] = Gaffer.Plug( direction = Gaffer.Plug.Direction.Out, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) input0 = Gaffer.Plug() From 24d6d93e88e20132c93a8bf7820e6e7c5d53b676 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 23 Aug 2024 16:51:04 +0100 Subject: [PATCH 08/10] CreateViews : Provide `elementPrototype` to ArrayPlug Passing `nullptr` is no longer allowed except to support the loading of legacy files. Unfortunately, all client code that is adding views is doing it via `addChild( NameValuePlug( ... ) )` rather than `resize()` or `next()`, meaning the elements are not forced to match the prototype exactly. And all existing serialisations are doing the same. And because `NameValuePlug( "left", ... )` produces a plug with a default value of "left", the array elements being created are not homogeneous. That breaks the new ArrayPlug serialisation, which assumes that a simple `resize()` is sufficient to recreate all children. We deal with this using a compatibility shim that detects the bad child additions and resets the default value while maintaining the current value. --- .../scripts/createViews-1.4.10.0.gfr | 51 +++++++++++++++ src/GafferImage/CreateViews.cpp | 7 ++- .../GafferImage/createViewsCompatibility.py | 62 +++++++++++++++++++ 3 files changed, 119 insertions(+), 1 deletion(-) create mode 100644 python/GafferImageTest/scripts/createViews-1.4.10.0.gfr create mode 100644 startup/GafferImage/createViewsCompatibility.py diff --git a/python/GafferImageTest/scripts/createViews-1.4.10.0.gfr b/python/GafferImageTest/scripts/createViews-1.4.10.0.gfr new file mode 100644 index 00000000000..27194c71700 --- /dev/null +++ b/python/GafferImageTest/scripts/createViews-1.4.10.0.gfr @@ -0,0 +1,51 @@ +import Gaffer +import GafferImage +import imath + +Gaffer.Metadata.registerValue( parent, "serialiser:milestoneVersion", 1, persistent=False ) +Gaffer.Metadata.registerValue( parent, "serialiser:majorVersion", 4, persistent=False ) +Gaffer.Metadata.registerValue( parent, "serialiser:minorVersion", 10, persistent=False ) +Gaffer.Metadata.registerValue( parent, "serialiser:patchVersion", 0, persistent=False ) + +__children = {} + +parent["variables"].addChild( Gaffer.NameValuePlug( "image:catalogue:port", Gaffer.IntPlug( "value", defaultValue = 0, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ), "imageCataloguePort", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) +parent["variables"].addChild( Gaffer.NameValuePlug( "project:name", Gaffer.StringPlug( "value", defaultValue = 'default', flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ), "projectName", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) +parent["variables"].addChild( Gaffer.NameValuePlug( "project:rootDirectory", Gaffer.StringPlug( "value", defaultValue = '$HOME/gaffer/projects/${project:name}', flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ), "projectRootDirectory", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) +__children["openColorIO"] = GafferImage.OpenColorIOConfigPlug( "openColorIO", flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) +parent.addChild( __children["openColorIO"] ) +__children["defaultFormat"] = GafferImage.FormatPlug( "defaultFormat", defaultValue = GafferImage.Format( 1920, 1080, 1.000 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) +parent.addChild( __children["defaultFormat"] ) +__children["CheckerboardLeft"] = GafferImage.Checkerboard( "CheckerboardLeft" ) +parent.addChild( __children["CheckerboardLeft"] ) +__children["CheckerboardLeft"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) +__children["CreateViews"] = GafferImage.CreateViews( "CreateViews" ) +parent.addChild( __children["CreateViews"] ) +__children["CreateViews"]["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug( "value", ), True, "view0", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) +__children["CreateViews"]["views"].addChild( Gaffer.NameValuePlug( "right", GafferImage.ImagePlug( "value", ), True, "view1", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) +__children["CreateViews"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) +__children["CheckerboardRight"] = GafferImage.Checkerboard( "CheckerboardRight" ) +parent.addChild( __children["CheckerboardRight"] ) +__children["CheckerboardRight"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) +parent["variables"]["imageCataloguePort"]["value"].setValue( 37035 ) +Gaffer.Metadata.registerValue( parent["variables"]["imageCataloguePort"], 'readOnly', True ) +Gaffer.Metadata.registerValue( parent["variables"]["projectName"]["name"], 'readOnly', True ) +Gaffer.Metadata.registerValue( parent["variables"]["projectRootDirectory"]["name"], 'readOnly', True ) +__children["CheckerboardLeft"]["size"]["y"].setInput( __children["CheckerboardLeft"]["size"]["x"] ) +__children["CheckerboardLeft"]["__uiPosition"].setValue( imath.V2f( -7.31522799, 2.82158232 ) ) +__children["CreateViews"]["out"]["format"].setInput( __children["CreateViews"]["__switch"]["out"]["format"] ) +__children["CreateViews"]["out"]["dataWindow"].setInput( __children["CreateViews"]["__switch"]["out"]["dataWindow"] ) +__children["CreateViews"]["out"]["metadata"].setInput( __children["CreateViews"]["__switch"]["out"]["metadata"] ) +__children["CreateViews"]["out"]["deep"].setInput( __children["CreateViews"]["__switch"]["out"]["deep"] ) +__children["CreateViews"]["out"]["sampleOffsets"].setInput( __children["CreateViews"]["__switch"]["out"]["sampleOffsets"] ) +__children["CreateViews"]["out"]["channelNames"].setInput( __children["CreateViews"]["__switch"]["out"]["channelNames"] ) +__children["CreateViews"]["out"]["channelData"].setInput( __children["CreateViews"]["__switch"]["out"]["channelData"] ) +__children["CreateViews"]["views"][0]["value"].setInput( __children["CheckerboardLeft"]["out"] ) +__children["CreateViews"]["views"][1]["value"].setInput( __children["CheckerboardRight"]["out"] ) +__children["CreateViews"]["__uiPosition"].setValue( imath.V2f( 0.366687864, -5.34316444 ) ) +__children["CheckerboardRight"]["size"]["y"].setInput( __children["CheckerboardRight"]["size"]["x"] ) +__children["CheckerboardRight"]["__uiPosition"].setValue( imath.V2f( 8.04854202, 2.82158256 ) ) + + +del __children + diff --git a/src/GafferImage/CreateViews.cpp b/src/GafferImage/CreateViews.cpp index f9fabbde361..df048333544 100644 --- a/src/GafferImage/CreateViews.cpp +++ b/src/GafferImage/CreateViews.cpp @@ -68,7 +68,12 @@ CreateViews::CreateViews( const std::string &name ) ArrayPlugPtr views = new ArrayPlug( "views", Plug::In, - nullptr, + new NameValuePlug( + /* nameDefault = */ "", + /* valuePlug = */ new ImagePlug(), + /* defaultEnabled = */ true, + /* name = */ "view0" + ), 0, std::numeric_limits::max(), Plug::Default, diff --git a/startup/GafferImage/createViewsCompatibility.py b/startup/GafferImage/createViewsCompatibility.py new file mode 100644 index 00000000000..8fc6afd1715 --- /dev/null +++ b/startup/GafferImage/createViewsCompatibility.py @@ -0,0 +1,62 @@ +########################################################################## +# +# Copyright (c) 2024, Cinesite VFX Ltd. All rights reserved. +# +# Redistribution and use in source and binary forms, with or without +# modification, are permitted provided that the following conditions are +# met: +# +# * Redistributions of source code must retain the above +# copyright notice, this list of conditions and the following +# disclaimer. +# +# * Redistributions in binary form must reproduce the above +# copyright notice, this list of conditions and the following +# disclaimer in the documentation and/or other materials provided with +# the distribution. +# +# * Neither the name of John Haddon nor the names of +# any other contributors to this software may be used to endorse or +# promote products derived from this software without specific prior +# written permission. +# +# THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND CONTRIBUTORS "AS +# IS" AND ANY EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, +# THE IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR +# PURPOSE ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR +# CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL, +# EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO, +# PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR +# PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY OF +# LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT (INCLUDING +# NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE OF THIS +# SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. +# +########################################################################## + +import GafferImage + +def __viewsChildAdded( parent, child ) : + + if child["name"].defaultValue() : + # Legacy code and files create children with non-empty default values + # in the `name`` plug. This violates the uniformity of the array, and + # can no longer be serialised. Reset the default value while preserving + # the current value. + value = child["name"].getValue() + child["name"].setValue( "" ) + child["name"].resetDefault() + child["name"].setValue( value ) + +def __initWrapper( originalInit ) : + + def init( self, *args, **kw ) : + + originalInit( self, *args, **kw ) + self["views"].childAddedSignal().connect( + __viewsChildAdded, scoped = False + ) + + return init + +GafferImage.CreateViews.__init__ = __initWrapper( GafferImage.CreateViews.__init__ ) From 5904d3540dc9daa9d9235089037a8f7378097554 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 23 Aug 2024 17:00:27 +0100 Subject: [PATCH 09/10] CreateViews clients : Don't violate ArrayPlug constraints --- python/GafferImageTest/AnaglyphTest.py | 5 ++- python/GafferImageTest/CatalogueTest.py | 5 ++- .../GafferImageTest/ContactSheetCoreTest.py | 3 +- python/GafferImageTest/CopyViewsTest.py | 5 ++- python/GafferImageTest/CreateViewsTest.py | 38 +++++++++++++++---- python/GafferImageTest/DeleteViewsTest.py | 7 ++-- python/GafferImageTest/FormatQueryTest.py | 5 ++- python/GafferImageTest/ImageSamplerTest.py | 5 ++- python/GafferImageTest/ImageStatsTest.py | 5 ++- python/GafferImageTest/ImageTestCase.py | 5 ++- python/GafferImageTest/MergeTest.py | 10 +++-- python/GafferImageTest/SelectViewTest.py | 7 ++-- python/GafferImageUI/CreateViewsUI.py | 6 +-- 13 files changed, 71 insertions(+), 35 deletions(-) diff --git a/python/GafferImageTest/AnaglyphTest.py b/python/GafferImageTest/AnaglyphTest.py index 7421460328a..f001c76addb 100644 --- a/python/GafferImageTest/AnaglyphTest.py +++ b/python/GafferImageTest/AnaglyphTest.py @@ -67,10 +67,11 @@ def test( self ) : right['transform']['translate'].setValue( imath.V2f( 10, 0 ) ) createViews = GafferImage.CreateViews() - createViews["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug(), True, "view0", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - createViews["views"].addChild( Gaffer.NameValuePlug( "right", GafferImage.ImagePlug(), True, "view1", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) + createViews["views"].resize( 2 ) createViews["views"][0]["value"].setInput( left["out"] ) + createViews["views"][0]["name"].setValue( "left" ) createViews["views"][1]["value"].setInput( right["out"] ) + createViews["views"][1]["name"].setValue( "right" ) anaglyph = GafferImage.Anaglyph() anaglyph["in"].setInput( createViews["out"] ) diff --git a/python/GafferImageTest/CatalogueTest.py b/python/GafferImageTest/CatalogueTest.py index 01c791d0ac2..d71b5151237 100644 --- a/python/GafferImageTest/CatalogueTest.py +++ b/python/GafferImageTest/CatalogueTest.py @@ -917,10 +917,11 @@ def testGenerateFileName( self ): # Check that two multi-view images match only if all views are identical createViews = GafferImage.CreateViews() - createViews["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug(), True, "view0", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - createViews["views"].addChild( Gaffer.NameValuePlug( "right", GafferImage.ImagePlug(), True, "view1", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) + createViews["views"].resize( 2 ) createViews["views"][0]["value"].setInput( constant1["out"] ) + createViews["views"][0]["name"].setValue( "left" ) createViews["views"][1]["value"].setInput( constant2["out"] ) + createViews["views"][1]["name"].setValue( "right" ) f3 = catalogue.generateFileName( createViews["out"] ) self.assertNotIn( f3, [f1, f2] ) diff --git a/python/GafferImageTest/ContactSheetCoreTest.py b/python/GafferImageTest/ContactSheetCoreTest.py index 479ba9f81ce..80d889f4863 100644 --- a/python/GafferImageTest/ContactSheetCoreTest.py +++ b/python/GafferImageTest/ContactSheetCoreTest.py @@ -124,8 +124,9 @@ def testNoInvalidViewAccesses( self ) : checker = GafferImage.Checkerboard() createViews = GafferImage.CreateViews() - createViews["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug(), True, "left", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) + createViews["views"].resize( 1 ) createViews["views"][0]["value"].setInput( checker["out"] ) + createViews["views"][0]["name"].setValue( "left" ) self.assertEqual( createViews["out"].viewNames(), IECore.StringVectorData( [ "left" ] ) ) contactSheet = GafferImage.ContactSheetCore() diff --git a/python/GafferImageTest/CopyViewsTest.py b/python/GafferImageTest/CopyViewsTest.py index f48ceecf9bf..78e518a4103 100644 --- a/python/GafferImageTest/CopyViewsTest.py +++ b/python/GafferImageTest/CopyViewsTest.py @@ -66,8 +66,9 @@ def test( self ) : name = "source%iview%i" % ( i, j ) createViews[i].addChild( constant ) - createViews[i]["views"].addChild( Gaffer.NameValuePlug( name, GafferImage.ImagePlug(), True, "view0", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - createViews[i]["views"][-1]["value"].setInput( constant["out"] ) + view = createViews[i]["views"].next() + view["name"].setValue( name ) + view["value"].setInput( constant["out"] ) copyViews = GafferImage.CopyViews() copyViews["in"][0].setInput( createViews[0]["out"] ) diff --git a/python/GafferImageTest/CreateViewsTest.py b/python/GafferImageTest/CreateViewsTest.py index 820f0a4173c..d6db37a61fc 100644 --- a/python/GafferImageTest/CreateViewsTest.py +++ b/python/GafferImageTest/CreateViewsTest.py @@ -37,6 +37,7 @@ import unittest import imath import inspect +import pathlib import IECore @@ -56,8 +57,9 @@ def test( self ) : script.addChild( createViews ) # Default views added by the UI - createViews["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug(), True, "view0", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - createViews["views"].addChild( Gaffer.NameValuePlug( "right", GafferImage.ImagePlug(), True, "view1", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) + createViews["views"].resize( 2 ) + createViews["views"][0]["name"].setValue( "left" ) + createViews["views"][1]["name"].setValue( "right" ) reader = GafferImage.ImageReader() script.addChild( reader ) @@ -91,9 +93,9 @@ def test( self ) : self.assertImagesEqual( createViews["out"], deserialise["CreateViews"]["out"] ) - createViews["views"].addChild( Gaffer.NameValuePlug( "custom", GafferImage.ImagePlug(), True, "view1", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - createViews["views"]["view2"]["name"].setValue( "blah" ) - createViews["views"]["view2"]["value"].setInput( constant2["out"] ) + createViews["views"].resize( 3 ) + createViews["views"][2]["name"].setValue( "blah" ) + createViews["views"][2]["value"].setInput( constant2["out"] ) self.assertEqual( createViews["out"].viewNames(), IECore.StringVectorData( [ "left", "right", "blah" ] ) ) self.assertEqual( @@ -192,8 +194,9 @@ def testInputToExpressionDrivingEnabledPlug( self ) : # `default` view with RGBA channels, and `notDefault` view with no channels script["createViews"] = GafferImage.CreateViews() - script["createViews"]["views"].addChild( Gaffer.NameValuePlug( "default", GafferImage.ImagePlug(), True, "view0", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - script["createViews"]["views"].addChild( Gaffer.NameValuePlug( "notDefault", GafferImage.ImagePlug(), True, "view1", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) + script["createViews"]["views"].resize( 2 ) + script["createViews"]["views"][0]["name"].setValue( "default" ) + script["createViews"]["views"][1]["name"].setValue( "notDefault" ) script["createViews"]["views"][0]["value"].setInput( script["checkerboard"]["out"] ) self.assertEqual( script["createViews"]["out"].channelNames( "default" ), IECore.StringVectorData( [ "R", "G", "B", "A" ] ) ) self.assertEqual( script["createViews"]["out"].channelNames( "notDefault" ), IECore.StringVectorData() ) @@ -228,5 +231,26 @@ def testNoRedundantSerialisation( self ) : script["createViews"] = GafferImage.CreateViews() self.assertNotIn( "setInput", script.serialise() ) + def testLoadFromVersion1_4( self ) : + + script = Gaffer.ScriptNode() + script["fileName"].setValue( pathlib.Path( __file__ ).parent / "scripts" / "createViews-1.4.10.0.gfr" ) + script.load() + + def assertLoadedOK( script ) : + + self.assertEqual( len( script["CreateViews"]["views"] ), 2 ) + self.assertEqual( script["CreateViews"]["views"][0]["name"].getValue(), "left" ) + self.assertEqual( script["CreateViews"]["views"][1]["name"].getValue(), "right" ) + self.assertEqual( script["CreateViews"]["views"][0]["value"].getInput(), script["CheckerboardLeft"]["out"] ) + self.assertEqual( script["CreateViews"]["views"][1]["value"].getInput(), script["CheckerboardRight"]["out"] ) + + assertLoadedOK( script ) + + script2 = Gaffer.ScriptNode() + script2.execute( script.serialise() ) + + assertLoadedOK( script2 ) + if __name__ == "__main__": unittest.main() diff --git a/python/GafferImageTest/DeleteViewsTest.py b/python/GafferImageTest/DeleteViewsTest.py index 45b69ff975e..db9fd39f7a8 100644 --- a/python/GafferImageTest/DeleteViewsTest.py +++ b/python/GafferImageTest/DeleteViewsTest.py @@ -63,9 +63,10 @@ def test( self ) : createViews = GafferImage.CreateViews() - createViews["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug(), True, "view0", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - createViews["views"].addChild( Gaffer.NameValuePlug( "right", GafferImage.ImagePlug(), True, "view1", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - createViews["views"].addChild( Gaffer.NameValuePlug( "default", GafferImage.ImagePlug(), True, "view2", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) + createViews["views"].resize( 3 ) + createViews["views"][0]["name"].setValue( "left" ) + createViews["views"][1]["name"].setValue( "right" ) + createViews["views"][2]["name"].setValue( "default" ) createViews["views"]["view0"]["value"].setInput( reader["out"] ) createViews["views"]["view1"]["value"].setInput( constant1["out"] ) diff --git a/python/GafferImageTest/FormatQueryTest.py b/python/GafferImageTest/FormatQueryTest.py index 074ea7710c6..c64a0e96c47 100644 --- a/python/GafferImageTest/FormatQueryTest.py +++ b/python/GafferImageTest/FormatQueryTest.py @@ -99,8 +99,9 @@ def testView( self ) : reader["fileName"].setValue( self.imagesPath() / "checkerboard.100x100.exr" ) views = GafferImage.CreateViews() - views["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug(), True ) ) - views["views"].addChild( Gaffer.NameValuePlug( "right", GafferImage.ImagePlug(), True ) ) + views["views"].resize( 2 ) + views["views"][0]["name"].setValue( "left" ) + views["views"][1]["name"].setValue( "right" ) views["views"][0]["value"].setInput( constantSource["out"] ) views["views"][1]["value"].setInput( reader["out"] ) diff --git a/python/GafferImageTest/ImageSamplerTest.py b/python/GafferImageTest/ImageSamplerTest.py index fe987ffef71..9a2dd71c8f6 100644 --- a/python/GafferImageTest/ImageSamplerTest.py +++ b/python/GafferImageTest/ImageSamplerTest.py @@ -132,8 +132,9 @@ def testView( self ) : reader["fileName"].setValue( self.imagesPath() / "blueWithDataWindow.100x100.exr" ) views = GafferImage.CreateViews() - views["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug(), True ) ) - views["views"].addChild( Gaffer.NameValuePlug( "right", GafferImage.ImagePlug(), True ) ) + views["views"].resize( 2 ) + views["views"][0]["name"].setValue( "left" ) + views["views"][1]["name"].setValue( "right" ) views["views"][0]["value"].setInput( constantSource["out"] ) views["views"][1]["value"].setInput( reader["out"] ) diff --git a/python/GafferImageTest/ImageStatsTest.py b/python/GafferImageTest/ImageStatsTest.py index cf5b105e569..68b618b15e1 100644 --- a/python/GafferImageTest/ImageStatsTest.py +++ b/python/GafferImageTest/ImageStatsTest.py @@ -310,8 +310,9 @@ def testView( self ) : reader["fileName"].setValue( self.__rgbFilePath ) views = GafferImage.CreateViews() - views["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug(), True ) ) - views["views"].addChild( Gaffer.NameValuePlug( "right", GafferImage.ImagePlug(), True ) ) + views["views"].resize( 2 ) + views["views"][0]["name"].setValue( "left" ) + views["views"][1]["name"].setValue( "right" ) views["views"][0]["value"].setInput( constantSource["out"] ) views["views"][1]["value"].setInput( reader["out"] ) diff --git a/python/GafferImageTest/ImageTestCase.py b/python/GafferImageTest/ImageTestCase.py index 84ffb91bb53..d5018a110fb 100644 --- a/python/GafferImageTest/ImageTestCase.py +++ b/python/GafferImageTest/ImageTestCase.py @@ -240,8 +240,9 @@ def channelTestImage( self ) : def channelTestImageMultiView( self ) : channelTestImageMultiView = GafferImage.CreateViews() - channelTestImageMultiView["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug(), True, "view0", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - channelTestImageMultiView["views"].addChild( Gaffer.NameValuePlug( "right", GafferImage.ImagePlug(), True, "view1", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) + channelTestImageMultiView["views"].resize( 2 ) + channelTestImageMultiView["views"][0]["name"].setValue( "left" ) + channelTestImageMultiView["views"][1]["name"].setValue( "right" ) channelTestImageMultiView["TestImage"] = self.channelTestImage() diff --git a/python/GafferImageTest/MergeTest.py b/python/GafferImageTest/MergeTest.py index 92d84e19f07..317318872ee 100644 --- a/python/GafferImageTest/MergeTest.py +++ b/python/GafferImageTest/MergeTest.py @@ -859,14 +859,16 @@ def testMultiView( self ) : c4["format"]["displayWindow"].setValue( imath.Box2i( imath.V2i( 0 ), imath.V2i( 64 ) ) ) createViews1 = GafferImage.CreateViews() - createViews1["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug(), True, "view0", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - createViews1["views"].addChild( Gaffer.NameValuePlug( "right", GafferImage.ImagePlug(), True, "view1", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) + createViews1["views"].resize( 2 ) + createViews1["views"][0]["name"].setValue( "left" ) + createViews1["views"][1]["name"].setValue( "right" ) createViews1["views"][0]["value"].setInput( c1["out"] ) createViews1["views"][1]["value"].setInput( c2["out"] ) createViews2 = GafferImage.CreateViews() - createViews2["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug(), True, "view0", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - createViews2["views"].addChild( Gaffer.NameValuePlug( "right", GafferImage.ImagePlug(), True, "view1", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) + createViews2["views"].resize( 2 ) + createViews2["views"][0]["name"].setValue( "left" ) + createViews2["views"][1]["name"].setValue( "right" ) createViews2["views"][0]["value"].setInput( c3["out"] ) createViews2["views"][1]["value"].setInput( c4["out"] ) diff --git a/python/GafferImageTest/SelectViewTest.py b/python/GafferImageTest/SelectViewTest.py index f9eeda69836..575a1e183c9 100644 --- a/python/GafferImageTest/SelectViewTest.py +++ b/python/GafferImageTest/SelectViewTest.py @@ -63,9 +63,10 @@ def test( self ) : createViews = GafferImage.CreateViews() - createViews["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug(), True, "view0", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - createViews["views"].addChild( Gaffer.NameValuePlug( "right", GafferImage.ImagePlug(), True, "view1", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - createViews["views"].addChild( Gaffer.NameValuePlug( "default", GafferImage.ImagePlug(), True, "view2", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) + createViews["views"].resize( 3 ) + createViews["views"][0]["name"].setValue( "left" ) + createViews["views"][1]["name"].setValue( "right" ) + createViews["views"][2]["name"].setValue( "default" ) createViews["views"]["view0"]["value"].setInput( reader["out"] ) createViews["views"]["view1"]["value"].setInput( constant1["out"] ) diff --git a/python/GafferImageUI/CreateViewsUI.py b/python/GafferImageUI/CreateViewsUI.py index e611641e0cc..96b53d309db 100644 --- a/python/GafferImageUI/CreateViewsUI.py +++ b/python/GafferImageUI/CreateViewsUI.py @@ -43,9 +43,9 @@ # sets up the default "left" and "right" views def postCreate( node, menu ) : - node["views"].addChild( Gaffer.NameValuePlug( "left", GafferImage.ImagePlug(), True, "view0", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - node["views"].addChild( Gaffer.NameValuePlug( "right", GafferImage.ImagePlug(), True, "view1", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) - + node["views"].resize( 2 ) + node["views"][0]["name"].setValue( "left" ) + node["views"][1]["name"].setValue( "right" ) Gaffer.Metadata.registerNode( From cd704635ded9ffd671a6ee716d03b4550549873f Mon Sep 17 00:00:00 2001 From: John Haddon Date: Tue, 27 Aug 2024 15:36:44 +0100 Subject: [PATCH 10/10] Document violations of ArrayPlug API And outline a possible path forwards. In practice, we're currently getting away with the violations because the serialisations for these nodes all call `addQuery()` to resize the arrays upfront, so that the arrays already have the required size before the `resize()` call is executed. --- src/Gaffer/ArrayPlug.cpp | 4 ++++ src/Gaffer/ContextQuery.cpp | 1 + src/GafferBindings/Serialisation.cpp | 2 +- src/GafferScene/OptionQuery.cpp | 1 + src/GafferScene/PrimitiveVariableQuery.cpp | 1 + src/GafferScene/ShaderQuery.cpp | 10 +++++++++- 6 files changed, 17 insertions(+), 2 deletions(-) diff --git a/src/Gaffer/ArrayPlug.cpp b/src/Gaffer/ArrayPlug.cpp index 5476f46b781..8450860af1d 100644 --- a/src/Gaffer/ArrayPlug.cpp +++ b/src/Gaffer/ArrayPlug.cpp @@ -104,6 +104,10 @@ bool ArrayPlug::acceptsChild( const GraphComponent *potentialChild ) const return true; } + /// \todo We could beef up these checks to check any descendants of + /// `potentialChild` and to check the default value etc. We can't do that + /// until we fix a few violators of our constraint that all children are + /// identical - see `ShaderQuery::ShaderQuery`. return potentialChild->typeId() == m_elementPrototype->typeId(); } diff --git a/src/Gaffer/ContextQuery.cpp b/src/Gaffer/ContextQuery.cpp index 1aaa295cb27..f387eab28e4 100644 --- a/src/Gaffer/ContextQuery.cpp +++ b/src/Gaffer/ContextQuery.cpp @@ -125,6 +125,7 @@ ContextQuery::ContextQuery( const std::string &name ) : Gaffer::ComputeNode( nam { storeIndexOfNextChild( g_firstPlugIndex ); + /// \todo See notes in `ShaderQuery::ShaderQuery`. addChild( new ArrayPlug( "queries", Plug::Direction::In, nullptr, 1, std::numeric_limits::max(), Plug::Flags::Default, false ) ); addChild( new ArrayPlug( "out", Plug::Direction::Out, nullptr, 1, std::numeric_limits::max(), Plug::Flags::Default, false ) ); } diff --git a/src/GafferBindings/Serialisation.cpp b/src/GafferBindings/Serialisation.cpp index f16e442ccfc..5d40ff6a45b 100644 --- a/src/GafferBindings/Serialisation.cpp +++ b/src/GafferBindings/Serialisation.cpp @@ -82,7 +82,7 @@ namespace // and more readable, and opens the possibility of omitting the // overhead of the names entirely one day. /// \todo Consider an official way for GraphComponents to opt in -/// to this behaviour. +/// to this behaviour. Perhaps this could just be driven by metadata? bool keyedByIndex( const GraphComponent *parent ) { return diff --git a/src/GafferScene/OptionQuery.cpp b/src/GafferScene/OptionQuery.cpp index 7c73b6bbd33..f77c960d962 100644 --- a/src/GafferScene/OptionQuery.cpp +++ b/src/GafferScene/OptionQuery.cpp @@ -135,6 +135,7 @@ OptionQuery::OptionQuery( const std::string &name ) : Gaffer::ComputeNode( name storeIndexOfNextChild( g_firstPlugIndex ); addChild( new ScenePlug( "scene" ) ); + /// \todo See notes in `ShaderQuery::ShaderQuery`. addChild( new ArrayPlug( "queries", Plug::Direction::In, nullptr, 1, std::numeric_limits::max(), Plug::Flags::Default, false ) ); addChild( new ArrayPlug( "out", Plug::Direction::Out, nullptr, 1, std::numeric_limits::max(), Plug::Flags::Default, false ) ); diff --git a/src/GafferScene/PrimitiveVariableQuery.cpp b/src/GafferScene/PrimitiveVariableQuery.cpp index f1828d0b70a..5c3305675a0 100644 --- a/src/GafferScene/PrimitiveVariableQuery.cpp +++ b/src/GafferScene/PrimitiveVariableQuery.cpp @@ -134,6 +134,7 @@ PrimitiveVariableQuery::PrimitiveVariableQuery( const std::string& name ) storeIndexOfNextChild( g_firstPlugIndex ); addChild( new ScenePlug( "scene" ) ); addChild( new Gaffer::StringPlug( "location" ) ); + /// \todo See notes in `ShaderQuery::ShaderQuery`. addChild( new Gaffer::ArrayPlug( "queries", Gaffer::Plug::Direction::In, nullptr, 1, std::numeric_limits< size_t >::max(), Gaffer::Plug::Flags::Default, false ) ); addChild( new Gaffer::ArrayPlug( "out", Gaffer::Plug::Direction::Out, diff --git a/src/GafferScene/ShaderQuery.cpp b/src/GafferScene/ShaderQuery.cpp index 7728dcef569..ef8aa7f813a 100644 --- a/src/GafferScene/ShaderQuery.cpp +++ b/src/GafferScene/ShaderQuery.cpp @@ -141,8 +141,16 @@ ShaderQuery::ShaderQuery( const std::string &name ) : Gaffer::ComputeNode( name addChild( new StringPlug( "location" ) ); addChild( new StringPlug( "shader" ) ); addChild( new BoolPlug( "inherit", Plug::In, false ) ); + /// \todo This is violating the ArrayPlug requirements by not providing an `elementPrototype` + /// at construction. And we later violate it further by creating non-homogeneous elements in + /// `addQuery()`. We're only using an ArrayPlug because we want the serialisation to use numeric + /// indexing for the children of `queries` and `out` - since the serialisation uses `addQuery()` + /// to recreate the children, and that doesn't maintain names. So perhaps we can just use a + /// ValuePlug instead of an ArrayPlug, and add a separate mechanism for requesting that children use + /// numeric indices separately (see `keyedByIndex()` in `Serialisation.cpp`). + /// + /// The same applies to OptionQuery, PrimitiveVariableQuery and ContextQuery. addChild( new ArrayPlug( "queries", Plug::Direction::In, nullptr, 1, std::numeric_limits::max(), Plug::Flags::Default, false ) ); - addChild( new ArrayPlug( "out", Plug::Direction::Out, nullptr, 1, std::numeric_limits::max(), Plug::Flags::Default, false ) ); AttributeQueryPtr attributeQuery = new AttributeQuery( "__attributeQuery" );