From 1bf0205fa39268951a6997d82e7d4108ad894aed Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 23 Aug 2024 09:25:35 +0100 Subject: [PATCH 1/2] 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 4108f9ce326..93207d04282 100644 --- a/Changes.md +++ b/Changes.md @@ -8,6 +8,9 @@ Fixes - Fixed partial image updates when an unrelated InteractiveRender was running (#6043). - Fixed "colour tearing", where updates to some image channels became visible before updates to others. - Fixed unnecessary texture updates when specific image tiles don't change. +- ArrayPlug : + - Fixed error when `resize()` removed plugs with input connections. + - Fixed error when `resize()` was used on an output plug. 1.3.16.8 (relative to 1.3.16.7) ======== diff --git a/python/GafferTest/ArrayPlugTest.py b/python/GafferTest/ArrayPlugTest.py index af71ce2d9b1..57c17d1e87e 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 75abec3e0472018af1063964fcb9149a5a24da76 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Wed, 25 Sep 2024 12:51:59 +0100 Subject: [PATCH 2/2] CreateViews : Support loading from Gaffer 1.5 In 1.5, the `views` array is created with a prototype, and `resize()` is used in the serialisation to add all necessary children. When running `resize()` in 1.3 though, we have no prototype and can't resize. So we make `ArrayPlug::resize()` a no-op in this case and add the plugs on a just-in-time basis in a compatibility config. I really don't like monkey-patching all ArrayPlugs like this, but it's not possible to monkey-patch individual instances when patching Python's magic methods (of which `__getitem__` is one). We'll rip out all of this code when merging forwards to `main` though, so it won't live for _too_ long. --- Changes.md | 1 + python/GafferImageTest/CreateViewsTest.py | 14 +++++ .../scripts/createViews-1.5.0.0.gfr | 45 +++++++++++++++ python/GafferTest/ArrayPlugTest.py | 6 ++ src/Gaffer/ArrayPlug.cpp | 24 ++++++++ .../GafferImage/createViewsCompatibility.py | 56 +++++++++++++++++++ 6 files changed, 146 insertions(+) create mode 100644 python/GafferImageTest/scripts/createViews-1.5.0.0.gfr create mode 100644 startup/GafferImage/createViewsCompatibility.py diff --git a/Changes.md b/Changes.md index 93207d04282..bf5c9ba090d 100644 --- a/Changes.md +++ b/Changes.md @@ -11,6 +11,7 @@ Fixes - ArrayPlug : - Fixed error when `resize()` removed plugs with input connections. - Fixed error when `resize()` was used on an output plug. +- CreateViews : Fixed loading of files saved from Gaffer 1.5+. 1.3.16.8 (relative to 1.3.16.7) ======== diff --git a/python/GafferImageTest/CreateViewsTest.py b/python/GafferImageTest/CreateViewsTest.py index 3db8b0252c5..cc6800d886f 100644 --- a/python/GafferImageTest/CreateViewsTest.py +++ b/python/GafferImageTest/CreateViewsTest.py @@ -38,6 +38,7 @@ import imath import inspect import os +import pathlib import IECore @@ -224,5 +225,18 @@ def testInputToExpressionDrivingEnabledPlug( self ) : # for this particular view. self.assertFalse( script["constant"]["enabled"].getValue() ) + def testLoadFromVersion1_5( self ) : + + script = Gaffer.ScriptNode() + script["fileName"].setValue( pathlib.Path( __file__ ).parent / "scripts" / "createViews-1.5.0.0.gfr" ) + script.load() + + 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.assertTrue( script["CreateViews"]["views"][0]["value"].getInput().isSame( script["Checkerboard"]["out"] ) ) + self.assertTrue( script["CreateViews"]["views"][1]["value"].getInput().isSame( script["Checkerboard1"]["out"] ) ) + self.assertEqual( script["CreateViews"]["out"].viewNames(), IECore.StringVectorData( [ "left", "right" ] ) ) + if __name__ == "__main__": unittest.main() diff --git a/python/GafferImageTest/scripts/createViews-1.5.0.0.gfr b/python/GafferImageTest/scripts/createViews-1.5.0.0.gfr new file mode 100644 index 00000000000..56b4bd1454a --- /dev/null +++ b/python/GafferImageTest/scripts/createViews-1.5.0.0.gfr @@ -0,0 +1,45 @@ +import Gaffer +import GafferImage +import imath + +Gaffer.Metadata.registerValue( parent, "serialiser:milestoneVersion", 1, persistent=False ) +Gaffer.Metadata.registerValue( parent, "serialiser:majorVersion", 5, persistent=False ) +Gaffer.Metadata.registerValue( parent, "serialiser:minorVersion", 0, 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["Checkerboard"] = GafferImage.Checkerboard( "Checkerboard" ) +parent.addChild( __children["Checkerboard"] ) +__children["Checkerboard"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) +__children["Checkerboard1"] = GafferImage.Checkerboard( "Checkerboard1" ) +parent.addChild( __children["Checkerboard1"] ) +__children["Checkerboard1"].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"].resize( 2 ) +__children["CreateViews"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) +parent["variables"]["imageCataloguePort"]["value"].setValue( 34907 ) +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["Checkerboard"]["size"]["y"].setInput( __children["Checkerboard"]["size"]["x"] ) +__children["Checkerboard"]["__uiPosition"].setValue( imath.V2f( -30.6000004, 10.4000006 ) ) +__children["Checkerboard1"]["size"]["y"].setInput( __children["Checkerboard1"]["size"]["x"] ) +__children["Checkerboard1"]["__uiPosition"].setValue( imath.V2f( -16.8000031, 10.5 ) ) +__children["CreateViews"]["views"][0]["name"].setValue( 'left' ) +__children["CreateViews"]["views"][0]["value"].setInput( __children["Checkerboard"]["out"] ) +__children["CreateViews"]["views"][1]["name"].setValue( 'right' ) +__children["CreateViews"]["views"][1]["value"].setInput( __children["Checkerboard1"]["out"] ) +__children["CreateViews"]["__uiPosition"].setValue( imath.V2f( -23.8000011, 2.23593783 ) ) + + +del __children + diff --git a/python/GafferTest/ArrayPlugTest.py b/python/GafferTest/ArrayPlugTest.py index 57c17d1e87e..1682f256b82 100644 --- a/python/GafferTest/ArrayPlugTest.py +++ b/python/GafferTest/ArrayPlugTest.py @@ -531,6 +531,12 @@ def testSerialisationUsesIndices( self ) : self.assertEqual( s2["n"]["in"][0].getInput(), s2["a"]["sum"] ) self.assertEqual( s2["n"]["in"][1].getInput(), s2["a"]["sum"] ) + def testResizeWithoutExistingChildren( self ) : + + p = Gaffer.ArrayPlug( name = "p" ) + with self.assertRaisesRegex( RuntimeError, "Can't resize ArrayPlug `p` as it has no children" ) : + p.resize( 1 ) + def tearDown( self ) : # some bugs in the InputGenerator only showed themselves when diff --git a/src/Gaffer/ArrayPlug.cpp b/src/Gaffer/ArrayPlug.cpp index 18165777072..78f6000a98f 100644 --- a/src/Gaffer/ArrayPlug.cpp +++ b/src/Gaffer/ArrayPlug.cpp @@ -151,6 +151,30 @@ void ArrayPlug::resize( size_t size ) throw IECore::Exception( "Invalid size" ); } + if( size && !children().size() ) + { + if( auto scriptNode = ancestor() ) + { + if( scriptNode->isExecuting() ) + { + // Needed to allow CreateViews nodes serialised from Gaffer 1.5 + // to be loaded. In 1.5, CreateViews creates an ArrayPlug with + // an element prototype, and uses `resize()` in the + // serialisation. We can't resize here because we don't have a + // prototype, but as long as we don't error, + // `startup/GafferImage/createViewsCompatibility.py` will deal + // with the rest for us. + return; + } + } + throw IECore::Exception( + fmt::format( + "Can't resize ArrayPlug `{}` as it has no children", + fullName() + ) + ); + } + while( size > children().size() ) { PlugPtr p = getChild( 0 )->createCounterpart( getChild( 0 )->getName(), direction() ); diff --git a/startup/GafferImage/createViewsCompatibility.py b/startup/GafferImage/createViewsCompatibility.py new file mode 100644 index 00000000000..fa199aa3cfe --- /dev/null +++ b/startup/GafferImage/createViewsCompatibility.py @@ -0,0 +1,56 @@ +########################################################################## +# +# 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 +import GafferImage + +def __arrayPlugGetItem( originalGetItem ) : + + def getItem( self, key ) : + + if isinstance( key, int ) and key >= len( self ) : + if isinstance( self.parent(), GafferImage.CreateViews ) and self.getName() == "views" : + # Probably loading a serialisation from Gaffer 1.5+, where `resize()` would + # have added the array elements that we are missing here. Just add them manually. + if not len( self ) : + self.addChild( Gaffer.NameValuePlug( "", GafferImage.ImagePlug(), True, "view0", Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic ) ) + self.resize( key + 1 ) + + return originalGetItem( self, key ) + + return getItem + +Gaffer.ArrayPlug.__getitem__ = __arrayPlugGetItem( Gaffer.ArrayPlug.__getitem__ )