Skip to content

Commit

Permalink
BackgroundTask : Follow plug inputs to find ScriptNode for cancellation
Browse files Browse the repository at this point in the history
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.

<details>

```
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
```

<details>
  • Loading branch information
johnhaddon committed Jul 3, 2023
1 parent 0516164 commit 67aea86
Show file tree
Hide file tree
Showing 2 changed files with 27 additions and 13 deletions.
4 changes: 3 additions & 1 deletion Changes.md
Original file line number Diff line number Diff line change
Expand Up @@ -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()`.
Expand Down
36 changes: 24 additions & 12 deletions src/Gaffer/BackgroundTask.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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<const Plug>( 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<Plug>( "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<Plug>( "in" )->getInput() );
}
subject = subject->parent();
}
return nullptr;
}

struct ActiveTask
Expand Down

0 comments on commit 67aea86

Please sign in to comment.