From 67aea86c662b9d2dc00b70367c74d48087458fdc Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 30 Jun 2023 15:46:52 +0100 Subject: [PATCH] BackgroundTask : Follow plug inputs to find ScriptNode for cancellation This allows us to find the ScriptNode from the LightEditor, which uses an internal ScenePlug to receive the scene to display. Prior to this, we were failing to find the ScriptNode at all, which meant that the LightEditor's background computes were not cancelled prior to graph modifications being made by the user. The _appearance_ of such cancellation was provided by the `ScenePath::changedSignal()` being emitted _after_ the edit was made, causing the LightEditor's internal PathListingWidget to cancel the current update and start a new one. But that left a crucial interval during which _anything_ could happen, because the old update was still accessing the graph while it was being modified. A fairly simple repro for the problem can be seen in the script below : 1. Start Gaffer with `GAFFER_HASHCACHE_MODE=Checked`. 1. Pin the Viewer and HierarchyView to show nothing, so only the LightEditor is active. 2. Select `Node`. 3. Focus `Set` and switch to the LightEditor tab to view it. 4. Use the cursor key to increment the `Node.locations` plug, and wait. Repeat this a few times, waiting between keypresses. All will be well, because the updates happen before the next edit. This step isn't essential, but it helps demonstrate what is happening. 5. Now increment `Node.locations` rapidly by holding down the cursor key. Now graph edits are being made before the UI update is complete, and without waiting for cancellation. This results in `Detected undeclared dependency` errors been thrown by the `Checked` hash cache mode. What's happening is that the `Checked` mode calculates the hash twice, and sometimes the calculations will fall either side of the graph edit, yielding different results. Of course, _anything_ could happen, and this is just one symptom. Crashes, hangs and incorrect output would be the other possible outcomes in the real world.
``` import Gaffer import GafferScene import IECore import imath Gaffer.Metadata.registerValue( parent, "serialiser:milestoneVersion", 1 persistent=False ) Gaffer.Metadata.registerValue( parent, "serialiser:majorVersion", 2, persistent=False ) Gaffer.Metadata.registerValue( parent, "serialiser:minorVersion", 8, persistent=False ) Gaffer.Metadata.registerValue( parent, "serialiser:patchVersion", 0, persistent=False ) __children = {} __children["Set"] = GafferScene.Set( "Set" ) parent.addChild( __children["Set"] ) __children["Set"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["PathFilter"] = GafferScene.PathFilter( "PathFilter" ) parent.addChild( __children["PathFilter"] ) __children["PathFilter"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["CollectScenes"] = GafferScene.CollectScenes( "CollectScenes" ) parent.addChild( __children["CollectScenes"] ) __children["CollectScenes"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["Expression3"] = Gaffer.Expression( "Expression3" ) parent.addChild( __children["Expression3"] ) __children["Expression3"]["__in"].addChild( Gaffer.IntPlug( "p0", defaultValue = 0, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["Expression3"]["__out"].addChild( Gaffer.StringVectorDataPlug( "p0", direction = Gaffer.Plug.Direction.Out, defaultValue = IECore.StringVectorData( [ ] ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["Expression3"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["Sphere1"] = GafferScene.Sphere( "Sphere1" ) parent.addChild( __children["Sphere1"] ) __children["Sphere1"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["Expression4"] = Gaffer.Expression( "Expression4" ) parent.addChild( __children["Expression4"] ) __children["Expression4"]["__out"].addChild( Gaffer.StringPlug( "p0", direction = Gaffer.Plug.Direction.Out, defaultValue = 'sphere', flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["Expression4"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["Node"] = Gaffer.Node( "Node" ) parent.addChild( __children["Node"] ) __children["Node"]["user"].addChild( Gaffer.IntPlug( "locations", defaultValue = 0, flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["Node"].addChild( Gaffer.V2fPlug( "__uiPosition", defaultValue = imath.V2f( 0, 0 ), flags = Gaffer.Plug.Flags.Default | Gaffer.Plug.Flags.Dynamic, ) ) __children["Set"]["in"].setInput( __children["CollectScenes"]["out"] ) __children["Set"]["filter"].setInput( __children["PathFilter"]["out"] ) __children["Set"]["name"].setValue( 'soloLights' ) __children["Set"]["__uiPosition"].setValue( imath.V2f( -18.9498405, -5.46412373 ) ) __children["PathFilter"]["paths"].setValue( IECore.StringVectorData( [ '/...' ] ) ) __children["PathFilter"]["__uiPosition"].setValue( imath.V2f( -5.94977856, 0.617907763 ) ) __children["CollectScenes"]["in"].setInput( __children["Sphere1"]["out"] ) __children["CollectScenes"]["rootNames"].setInput( __children["Expression3"]["__out"]["p0"] ) __children["CollectScenes"]["__uiPosition"].setValue( imath.V2f( -18.9497757, 2.70000958 ) ) __children["Expression3"]["__in"]["p0"].setInput( __children["Node"]["user"]["locations"] ) __children["Expression3"]["__uiPosition"].setValue( imath.V2f( -29.450222, 2.70068288 ) ) __children["Sphere1"]["name"].setInput( __children["Expression4"]["__out"]["p0"] ) __children["Sphere1"]["__uiPosition"].setValue( imath.V2f( -18.9479485, 10.8641243 ) ) __children["Expression4"]["__uiPosition"].setValue( imath.V2f( -29.4481468, 10.8649883 ) ) __children["Node"]["user"]["locations"].setValue( 71 ) Gaffer.Metadata.registerValue( __children["Node"]["user"]["locations"], 'nodule:type', '' ) __children["Node"]["__uiPosition"].setValue( imath.V2f( 1.29999924, 13.1000013 ) ) __children["Expression3"]["__engine"].setValue( 'python' ) __children["Expression3"]["__expression"].setValue( 'l = parent["__in"]["p0"]\nprint( "NUM LOCATIONS", l )\nparent["__out"]["p0"] = IECore.StringVectorData( [ str(i) for i in range( l ) ] )' ) __children["Expression4"]["__engine"].setValue( 'python' ) __children["Expression4"]["__expression"].setValue( 'parent["__out"]["p0"] = context.get( "collect:rootName", "blah" )' ) del __children ```
--- Changes.md | 4 +++- src/Gaffer/BackgroundTask.cpp | 36 +++++++++++++++++++++++------------ 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/Changes.md b/Changes.md index a4efe389e4b..39be9716e95 100644 --- a/Changes.md +++ b/Changes.md @@ -18,7 +18,9 @@ Fixes ----- - Arnold : Fixed bug that caused `ai:volume:step_scale` to be ignored if `ai:volume_step` was set explicitly to `0.0`. This was different to the behaviour when `ai:volume_step` was not set at all. -- LightEditor : Fixed hang caused by the `soloLights` set triggering on an upstream Python expression or ComputeNode (#5365). +- LightEditor : + - Fixed hang caused by the `soloLights` set triggering on an upstream Python expression or ComputeNode (#5365). + - Fixed thread-safety bug that meant the LightEditor attempted to perform computes while the graph was being edited. - OSLImage : Fixed bug preventing channels / layers from being deleted using the right-click menu. - HierarchyView : Fixed crash triggered by layouts with two or more HierarchyViews (#5364). - Context : Fixed crash triggered by a reentrant call to `Context::set()` from within a slot connected to `Context::changedSignal()`. diff --git a/src/Gaffer/BackgroundTask.cpp b/src/Gaffer/BackgroundTask.cpp index 4dd7214fb72..8721e8f1015 100644 --- a/src/Gaffer/BackgroundTask.cpp +++ b/src/Gaffer/BackgroundTask.cpp @@ -60,6 +60,8 @@ namespace const ScriptNode *scriptNode( const GraphComponent *subject ) { + // Simple cases - there is no subject, or it is part of a script. + if( !subject ) { return nullptr; @@ -72,22 +74,32 @@ const ScriptNode *scriptNode( const GraphComponent *subject ) { return s; } - else + + // Special cases to accommodate the UI. + + // UI classes often house their own internal plugs which receive their input + // from nodes in the graph. Follow the inputs to see if we can find the + // graph. + if( auto p = runTimeCast( subject ) ) { - // Unfortunately the GafferUI::View classes house internal - // nodes which live outside any ScriptNode, but must still - // take part in cancellation. This hack recovers the ScriptNode - // for the node the view is currently connected to. - while( subject ) + if( auto s = scriptNode( p->getInput() ) ) { - if( subject->isInstanceOf( "GafferUI::View" ) ) - { - return scriptNode( subject->getChild( "in" )->getInput() ); - } - subject = subject->parent(); + return s; } - return nullptr; } + + // The `GafferUI::View` classes house internal nodes which might not be + // directly connected to the graph. This hack recovers the ScriptNode from + // the node the view is currently connected to. + while( subject ) + { + if( subject->isInstanceOf( "GafferUI::View" ) ) + { + return scriptNode( subject->getChild( "in" )->getInput() ); + } + subject = subject->parent(); + } + return nullptr; } struct ActiveTask