From 632e7d5f1225e829e178ca325b2b331c704710a4 Mon Sep 17 00:00:00 2001 From: John Haddon Date: Fri, 2 Aug 2024 16:26:14 +0100 Subject: [PATCH] Dispatcher : Call `SetupPlugsFn` _after_ construction, not during This uses an `intrusive_ptr_add_ref` gambit first introduced in https://github.com/GafferHQ/gaffer/pull/5985, with the rationale for that over various alternatives being documented there. Fixes #915. --- Changes.md | 1 + include/GafferDispatch/Dispatcher.h | 14 ++++-------- include/GafferDispatch/TaskNode.h | 2 ++ python/GafferDispatchTest/DispatcherTest.py | 24 +++++++++++++++++++++ src/GafferDispatch/TaskNode.cpp | 16 ++++++++++++-- 5 files changed, 45 insertions(+), 12 deletions(-) diff --git a/Changes.md b/Changes.md index 3cc76028e43..2f3a92155a3 100644 --- a/Changes.md +++ b/Changes.md @@ -27,6 +27,7 @@ Fixes - Shuffle, ShuffleAttributes, ShufflePrimitiveVariables : Fixed some special cases where shuffling a source to itself would fail to have the expected effect. - GraphEditor : Fixed dimming of labels for BoxIn and BoxOut 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). API --- diff --git a/include/GafferDispatch/Dispatcher.h b/include/GafferDispatch/Dispatcher.h index 1556292b25a..f43e0aabc4d 100644 --- a/include/GafferDispatch/Dispatcher.h +++ b/include/GafferDispatch/Dispatcher.h @@ -201,8 +201,6 @@ class GAFFERDISPATCH_API Dispatcher : public TaskNode protected : - friend class TaskNode; - IE_CORE_FORWARDDECLARE( TaskBatch ) using TaskBatches = std::vector; @@ -270,16 +268,12 @@ class GAFFERDISPATCH_API Dispatcher : public TaskNode /// batches have been dispatched in order to prevent duplicate work. virtual void doDispatch( const TaskBatch *batch ) const = 0; - //! @name TaskNode Customization - /// Dispatchers are able to create custom plugs on TaskNodes when they are constructed. - ///////////////////////////////////////////////////////////////////////////////////////////// - //@{ - /// Adds the custom plugs from all registered Dispatchers to the given parent Plug. - static void setupPlugs( Gaffer::Plug *parentPlug ); - //@} - private : + // Friendship to allow `setupPlugs()` to be called. + friend void intrusive_ptr_add_ref( TaskNode *node ); + static void setupPlugs( Gaffer::Plug *parentPlug ); + void preTasks( const Gaffer::Context *context, Tasks &tasks ) const final; void postTasks( const Gaffer::Context *context, Tasks &tasks ) const final; IECore::MurmurHash hash( const Gaffer::Context *context ) const final; diff --git a/include/GafferDispatch/TaskNode.h b/include/GafferDispatch/TaskNode.h index 7430178a7e7..a1580826227 100644 --- a/include/GafferDispatch/TaskNode.h +++ b/include/GafferDispatch/TaskNode.h @@ -235,4 +235,6 @@ class GAFFERDISPATCH_API TaskNode : public Gaffer::DependencyNode }; +GAFFERDISPATCH_API void intrusive_ptr_add_ref( TaskNode *node ); + } // namespace GafferDispatch diff --git a/python/GafferDispatchTest/DispatcherTest.py b/python/GafferDispatchTest/DispatcherTest.py index 7ea4d6892a6..14ff232d734 100644 --- a/python/GafferDispatchTest/DispatcherTest.py +++ b/python/GafferDispatchTest/DispatcherTest.py @@ -2313,5 +2313,29 @@ def testDispatchSignalShutdownCrash( self ) : """import GafferDispatch; GafferDispatch.Dispatcher.postDispatchSignal().connect( lambda d, s : None, scoped = False )""" ] ) + def testAccessTaskNodeInSetupPlugs( self ) : + + class SetupPlugsTestDispatcher( GafferDispatch.Dispatcher ) : + + def _doDispatch( self, batch ) : + + pass + + lastNode = None + + @classmethod + def _setupPlugs( cls, parentPlug ) : + + node = parentPlug.node() + self.assertIsInstance( node, GafferDispatch.PythonCommand ) + self.assertEqual( node.typeId(), GafferDispatch.PythonCommand.staticTypeId() ) + cls.lastNode = node + + GafferDispatch.Dispatcher.registerDispatcher( "SetupPlugsTestDispatcher", SetupPlugsTestDispatcher, SetupPlugsTestDispatcher._setupPlugs ) + self.addCleanup( GafferDispatch.Dispatcher.deregisterDispatcher, "SetupPlugsTestDispatcher" ) + + pythonCommand = GafferDispatch.PythonCommand() + self.assertIs( SetupPlugsTestDispatcher.lastNode, pythonCommand ) + if __name__ == "__main__": unittest.main() diff --git a/src/GafferDispatch/TaskNode.cpp b/src/GafferDispatch/TaskNode.cpp index 2c4ef2ab915..076677b28a0 100644 --- a/src/GafferDispatch/TaskNode.cpp +++ b/src/GafferDispatch/TaskNode.cpp @@ -286,8 +286,6 @@ TaskNode::TaskNode( const std::string &name ) PlugPtr dispatcherPlug = new Plug( "dispatcher", Plug::In ); addChild( dispatcherPlug ); - - Dispatcher::setupPlugs( dispatcherPlug.get() ); } TaskNode::~TaskNode() @@ -400,3 +398,17 @@ bool TaskNode::requiresSequenceExecution() const { return false; } + +void GafferDispatch::intrusive_ptr_add_ref( TaskNode *node ) +{ + bool firstRef = node->refCount() == 0; + node->addRef(); + if( firstRef ) + { + // We defer calling `setupPlugs()` till we have our first reference, as + // otherwise Python `setupPlugs()` functions which accessed + // `plug.node()` would crash. This also means that `setupPlugs()` is able + // to introspect the node type and add different plugs on different nodes. + Dispatcher::setupPlugs( node->dispatcherPlug() ); + } +}