From cc9cca158f74a5f943727a32294dc81d96caf038 Mon Sep 17 00:00:00 2001 From: Murray Stevenson <50844517+murraystevenson@users.noreply.github.com> Date: Thu, 12 Sep 2024 18:04:20 -0700 Subject: [PATCH] Inspector, TransformTool : Do not edit EditScopes when target is "None" --- Changes.md | 1 + .../AttributeInspectorTest.py | 15 +++++++++++++ .../GafferSceneUITest/OptionInspectorTest.py | 15 +++++++++++++ .../ParameterInspectorTest.py | 13 +++++++++++ .../SetMembershipInspectorTest.py | 17 +++++++++++++- python/GafferSceneUITest/TransformToolTest.py | 22 +++++++++++++++++++ python/GafferSceneUITest/TranslateToolTest.py | 18 +++++---------- python/GafferUI/EditScopeUI.py | 2 +- src/GafferSceneUI/Inspector.cpp | 16 +++++++++++++- src/GafferSceneUI/TransformTool.cpp | 19 ++++++++++++---- 10 files changed, 119 insertions(+), 19 deletions(-) diff --git a/Changes.md b/Changes.md index d7b6e155bcf..22f6dbdef9d 100644 --- a/Changes.md +++ b/Changes.md @@ -29,6 +29,7 @@ Improvements - PlugLayout : Summaries and activators are now evaluated in a context determined relative to the focus node. - Editor : The node graph is now evaluated in a context determined relative to the focus node. - LightEditor, RenderPassEditor : The "Disable Edit" right-click menu item and D shortcut now act as a toggle, where edits disabled in the current session via these actions can be reenabled with D or by selecting "Reenable Edit" from the right-click menu. +- EditScope : Setting a Viewer or Editor's target edit scope to "None" will now prevent edits from being made within any upstream edit scope. To make edits in an edit scope, it must be set as the target. Fixes ----- diff --git a/python/GafferSceneUITest/AttributeInspectorTest.py b/python/GafferSceneUITest/AttributeInspectorTest.py index 2c2ab6b8e7d..2c99d5868b8 100644 --- a/python/GafferSceneUITest/AttributeInspectorTest.py +++ b/python/GafferSceneUITest/AttributeInspectorTest.py @@ -318,6 +318,16 @@ def testSourceAndEdits( self ) : edit = editScopeAttributeTweak ) + # When using no scope, make sure that we don't inadvertently edit the contents of an EditScope. + + self.__assertExpectedResult( + self.__inspect( s["editScope2"]["out"], "/light2", "gl:visualiser:scale", None ), + source = editScopeAttributeTweak, + sourceType = SourceType.Other, + editable = False, + nonEditableReason = "Source is in an EditScope. Change scope to editScope1 to edit." + ) + # If there is a manual tweak outside of an edit scope, make sure we use that with no scope s["independentAttributeTweak"] = GafferScene.AttributeTweaks() s["independentAttributeTweak"]["in"].setInput( s["editScope2"]["out"] ) @@ -895,6 +905,11 @@ def testDisableEdit( self ) : self.assertFalse( inspection.canDisableEdit() ) self.assertEqual( inspection.nonDisableableReason(), "The target edit scope editScope2 is not in the scene history." ) + inspection = self.__inspect( s["editScope2"]["out"], "/group/light", "gl:visualiser:scale", None ) + self.assertFalse( inspection.canDisableEdit() ) + self.assertEqual( inspection.nonDisableableReason(), "Source is in an EditScope. Change scope to editScope1 to disable." ) + self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : Source is in an EditScope. Change scope to editScope1 to disable.", inspection.disableEdit ) + inspection = self.__inspect( s["editScope2"]["out"], "/group/light", "gl:visualiser:scale", s["editScope2"] ) self.assertFalse( inspection.canDisableEdit() ) self.assertEqual( inspection.nonDisableableReason(), "Edit is not in the current edit scope. Change scope to editScope1 to disable." ) diff --git a/python/GafferSceneUITest/OptionInspectorTest.py b/python/GafferSceneUITest/OptionInspectorTest.py index 016a61967e7..118332b9741 100644 --- a/python/GafferSceneUITest/OptionInspectorTest.py +++ b/python/GafferSceneUITest/OptionInspectorTest.py @@ -651,6 +651,16 @@ def testRenderPassSourceAndEdits( self ) : edit = s["editScope1"]["standardOptions3"]["options"]["renderCamera"] ) + # When using no scope, make sure that we don't inadvertently edit the contents of an EditScope. + + self.__assertExpectedResult( + self.__inspect( s["editScope2"]["out"], "render:camera", None, context ), + source = s["editScope1"]["standardOptions3"]["options"]["renderCamera"], + sourceType = SourceType.Other, + editable = False, + nonEditableReason = "Source is in an EditScope. Change scope to editScope1 to edit." + ) + # If there is a StandardOptions node outside of an edit scope, make sure we use that with no scope s["independentOptions"] = GafferScene.StandardOptions() @@ -969,6 +979,11 @@ def testDisableEdit( self ) : self.assertFalse( inspection.canDisableEdit() ) self.assertEqual( inspection.nonDisableableReason(), "The target edit scope editScope2 is not in the scene history." ) + inspection = self.__inspect( s["editScope2"]["out"], "render:camera", None ) + self.assertFalse( inspection.canDisableEdit() ) + self.assertEqual( inspection.nonDisableableReason(), "Source is in an EditScope. Change scope to editScope1 to disable." ) + self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : Source is in an EditScope. Change scope to editScope1 to disable.", inspection.disableEdit ) + inspection = self.__inspect( s["editScope2"]["out"], "render:camera", s["editScope2"] ) self.assertFalse( inspection.canDisableEdit() ) self.assertEqual( inspection.nonDisableableReason(), "Edit is not in the current edit scope. Change scope to editScope1 to disable." ) diff --git a/python/GafferSceneUITest/ParameterInspectorTest.py b/python/GafferSceneUITest/ParameterInspectorTest.py index f37651b572c..7cece6205ce 100644 --- a/python/GafferSceneUITest/ParameterInspectorTest.py +++ b/python/GafferSceneUITest/ParameterInspectorTest.py @@ -247,6 +247,14 @@ def testSourceAndEdits( self ) : editable = True, edit = editScopeShaderTweak ) + # When using no scope, make sure that we don't inadvertently edit the contents of an EditScope. + + self.__assertExpectedResult( + self.__inspect( s["editScope2"]["out"], "/light2", "intensity", None ), + source = editScopeShaderTweak, sourceType = SourceType.Other, + editable = False, nonEditableReason = "Source is in an EditScope. Change scope to editScope1 to edit." + ) + # If there is a manual tweak outside of an edit scope make sure we use that with no scope s["independentLightTweak"] = GafferScene.ShaderTweaks() @@ -469,6 +477,11 @@ def testDisableEdit( self ) : self.assertFalse( inspection.canDisableEdit() ) self.assertEqual( inspection.nonDisableableReason(), "The target edit scope editScope2 is not in the scene history." ) + inspection = self.__inspect( s["editScope2"]["out"], "/light", "exposure", None ) + self.assertFalse( inspection.canDisableEdit() ) + self.assertEqual( inspection.nonDisableableReason(), "Source is in an EditScope. Change scope to editScope to disable." ) + self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : Source is in an EditScope. Change scope to editScope to disable.", inspection.disableEdit ) + inspection = self.__inspect( s["editScope2"]["out"], "/light", "exposure", s["editScope2"] ) self.assertFalse( inspection.canDisableEdit() ) self.assertEqual( inspection.nonDisableableReason(), "Edit is not in the current edit scope. Change scope to editScope to disable." ) diff --git a/python/GafferSceneUITest/SetMembershipInspectorTest.py b/python/GafferSceneUITest/SetMembershipInspectorTest.py index d16f991cf20..ee18066a440 100644 --- a/python/GafferSceneUITest/SetMembershipInspectorTest.py +++ b/python/GafferSceneUITest/SetMembershipInspectorTest.py @@ -293,6 +293,16 @@ def testSourceAndEdits( self ) : edit = s["editScope1"]["setPlane2"]["name"] ) + # When using no scope, make sure that we don't inadvertently edit the contents of an EditScope. + + self.__assertExpectedResult( + self.__inspect( s["editScope2"]["out"], "/plane2", "planeSet", None ), + source = s["editScope1"]["setPlane2"]["name"], + sourceType = SourceType.Other, + editable = False, + nonEditableReason = "Source is in an EditScope. Change scope to editScope1 to edit." + ) + # If there is a manual set node outside of an edit scope, make sure we use that with no scope s["independentSet"] = GafferScene.Set() s["independentSet"]["name"].setValue( "planeSet" ) @@ -673,13 +683,18 @@ def testDisableEdit( self ) : ) Gaffer.MetadataAlgo.setReadOnly( s["editScope1"], True ) - inspection = self.__inspect( s["editScope1"]["out"], "/group/plane", "planeSetEditScope", None ) + inspection = self.__inspect( s["editScope1"]["out"], "/group/plane", "planeSetEditScope", s["editScope1"] ) self.assertFalse( inspection.canDisableEdit() ) self.assertEqual( inspection.nonDisableableReason(), "editScope1 is locked." ) self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : editScope1 is locked.", inspection.disableEdit ) Gaffer.MetadataAlgo.setReadOnly( s["editScope1"], False ) inspection = self.__inspect( s["editScope1"]["out"], "/group/plane", "planeSetEditScope", None ) + self.assertFalse( inspection.canDisableEdit() ) + self.assertEqual( inspection.nonDisableableReason(), "Source is in an EditScope. Change scope to editScope1 to disable." ) + self.assertRaisesRegex( IECore.Exception, "Cannot disable edit : Source is in an EditScope. Change scope to editScope1 to disable.", inspection.disableEdit ) + + inspection = self.__inspect( s["editScope1"]["out"], "/group/plane", "planeSetEditScope", s["editScope1"] ) self.assertTrue( inspection.canDisableEdit() ) self.assertEqual( inspection.nonDisableableReason(), "" ) diff --git a/python/GafferSceneUITest/TransformToolTest.py b/python/GafferSceneUITest/TransformToolTest.py index 4183104c519..e4d47122765 100644 --- a/python/GafferSceneUITest/TransformToolTest.py +++ b/python/GafferSceneUITest/TransformToolTest.py @@ -333,5 +333,27 @@ def testDontEditUpstreamOfReference( self ) : self.assertEqual( selection.warning(), "Transform is locked as it is inside \"reference\" which disallows edits to its children" ) self.assertFalse( selection.editable() ) + def testDisallowEditingWithEditScopeNone( self ) : + + script = Gaffer.ScriptNode() + + script["plane"] = GafferScene.Plane() + + script["editScope"] = Gaffer.EditScope() + script["editScope"].setup( script["plane"]["out"] ) + script["editScope"]["in"].setInput( script["plane"]["out"] ) + + selection = GafferSceneUI.TransformTool.Selection( script["editScope"]["out"], "/plane", script.context(), script["editScope"] ) + edit = selection.acquireTransformEdit() + self.assertTrue( script["editScope"].isAncestorOf( edit.translate ) ) + + selection = GafferSceneUI.TransformTool.Selection( script["editScope"]["out"], "/plane", script.context(), None ) + self.assertFalse( selection.editable() ) + self.assertEqual( selection.warning(), "Source is in an EditScope. Change scope to editScope to edit" ) + + selection = GafferSceneUI.TransformTool.Selection( script["editScope"]["out"], "/plane", script.context(), script["editScope"] ) + self.assertTrue( selection.editable() ) + self.assertEqual( selection.warning(), "" ) + if __name__ == "__main__": unittest.main() diff --git a/python/GafferSceneUITest/TranslateToolTest.py b/python/GafferSceneUITest/TranslateToolTest.py index 95d33f8de37..c2c7a44a658 100644 --- a/python/GafferSceneUITest/TranslateToolTest.py +++ b/python/GafferSceneUITest/TranslateToolTest.py @@ -1043,26 +1043,20 @@ def testTransformInEditScopeButEditScopeOff( self ) : GafferSceneUI.ScriptNodeAlgo.setSelectedPaths( script, IECore.PathMatcher( [ "/sphere" ] ) ) - # We want the TranslateTool to pick up and use that edit - # even if we haven't told it to use that EditScope. + # We don't want the TranslateTool to pick up and use that edit + # as we haven't told it to use that EditScope. tool = GafferSceneUI.TranslateTool( view ) tool["active"].setValue( True ) - self.assertEqual( tool.handlesTransform(), imath.M44f().translate( imath.V3f( 1, 0, 0 ) ) ) self.assertEqual( len( tool.selection() ), 1 ) - self.assertTrue( tool.selectionEditable() ) - self.assertTrue( tool.selection()[0].editable() ) - self.assertEqual( tool.selection()[0].acquireTransformEdit( createIfNecessary = False ), transformEdit ) + self.assertFalse( tool.selectionEditable() ) + self.assertFalse( tool.selection()[0].editable() ) self.assertEqual( - tool.selection()[0].editTarget(), - transformEdit.translate.ancestor( Gaffer.Spreadsheet.RowPlug ) + tool.selection()[0].warning(), + "Source is in an EditScope. Change scope to editScope to edit" ) - tool.translate( imath.V3f( 0, 1, 0 ) ) - self.assertEqual( tool.handlesTransform(), imath.M44f().translate( imath.V3f( 1, 1, 0 ) ) ) - self.assertEqual( transformEdit.translate.getValue(), imath.V3f( 1, 1, 0 ) ) - def testNonEditableSelections( self ) : script = Gaffer.ScriptNode() diff --git a/python/GafferUI/EditScopeUI.py b/python/GafferUI/EditScopeUI.py index 0a8468267c8..d11dd83df0f 100644 --- a/python/GafferUI/EditScopeUI.py +++ b/python/GafferUI/EditScopeUI.py @@ -169,7 +169,7 @@ def getToolTip( self ) : editScope = self.__editScope() if editScope is None : - return "Edits will be made using the last relevant node, including nodes not in any EditScope." + return "Edits will be made using the last relevant node found outside of an edit scope.\n\nTo make an edit in an edit scope, choose it from the menu." unusableReason = self.__unusableReason( editScope ) if unusableReason : diff --git a/src/GafferSceneUI/Inspector.cpp b/src/GafferSceneUI/Inspector.cpp index 8ce8c28f619..03d426da761 100644 --- a/src/GafferSceneUI/Inspector.cpp +++ b/src/GafferSceneUI/Inspector.cpp @@ -289,7 +289,21 @@ void Inspector::inspectHistoryWalk( const GafferScene::SceneAlgo::History *histo // See if we can use it for editing - if( !result->m_editScope || node->ancestor() == result->m_editScope ) + if( !result->m_editScope && node->ancestor() ) + { + // We don't allow editing if the user hasn't requested a specific scope + // (they have selected "None" from the Menu) and the upstream edit is + // inside _any_ EditScope. + result->m_editFunction = fmt::format( + "Source is in an EditScope. Change scope to {} to edit.", + node->ancestor()->relativeName( node->ancestor()->scriptNode() ) + ); + result->m_disableEditFunction = fmt::format( + "Source is in an EditScope. Change scope to {} to disable.", + node->ancestor()->relativeName( node->ancestor()->scriptNode() ) + ); + } + else if( !result->m_editScope || node->ancestor() == result->m_editScope ) { const std::string nonEditableReason = ::nonEditableReason( result->m_source.get() ); diff --git a/src/GafferSceneUI/TransformTool.cpp b/src/GafferSceneUI/TransformTool.cpp index 0ceb4f75429..bbb3acc3541 100644 --- a/src/GafferSceneUI/TransformTool.cpp +++ b/src/GafferSceneUI/TransformTool.cpp @@ -471,11 +471,22 @@ void TransformTool::Selection::initWalk( const GafferScene::SceneAlgo::History * // First, check for a supported node in this history entry initFromSceneNode( history ); - // If we found a node to edit here and the user has requested a - // specific scope, check if the edit is in it. - if( m_upstreamScene && m_editScope ) + if( m_upstreamScene ) { - editScopeFound = m_upstreamScene->ancestor() == m_editScope; + const auto upstreamEditScope = m_upstreamScene->ancestor(); + if( m_editScope ) + { + // If we found a node to edit here and the user has requested a + // specific scope, check if the edit is in it. + editScopeFound = upstreamEditScope == m_editScope; + } + else if( upstreamEditScope ) + { + // We don't allow editing if the user hasn't requested a specific scope + // and the upstream edit is inside an EditScope. + m_warning = "Source is in an EditScope. Change scope to " + displayName( upstreamEditScope ) + " to edit"; + m_editable = false; + } } }