From f05e32904800cf18dc07f0bef20ab15e3de9a73a Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 5 Sep 2024 12:34:11 +0100 Subject: [PATCH] SetEditor : Prefer ScriptNodeAlgo to ContextAlgo (phase 1) This takes the same approach as we've taken in the HierarchyView, but hasn't touched the stuff we're using to get selection in the SetPath itself. I'm going to do that in a separate commit so it can be reviewed in isolation. --- Changes.md | 2 +- python/GafferSceneUI/SetEditor.py | 12 +-- src/GafferSceneUIModule/SetEditorBinding.cpp | 82 ++++++++++---------- 3 files changed, 49 insertions(+), 47 deletions(-) diff --git a/Changes.md b/Changes.md index ea5ceb7b0a..247d487638 100644 --- a/Changes.md +++ b/Changes.md @@ -44,7 +44,7 @@ Fixes - PrimitiveInspector : - Fixed bug which prevented cancellation of long-running computes, making the UI unresponsive until they completed. - Fixed thread-safety bug. -- HierarchyView : Fixed thread-safety bug. +- HierarchyView, SetEditor : Fixed thread-safety bugs. API --- diff --git a/python/GafferSceneUI/SetEditor.py b/python/GafferSceneUI/SetEditor.py index 448b9f1284..ae89470df9 100644 --- a/python/GafferSceneUI/SetEditor.py +++ b/python/GafferSceneUI/SetEditor.py @@ -76,8 +76,8 @@ def __init__( self, scriptNode, **kw ) : self.__setMembersColumn = _GafferSceneUI._SetEditor.SetMembersColumn() self.__selectedSetMembersColumn = _GafferSceneUI._SetEditor.SetSelectionColumn() - self.__includedSetMembersColumn = _GafferSceneUI._SetEditor.VisibleSetInclusionsColumn( scriptNode.context() ) - self.__excludedSetMembersColumn = _GafferSceneUI._SetEditor.VisibleSetExclusionsColumn( scriptNode.context() ) + self.__includedSetMembersColumn = _GafferSceneUI._SetEditor.VisibleSetInclusionsColumn( scriptNode ) + self.__excludedSetMembersColumn = _GafferSceneUI._SetEditor.VisibleSetExclusionsColumn( scriptNode ) self.__pathListing = GafferUI.PathListingWidget( Gaffer.DictPath( {}, "/" ), # temp till we make a SetPath columns = [ @@ -229,21 +229,21 @@ def __getSelectedSetMembers( self, setNames, *unused ) : setMembers = self.__getSetMembers( setNames ) return IECore.PathMatcher( [ - p for p in ContextAlgo.getSelectedPaths( self.context() ).paths() + p for p in GafferSceneUI.ScriptNodeAlgo.getSelectedPaths( self.scriptNode() ).paths() if setMembers.match( p ) & ( IECore.PathMatcher.Result.ExactMatch | IECore.PathMatcher.Result.AncestorMatch ) ] ) def __getIncludedSetMembers( self, setNames, *unused ) : - return self.__getSetMembers( setNames ).intersection( ContextAlgo.getVisibleSet( self.context() ).inclusions ) + return self.__getSetMembers( setNames ).intersection( GafferSceneUI.ScriptNodeAlgo.getVisibleSet( self.scriptNode() ).inclusions ) def __getExcludedSetMembers( self, setNames, *unused ) : - return self.__getSetMembers( setNames ).intersection( ContextAlgo.getVisibleSet( self.context() ).exclusions ) + return self.__getSetMembers( setNames ).intersection( GafferSceneUI.ScriptNodeAlgo.getVisibleSet( self.scriptNode() ).exclusions ) def __selectSetMembers( self, *unused ) : - ContextAlgo.setSelectedPaths( self.context(), self.__getSetMembers( self.__selectedSetNames() ) ) + GafferSceneUI.ScriptNodeAlgo.setSelectedPaths( self.scriptNode(), self.__getSetMembers( self.__selectedSetNames() ) ) def __copySetMembers( self, *unused ) : diff --git a/src/GafferSceneUIModule/SetEditorBinding.cpp b/src/GafferSceneUIModule/SetEditorBinding.cpp index 2a494f69e3..583536899e 100644 --- a/src/GafferSceneUIModule/SetEditorBinding.cpp +++ b/src/GafferSceneUIModule/SetEditorBinding.cpp @@ -39,6 +39,7 @@ #include "SetEditorBinding.h" #include "GafferSceneUI/ContextAlgo.h" +#include "GafferSceneUI/ScriptNodeAlgo.h" #include "GafferSceneUI/TypeIds.h" #include "GafferScene/SceneAlgo.h" @@ -53,6 +54,7 @@ #include "Gaffer/Path.h" #include "Gaffer/PathFilter.h" #include "Gaffer/Private/IECorePreview/LRUCache.h" +#include "Gaffer/ScriptNode.h" #include "IECorePython/RefCountedBinding.h" @@ -537,12 +539,12 @@ class VisibleSetInclusionsColumn : public PathColumn IE_CORE_DECLAREMEMBERPTR( VisibleSetInclusionsColumn ) - VisibleSetInclusionsColumn( ContextPtr context ) - : PathColumn(), m_context( context ) + VisibleSetInclusionsColumn( ScriptNodePtr script ) + : PathColumn(), m_script( script ), m_visibleSet( ScriptNodeAlgo::getVisibleSet( script.get() ) ) { buttonPressSignal().connect( boost::bind( &VisibleSetInclusionsColumn::buttonPress, this, ::_3 ) ); buttonReleaseSignal().connect( boost::bind( &VisibleSetInclusionsColumn::buttonRelease, this, ::_1, ::_2, ::_3 ) ); - m_context->changedSignal().connect( boost::bind( &VisibleSetInclusionsColumn::contextChanged, this, ::_2 ) ); + ScriptNodeAlgo::visibleSetChangedSignal( script.get() ).connect( boost::bind( &VisibleSetInclusionsColumn::visibleSetChanged, this ) ); } CellData cellData( const Gaffer::Path &path, const IECore::Canceller *canceller ) const override @@ -567,16 +569,15 @@ class VisibleSetInclusionsColumn : public PathColumn result.icon = iconData; result.toolTip = g_inclusionToolTip; - const auto visibleSet = ContextAlgo::getVisibleSet( m_context.get() ); - if( visibleSet.inclusions.isEmpty() ) + if( m_visibleSet.inclusions.isEmpty() ) { result.value = new IntData( 0 ); return result; } - Context::Scope scopedContext( m_context.get() ); + Context::Scope scopedContext( setPath->getContext() ); const auto setMembers = setPath->getScene()->set( setName->readable() ); - const auto includedSetMembers = setMembers->readable().intersection( visibleSet.inclusions ); + const auto includedSetMembers = setMembers->readable().intersection( m_visibleSet.inclusions ); result.value = new IntData( includedSetMembers.size() ); if( includedSetMembers.isEmpty() ) { @@ -584,11 +585,11 @@ class VisibleSetInclusionsColumn : public PathColumn } size_t excludedSetMemberCount = 0; - if( !visibleSet.exclusions.isEmpty() ) + if( !m_visibleSet.exclusions.isEmpty() ) { for( IECore::PathMatcher::Iterator it = includedSetMembers.begin(), eIt = includedSetMembers.end(); it != eIt; ++it ) { - const auto visibility = visibleSet.visibility( *it ); + const auto visibility = m_visibleSet.visibility( *it ); if( visibility.drawMode != GafferScene::VisibleSet::Visibility::Visible ) { excludedSetMemberCount++; @@ -619,18 +620,18 @@ class VisibleSetInclusionsColumn : public PathColumn CellData headerData( const IECore::Canceller *canceller ) const override { - const auto visibleSet = ContextAlgo::getVisibleSet( m_context.get() ); - return CellData( /* value = */ nullptr, /* icon = */ visibleSet.inclusions.isEmpty() ? g_inclusionsEmptyIconName : g_setIncludedIconName, /* background = */ nullptr, /* tooltip = */ new StringData( "Visible Set Inclusions" ) ); + return CellData( /* value = */ nullptr, /* icon = */ m_visibleSet.inclusions.isEmpty() ? g_inclusionsEmptyIconName : g_setIncludedIconName, /* background = */ nullptr, /* tooltip = */ new StringData( "Visible Set Inclusions" ) ); } private : - void contextChanged( const IECore::InternedString &name ) + void visibleSetChanged() { - if( ContextAlgo::affectsVisibleSet( name ) ) - { - changedSignal()( this ); - } + // We take a copy, because `cellData()` is called from background threads, + // and it's not safe to call `getVisibleSet()` concurrently with modifications + // on the foreground thread. + m_visibleSet = ScriptNodeAlgo::getVisibleSet( m_script.get() ); + changedSignal()( this ); } bool buttonPress( const ButtonEvent &event ) @@ -658,7 +659,7 @@ class VisibleSetInclusionsColumn : public PathColumn return false; } - Context::Scope scopedContext( m_context.get() ); + Context::Scope scopedContext( setPath->getContext() ); const auto setMembers = setPath->getScene()->set( setName->readable() ); auto pathsToInclude = IECore::PathMatcher( setMembers->readable() ); const auto selection = widget.getSelection(); @@ -682,7 +683,7 @@ class VisibleSetInclusionsColumn : public PathColumn } bool update = false; - auto visibleSet = ContextAlgo::getVisibleSet( m_context.get() ); + auto visibleSet = m_visibleSet; if( event.button == ButtonEvent::Left && !event.modifiers ) { const auto includedSetMembers = setMembers->readable().intersection( visibleSet.inclusions ); @@ -702,13 +703,14 @@ class VisibleSetInclusionsColumn : public PathColumn if( update ) { - ContextAlgo::setVisibleSet( m_context.get(), visibleSet ); + ScriptNodeAlgo::setVisibleSet( m_script.get(), visibleSet ); } return true; } - ContextPtr m_context; + ScriptNodePtr m_script; + VisibleSet m_visibleSet; static IECore::StringDataPtr g_setIncludedIconName; static IECore::StringDataPtr g_setIncludedDisabledIconName; @@ -777,12 +779,12 @@ class VisibleSetExclusionsColumn : public PathColumn IE_CORE_DECLAREMEMBERPTR( VisibleSetExclusionsColumn ) - VisibleSetExclusionsColumn( ContextPtr context ) - : PathColumn(), m_context( context ) + VisibleSetExclusionsColumn( ScriptNodePtr script ) + : PathColumn(), m_script( script ), m_visibleSet( ScriptNodeAlgo::getVisibleSet( script.get() ) ) { buttonPressSignal().connect( boost::bind( &VisibleSetExclusionsColumn::buttonPress, this, ::_3 ) ); buttonReleaseSignal().connect( boost::bind( &VisibleSetExclusionsColumn::buttonRelease, this, ::_1, ::_2, ::_3 ) ); - m_context->changedSignal().connect( boost::bind( &VisibleSetExclusionsColumn::contextChanged, this, ::_2 ) ); + ScriptNodeAlgo::visibleSetChangedSignal( script.get() ).connect( boost::bind( &VisibleSetExclusionsColumn::visibleSetChanged, this ) ); } CellData cellData( const Gaffer::Path &path, const IECore::Canceller *canceller ) const override @@ -807,16 +809,15 @@ class VisibleSetExclusionsColumn : public PathColumn result.icon = iconData; result.toolTip = g_exclusionToolTip; - const auto visibleSet = ContextAlgo::getVisibleSet( m_context.get() ); - if( visibleSet.exclusions.isEmpty() ) + if( m_visibleSet.exclusions.isEmpty() ) { result.value = new IntData( 0 ); return result; } - Context::Scope scopedContext( m_context.get() ); + Context::Scope scopedContext( setPath->getContext() ); const auto setMembers = setPath->getScene()->set( setName->readable() ); - const auto excludedSetMembers = setMembers->readable().intersection( visibleSet.exclusions ); + const auto excludedSetMembers = setMembers->readable().intersection( m_visibleSet.exclusions ); result.value = new IntData( excludedSetMembers.size() ); if( excludedSetMembers.isEmpty() ) { @@ -833,18 +834,18 @@ class VisibleSetExclusionsColumn : public PathColumn CellData headerData( const IECore::Canceller *canceller ) const override { - const auto visibleSet = ContextAlgo::getVisibleSet( m_context.get() ); - return CellData( /* value = */ nullptr, /* icon = */ visibleSet.exclusions.isEmpty() ? g_exclusionsEmptyIconName : g_setExcludedIconName, /* background = */ nullptr, /* tooltip = */ new StringData( "Visible Set Exclusions" ) ); + return CellData( /* value = */ nullptr, /* icon = */ m_visibleSet.exclusions.isEmpty() ? g_exclusionsEmptyIconName : g_setExcludedIconName, /* background = */ nullptr, /* tooltip = */ new StringData( "Visible Set Exclusions" ) ); } private : - void contextChanged( const IECore::InternedString &name ) + void visibleSetChanged() { - if( ContextAlgo::affectsVisibleSet( name ) ) - { - changedSignal()( this ); - } + // We take a copy, because `cellData()` is called from background threads, + // and it's not safe to call `getVisibleSet()` concurrently with modifications + // on the foreground thread. + m_visibleSet = ScriptNodeAlgo::getVisibleSet( m_script.get() ); + changedSignal()( this ); } bool buttonPress( const ButtonEvent &event ) @@ -872,7 +873,7 @@ class VisibleSetExclusionsColumn : public PathColumn return false; } - Context::Scope scopedContext( m_context.get() ); + Context::Scope scopedContext( setPath->getContext() ); const auto setMembers = setPath->getScene()->set( setName->readable() ); auto pathsToExclude = IECore::PathMatcher( setMembers->readable() ); const auto selection = widget.getSelection(); @@ -896,7 +897,7 @@ class VisibleSetExclusionsColumn : public PathColumn } bool update = false; - auto visibleSet = ContextAlgo::getVisibleSet( m_context.get() ); + auto visibleSet = m_visibleSet; if( event.button == ButtonEvent::Left && !event.modifiers ) { const auto excludedSetMembers = setMembers->readable().intersection( visibleSet.exclusions ); @@ -916,13 +917,14 @@ class VisibleSetExclusionsColumn : public PathColumn if( update ) { - ContextAlgo::setVisibleSet( m_context.get(), visibleSet ); + ScriptNodeAlgo::setVisibleSet( m_script.get(), visibleSet ); } return true; } - ContextPtr m_context; + ScriptNodePtr m_script; + VisibleSet m_visibleSet; static IECore::StringDataPtr g_setExcludedIconName; static IECore::StringDataPtr g_setPartiallyExcludedIconName; @@ -1162,11 +1164,11 @@ void GafferSceneUIModule::bindSetEditor() ; RefCountedClass( "VisibleSetInclusionsColumn" ) - .def( init< ContextPtr >() ) + .def( init() ) ; RefCountedClass( "VisibleSetExclusionsColumn" ) - .def( init< ContextPtr >() ) + .def( init() ) ; }