From 1c0dfe1f8f17575b2ecb7f516107d70572a6b13d Mon Sep 17 00:00:00 2001 From: John Haddon Date: Thu, 5 Sep 2024 15:35:44 +0100 Subject: [PATCH] SetEditor : Prefer ScriptNodeAlgo to ContextAlgo (phase 2) This moves the handling of selection out of SetPath and into the SetEditorEmptySetFilter and SetSelectionColumn, switching to ScriptNodeAlgo at the same time. It felt a bit more natural to have selection be managed in the "presentation layer" rather than in the data (Path) layer itself. But it would also be possible to have kept the original organisation. Can switch back if preferred. --- python/GafferSceneUI/SetEditor.py | 9 +- python/GafferSceneUITest/SetEditorTest.py | 33 +++--- src/GafferSceneUIModule/SetEditorBinding.cpp | 117 ++++++++++++------- 3 files changed, 99 insertions(+), 60 deletions(-) diff --git a/python/GafferSceneUI/SetEditor.py b/python/GafferSceneUI/SetEditor.py index ae89470df9..6c6e420349 100644 --- a/python/GafferSceneUI/SetEditor.py +++ b/python/GafferSceneUI/SetEditor.py @@ -44,7 +44,6 @@ import GafferUI import GafferSceneUI -from . import ContextAlgo from . import _GafferSceneUI class SetEditor( GafferSceneUI.SceneEditor ) : @@ -56,11 +55,11 @@ def __init__( self, scriptNode, **kw ) : GafferSceneUI.SceneEditor.__init__( self, mainColumn, scriptNode, **kw ) searchFilter = _GafferSceneUI._SetEditor.SearchFilter() - emptySetFilter = _GafferSceneUI._SetEditor.EmptySetFilter() + emptySetFilter = _GafferSceneUI._SetEditor.EmptySetFilter( scriptNode ) emptySetFilter.userData()["UI"] = { "label" : "Hide Empty Members", "toolTip" : "Hide sets with no members" } emptySetFilter.setEnabled( False ) - emptySelectionFilter = _GafferSceneUI._SetEditor.EmptySetFilter( propertyName = "setPath:selectedMemberCount" ) + emptySelectionFilter = _GafferSceneUI._SetEditor.EmptySetFilter( scriptNode, useSelection = True ) emptySelectionFilter.userData()["UI"] = { "label" : "Hide Empty Selection", "toolTip" : "Hide sets with no selected members or descendants" } emptySelectionFilter.setEnabled( False ) @@ -75,7 +74,7 @@ def __init__( self, scriptNode, **kw ) : GafferUI.BasicPathFilterWidget( emptySelectionFilter ) self.__setMembersColumn = _GafferSceneUI._SetEditor.SetMembersColumn() - self.__selectedSetMembersColumn = _GafferSceneUI._SetEditor.SetSelectionColumn() + self.__selectedSetMembersColumn = _GafferSceneUI._SetEditor.SetSelectionColumn( scriptNode ) self.__includedSetMembersColumn = _GafferSceneUI._SetEditor.VisibleSetInclusionsColumn( scriptNode ) self.__excludedSetMembersColumn = _GafferSceneUI._SetEditor.VisibleSetExclusionsColumn( scriptNode ) self.__pathListing = GafferUI.PathListingWidget( @@ -104,7 +103,7 @@ def __repr__( self ) : def _updateFromContext( self, modifiedItems ) : - if any( not i.startswith( "ui:" ) for i in modifiedItems ) or any( ContextAlgo.affectsSelectedPaths( i ) for i in modifiedItems ) : + if any( not i.startswith( "ui:" ) for i in modifiedItems ) : self.__updatePathListingPath() @GafferUI.LazyMethod( deferUntilPlaybackStops = True ) diff --git a/python/GafferSceneUITest/SetEditorTest.py b/python/GafferSceneUITest/SetEditorTest.py index 71380b7eb6..dd9fbd3f4f 100644 --- a/python/GafferSceneUITest/SetEditorTest.py +++ b/python/GafferSceneUITest/SetEditorTest.py @@ -183,7 +183,7 @@ def testSetPathMemberCount( self ) : path.setFromString( parent ) self.assertEqual( path.property( "setPath:memberCount" ), count ) - def testSetPathSelectedMemberCount( self ) : + def testSetSelectionColumn( self ) : script = Gaffer.ScriptNode() @@ -203,6 +203,8 @@ def testSetPathSelectedMemberCount( self ) : self.assertTrue( path.isValid() ) self.assertFalse( path.isLeaf() ) + column = _GafferSceneUI._SetEditor.SetSelectionColumn( script ) + for parent, selection, count in [ ( "/", [], None ), ( "/", [ "/plane" ], None ), @@ -232,9 +234,9 @@ def testSetPathSelectedMemberCount( self ) : path.setFromString( parent ) GafferSceneUI.ScriptNodeAlgo.setSelectedPaths( script, IECore.PathMatcher( selection ) ) - self.assertEqual( path.property( "setPath:selectedMemberCount" ), count ) + self.assertEqual( column.cellData( path ).value, count ) - def testSetPathSelectedMemberCountWithInheritance( self ) : + def testSetSelectionColumnWithInheritance( self ) : script = Gaffer.ScriptNode() @@ -268,6 +270,8 @@ def testSetPathSelectedMemberCountWithInheritance( self ) : self.assertTrue( path.isValid() ) self.assertFalse( path.isLeaf() ) + column = _GafferSceneUI._SetEditor.SetSelectionColumn( script ) + for parent, selection, count in [ ( "/", [], None ), ( "/", [ "/group" ], None ), @@ -290,7 +294,7 @@ def testSetPathSelectedMemberCountWithInheritance( self ) : path.setFromString( parent ) GafferSceneUI.ScriptNodeAlgo.setSelectedPaths( script, IECore.PathMatcher( selection ) ) - self.assertEqual( path.property( "setPath:selectedMemberCount" ), count ) + self.assertEqual( column.cellData( path ).value, count ) def testSetPathCancellation( self ) : @@ -351,19 +355,20 @@ def testSearchFilterRemovesEmptyParents( self ) : def testEmptySetFilter( self ) : - plane = GafferScene.Plane() - plane["sets"].setValue( "A A:E B C D" ) + script = Gaffer.ScriptNode() - emptySet = GafferScene.Set() - emptySet["name"].setValue( "EMPTY A:EMPTY" ) - emptySet["in"].setInput( plane["out"] ) + script["plane"] = GafferScene.Plane() + script["plane"]["sets"].setValue( "A A:E B C D" ) - context = Gaffer.Context() - path = _GafferSceneUI._SetEditor.SetPath( emptySet["out"], context, "/" ) + script["emptySet"] = GafferScene.Set() + script["emptySet"]["name"].setValue( "EMPTY A:EMPTY" ) + script["emptySet"]["in"].setInput( script["plane"]["out"] ) + + path = _GafferSceneUI._SetEditor.SetPath( script["emptySet"]["out"], script.context(), "/" ) self.assertEqual( [ str( c ) for c in path.children() ], [ "/A", "/B", "/C", "/D", "/EMPTY" ] ) - emptySetFilter = _GafferSceneUI._SetEditor.EmptySetFilter() + emptySetFilter = _GafferSceneUI._SetEditor.EmptySetFilter( script ) path.setFilter( emptySetFilter ) self.assertEqual( [ str( c ) for c in path.children() ], [ "/A", "/B", "/C", "/D" ] ) @@ -378,7 +383,7 @@ def testEmptySetFilter( self ) : emptySetFilter.setEnabled( True ) self.assertEqual( [ str( c ) for c in path.children() ], [ "/A/A:E" ] ) - def testEmptySetFilterWithSelectedMemberCount( self ) : + def testEmptySetFilterUsingSelection( self ) : script = Gaffer.ScriptNode() @@ -402,7 +407,7 @@ def testEmptySetFilterWithSelectedMemberCount( self ) : self.assertEqual( [ str( c ) for c in path.children() ], [ "/A", "/B", "/C", "/D", "/EMPTY", "/F" ] ) - emptySetFilter = _GafferSceneUI._SetEditor.EmptySetFilter( propertyName = "setPath:selectedMemberCount" ) + emptySetFilter = _GafferSceneUI._SetEditor.EmptySetFilter( script, useSelection = True ) path.setFilter( emptySetFilter ) GafferSceneUI.ScriptNodeAlgo.setSelectedPaths( script, IECore.PathMatcher( [ "/plane" ] ) ) diff --git a/src/GafferSceneUIModule/SetEditorBinding.cpp b/src/GafferSceneUIModule/SetEditorBinding.cpp index 583536899e..9c3c870353 100644 --- a/src/GafferSceneUIModule/SetEditorBinding.cpp +++ b/src/GafferSceneUIModule/SetEditorBinding.cpp @@ -38,7 +38,6 @@ #include "SetEditorBinding.h" -#include "GafferSceneUI/ContextAlgo.h" #include "GafferSceneUI/ScriptNodeAlgo.h" #include "GafferSceneUI/TypeIds.h" @@ -58,6 +57,7 @@ #include "IECorePython/RefCountedBinding.h" +#include "IECore/PathMatcherData.h" #include "IECore/StringAlgo.h" #include "boost/algorithm/string/predicate.hpp" @@ -104,6 +104,19 @@ Path::Names parent( InternedString setName ) } } +int numSelected( const PathMatcher &set, const PathMatcher &selectedPaths ) +{ + int result = 0; + // Consider inheritance in selected member count so descendants + // of set members are included in the count + for( PathMatcher::Iterator it = set.begin(), eIt = set.end(); it != eIt; ++it ) + { + result += selectedPaths.subTree( *it ).size(); + it.prune(); + } + return result; +} + ////////////////////////////////////////////////////////////////////////// // LRU cache of PathMatchers built from set names ////////////////////////////////////////////////////////////////////////// @@ -152,7 +165,7 @@ PathMatcherCache g_pathMatcherCache( pathMatcherCacheGetter, 25 ); const InternedString g_setNamePropertyName( "setPath:setName" ); const InternedString g_memberCountPropertyName( "setPath:memberCount" ); -const InternedString g_selectedMemberCountPropertyName( "setPath:selectedMemberCount" ); +const InternedString g_setPropertyName( "setPath:set" ); ////////////////////////////////////////////////////////////////////////// // SetPath @@ -257,7 +270,7 @@ class SetPath : public Gaffer::Path Path::propertyNames( names, canceller ); names.push_back( g_setNamePropertyName ); names.push_back( g_memberCountPropertyName ); - names.push_back( g_selectedMemberCountPropertyName ); + names.push_back( g_setPropertyName ); } IECore::ConstRunTimeTypedPtr property( const IECore::InternedString &name, const IECore::Canceller *canceller = nullptr ) const override @@ -270,7 +283,7 @@ class SetPath : public Gaffer::Path return new StringData( names().back().string() ); } } - else if( name == g_memberCountPropertyName ) + else if( name == g_setPropertyName ) { const PathMatcher p = pathMatcher( canceller ); if( p.match( names() ) & PathMatcher::ExactMatch ) @@ -280,33 +293,17 @@ class SetPath : public Gaffer::Path { scopedContext.setCanceller( canceller ); } - const auto setMembers = getScene()->set( names().back().string() ); - return new IntData( setMembers->readable().size() ); + return getScene()->set( names().back().string() ); } } - else if( name == g_selectedMemberCountPropertyName ) + else if( name == g_memberCountPropertyName ) { - const PathMatcher p = pathMatcher( canceller ); - if( p.match( names() ) & PathMatcher::ExactMatch ) + if( auto set = runTimeCast( property( g_setPropertyName, canceller ) ) ) { - Context::EditableScope scopedContext( getContext() ); - if( canceller ) - { - scopedContext.setCanceller( canceller ); - } - const auto setMembers = getScene()->set( names().back().string() ); - const auto selectedPaths = ContextAlgo::getSelectedPaths( Context::current() ); - int memberCount = 0; - // Consider inheritance in selected member count so descendants - // of set members are included in the count - for( PathMatcher::Iterator it = setMembers->readable().begin(), eIt = setMembers->readable().end(); it != eIt; ++it ) - { - memberCount += selectedPaths.subTree( *it ).size(); - it.prune(); - } - return new IntData( memberCount ); + return new IntData( set->readable().size() ); } } + return Path::property( name, canceller ); } @@ -483,28 +480,31 @@ class SetMembersColumn : public StandardPathColumn // SetSelectionColumn ////////////////////////////////////////////////////////////////////////// -class SetSelectionColumn : public StandardPathColumn +class SetSelectionColumn : public PathColumn { public : IE_CORE_DECLAREMEMBERPTR( SetSelectionColumn ) - SetSelectionColumn() - : StandardPathColumn( "Selection", g_selectedMemberCountPropertyName ) + SetSelectionColumn( ScriptNodePtr script ) + : PathColumn(), m_script( script ), m_selectedPaths( ScriptNodeAlgo::getSelectedPaths( script.get() ) ) { + ScriptNodeAlgo::selectedPathsChangedSignal( script.get() ).connect( + boost::bind( &SetSelectionColumn::selectedPathsChanged, this ) + ); } CellData cellData( const Gaffer::Path &path, const IECore::Canceller *canceller ) const override { - CellData result = StandardPathColumn::cellData( path, canceller ); - if( const auto setName = runTimeCast( path.property( g_setNamePropertyName, canceller ) ) ) + CellData result; + if( const auto set = runTimeCast( path.property( g_setPropertyName, canceller ) ) ) { - const auto selectedMemberCount = runTimeCast( path.property( g_selectedMemberCountPropertyName, canceller ) ); - const auto count = selectedMemberCount ? selectedMemberCount->readable() : 0; + int count = numSelected( set->readable(), m_selectedPaths ); + result.value = new IntData( count ); result.toolTip = new IECore::StringData( fmt::format( "{} selected scene location{} of {}", - count, count == 1 ? " is a member or descendant" : "s are members and/or descendants", setName->readable() + count, count == 1 ? " is a member or descendant" : "s are members and/or descendants", path.names().back().string() ) ); } @@ -514,17 +514,29 @@ class SetSelectionColumn : public StandardPathColumn CellData headerData( const IECore::Canceller *canceller ) const override { - CellData result = StandardPathColumn::headerData( canceller ); + CellData result; + result.value = g_headerValue; result.toolTip = g_headerToolTip; return result; } private : + void selectedPathsChanged() + { + m_selectedPaths = ScriptNodeAlgo::getSelectedPaths( m_script.get() ); + changedSignal()( this ); + } + + ScriptNodePtr m_script; + PathMatcher m_selectedPaths; + + static const ConstStringDataPtr g_headerValue; static const ConstStringDataPtr g_headerToolTip; }; +const ConstStringDataPtr SetSelectionColumn::g_headerValue = new StringData( "Selected" ); const ConstStringDataPtr SetSelectionColumn::g_headerToolTip = new StringData( "The number of selected scene locations that are set members, or their descendants" ); ////////////////////////////////////////////////////////////////////////// @@ -1047,9 +1059,16 @@ class SetEditorEmptySetFilter : public Gaffer::PathFilter IE_CORE_DECLAREMEMBERPTR( SetEditorEmptySetFilter ) - SetEditorEmptySetFilter( IECore::CompoundDataPtr userData = nullptr, const std::string &propertyName = g_memberCountPropertyName ) - : PathFilter( userData ), m_propertyName( propertyName ) + SetEditorEmptySetFilter( ScriptNodePtr scriptNode, bool useSelection = false, IECore::CompoundDataPtr userData = nullptr ) + : PathFilter( userData ), m_scriptNode( scriptNode ) { + if( useSelection ) + { + m_selectedPathsChangedConnection = ScriptNodeAlgo::selectedPathsChangedSignal( m_scriptNode.get() ).connect( + boost::bind( &SetEditorEmptySetFilter::selectedPathsChanged, this ) + ); + m_selectedPaths = ScriptNodeAlgo::getSelectedPaths( scriptNode.get() ); + } } void doFilter( std::vector &paths, const IECore::Canceller *canceller ) const override @@ -1081,9 +1100,16 @@ class SetEditorEmptySetFilter : public Gaffer::PathFilter } bool members = false; - if( const auto memberCountData = IECore::runTimeCast( path->property( m_propertyName, canceller ) ) ) + if( auto set = runTimeCast( path->property( g_setPropertyName ) ) ) { - members = memberCountData->readable() > 0; + if( m_selectedPathsChangedConnection.connected() ) + { + members = numSelected( set->readable(), m_selectedPaths ); + } + else + { + members = set->readable().size() > 0 ; + } } return leaf && !members; @@ -1091,7 +1117,16 @@ class SetEditorEmptySetFilter : public Gaffer::PathFilter private : - const InternedString m_propertyName; + void selectedPathsChanged() + { + m_selectedPaths = ScriptNodeAlgo::getSelectedPaths( m_scriptNode.get() ); + changedSignal()( this ); + } + + ScriptNodePtr m_scriptNode; + // Only connected if using selection. + Gaffer::Signals::ScopedConnection m_selectedPathsChangedConnection; + PathMatcher m_selectedPaths; }; @@ -1148,7 +1183,7 @@ void GafferSceneUIModule::bindSetEditor() ; RefCountedClass( "EmptySetFilter" ) - .def( init( ( boost::python::arg( "userData" ) = object(), boost::python::arg( "propertyName" ) = g_memberCountPropertyName ) ) ) + .def( init( ( boost::python::arg( "scriptNode" ), boost::python::arg( "useSelection" ) = false, boost::python::arg( "userData" ) = object() ) ) ) ; RefCountedClass( "SetNameColumn" ) @@ -1160,7 +1195,7 @@ void GafferSceneUIModule::bindSetEditor() ; RefCountedClass( "SetSelectionColumn" ) - .def( init<>() ) + .def( init() ) ; RefCountedClass( "VisibleSetInclusionsColumn" )