diff --git a/Changes.md b/Changes.md index 6a895d4ede9..d03b99803f8 100644 --- a/Changes.md +++ b/Changes.md @@ -54,6 +54,7 @@ Improvements - Checkerboard : Optimized image generation when rotation is 0. - InteractiveRender : Added a warning when attribute edits require geometry to be regenerated, as this can have a performance impact. Examples in Arnold include subdivision changes or changes to attributes inherited by procedurals. - NodeMenu : Removed Loop node. This node can have severe consequences for performance if used inappropriately. Depending on the use case, the Collect nodes and others often provide a more performant alternative. The Loop node can still be created via the scripting API, but we recommend you consider the alternatives and/or request advice before using it. +- TransformTool : Improved tool state messaging to include node names and/or paths. Fixes ----- @@ -65,6 +66,7 @@ Fixes - Fixed handling of interpolation for normals. - Fixed writing of indexed primitive variables to non-indexed USD attributes. - Fixed handling of GeometricInterpretation/Role. +- TransformTools : Fixed bug finding existing nodes inside EditScopes. - Viewer : Fixed bug that caused mouse clicks in empty toolbar regions to be ignored. - PlugAlgo/BoxIO : Fixed bug handling nested compound plugs. - Resample : Fixed hash of intermediate `deep` plug. diff --git a/include/Gaffer/EditScope.h b/include/Gaffer/EditScope.h index 07bd48a7767..e8bd5c333b6 100644 --- a/include/Gaffer/EditScope.h +++ b/include/Gaffer/EditScope.h @@ -44,8 +44,27 @@ namespace Gaffer class BoxOut; -/// A container node for interactive tools to make nodes -/// in as necessary. +/// A container node for interactive tools to make nodes in as necessary. +/// +/// EditScopes and Tools +/// ==================== +/// +/// Tools that affect change by modifying nodes/plugs in the Node Graph +/// should use the following logic to determine their edit target: +/// +/// - If no EditScope has been selected, use the last (closest) upstream +/// target. +/// +/// - If an EditScope has been selected, prefer existing targets inside +/// the EditScope over using the EditScopeAlgo to acquire a new target. +/// +/// - If an EditScope has been selected, but is upstream of another target +/// either error (if overrides preclude editing), or allow editing with +/// a suitable warning identifying the last downstream target. +/// +/// - If an EditScope has been selected but is not in the scene history, +/// error. +/// class GAFFER_API EditScope : public Box { diff --git a/include/GafferSceneUI/TransformTool.h b/include/GafferSceneUI/TransformTool.h index 3e7394d9e30..9c00db9c831 100644 --- a/include/GafferSceneUI/TransformTool.h +++ b/include/GafferSceneUI/TransformTool.h @@ -169,8 +169,16 @@ class GAFFERSCENEUI_API TransformTool : public GafferSceneUI::SelectionTool private : + void initFromHistory( const GafferScene::SceneAlgo::History *history ); void initFromSceneNode( const GafferScene::SceneAlgo::History *history ); - void initWalk( const GafferScene::SceneAlgo::History *history, bool &editScopeFound ); + void initFromEditScope( const GafferScene::SceneAlgo::History *history ); + void initWalk( + const GafferScene::SceneAlgo::History *history, + bool &editScopeFound, + const GafferScene::SceneAlgo::History *editScopeOutHistory = nullptr + ); + bool initRequirementsSatisfied( bool editScopeFound ); + void throwIfNotEditable() const; Imath::M44f transformToLocalSpace() const; @@ -189,6 +197,7 @@ class GAFFERSCENEUI_API TransformTool : public GafferSceneUI::SelectionTool Imath::M44f m_transformSpace; bool m_aimConstraint; + static std::string displayName( const GraphComponent *component ); }; /// Returns the current selection. diff --git a/python/GafferSceneUITest/TransformToolTest.py b/python/GafferSceneUITest/TransformToolTest.py index bc97b2d894c..394f5394ac3 100644 --- a/python/GafferSceneUITest/TransformToolTest.py +++ b/python/GafferSceneUITest/TransformToolTest.py @@ -109,7 +109,7 @@ def testSelectionEditScopes( self ) : selection = GafferSceneUI.TransformTool.Selection( plane["out"], "/plane", Gaffer.Context(), editScope ) self.assertFalse( selection.editable() ) - self.assertEqual( selection.warning(), "EditScope not in history" ) + self.assertEqual( selection.warning(), "The target EditScope \"EditScope\" is not in the scene history" ) self.assertRaises( RuntimeError, selection.acquireTransformEdit ) self.assertRaises( RuntimeError, selection.transformSpace ) self.assertEqual( selection.editScope(), editScope ) @@ -132,7 +132,7 @@ def testSelectionEditScopes( self ) : editScope["enabled"].setValue( False ) selection = GafferSceneUI.TransformTool.Selection( transform["out"], "/plane", Gaffer.Context(), editScope ) self.assertFalse( selection.editable() ) - self.assertEqual( selection.warning(), "EditScope disabled" ) + self.assertEqual( selection.warning(), "The target EditScope \"EditScope\" is disabled" ) self.assertEqual( selection.editScope(), editScope ) editScope["enabled"].setValue( True ) @@ -150,7 +150,7 @@ def testSelectionEditScopes( self ) : selection = GafferSceneUI.TransformTool.Selection( transform["out"], "/plane", Gaffer.Context(), editScope ) self.assertFalse( selection.editable() ) - self.assertEqual( selection.warning(), "EditScope overridden downstream" ) + self.assertEqual( selection.warning(), "The target EditScope \"EditScope\" is overridden downstream by \"Transform\"" ) self.assertEqual( selection.upstreamScene(), transform["out"] ) # Disable the downstream node and we should be back in business. @@ -161,6 +161,59 @@ def testSelectionEditScopes( self ) : self.assertEqual( selection.warning(), "" ) self.assertEqual( selection.upstreamScene(), editScope["out"] ) + # Ensure that we use any existing node within an EditScope in preference + # to creating a new tweak node + + plane2 = GafferScene.Plane() + plane2["name"].setValue( "otherPlane" ) + editScope["plane2"] = plane2 + + editScope["parent"] = GafferScene.Parent() + editScope["parent"]["parent"].setValue( "/" ) + editScope["parent"]["in"].setInput( editScope["BoxOut"]["in"].getInput() ) + editScope["parent"]["children"][0].setInput( plane2["out"] ) + editScope["BoxOut"]["in"].setInput( editScope["parent"]["out"] ) + + selection = GafferSceneUI.TransformTool.Selection( transform["out"], "/otherPlane", Gaffer.Context(), editScope ) + + self.assertTrue( selection.editable() ) + self.assertEqual( selection.warning(), "" ) + self.assertEqual( selection.upstreamScene(), plane2["out"] ) + self.assertEqual( selection.editScope(), editScope ) + self.assertEqual( selection.editTarget(), plane2["transform"] ) + + # Ensure we handle being inside the chosen edit scope + + selection = GafferSceneUI.TransformTool.Selection( editScope["parent"]["out"], "/plane", Gaffer.Context(), editScope ) + self.assertFalse( selection.editable() ) + self.assertEqual( selection.warning(), "The output of the target EditScope \"EditScope\" is not in the scene history" ) + + def testNestedEditScopes( self ) : + + outerScope = Gaffer.EditScope() + outerScope.setup( GafferScene.ScenePlug() ) + innerScope = Gaffer.EditScope() + outerScope["Inner"] = innerScope + innerScope.setup( outerScope["BoxIn"]["out"] ) + innerScope["in"].setInput( outerScope["BoxIn"]["out"] ) + innerScope["parent"] = GafferScene.Parent() + innerScope["parent"]["parent"].setValue( "/" ) + innerScope["parent"]["in"].setInput( innerScope["BoxIn"]["out"] ) + innerScope["BoxOut"]["in"].setInput( innerScope["parent"]["out"] ) + innerScope["plane"] = GafferScene.Plane() + innerScope["parent"]["children"][0].setInput( innerScope["plane"]["out"] ) + outerScope["BoxOut"]["in"].setInput( innerScope["out"] ) + + selection = GafferSceneUI.TransformTool.Selection( outerScope["out"], "/plane", Gaffer.Context(), innerScope ) + self.assertTrue( selection.editable() ) + self.assertEqual( selection.editTarget(), innerScope["plane"]["transform"] ) + self.assertEqual( selection.editScope(), innerScope ) + + selection = GafferSceneUI.TransformTool.Selection( outerScope["out"], "/plane", Gaffer.Context(), outerScope ) + self.assertTrue( selection.editable() ) + self.assertEqual( selection.editTarget(), outerScope ) + self.assertEqual( selection.editScope(), outerScope ) + def testSceneReaderSelectionEditability( self ) : sceneReader = GafferScene.SceneReader() @@ -203,6 +256,40 @@ def testAcquireTransformEdit( self ) : edit = selection.acquireTransformEdit() self.assertTrue( editScope.isAncestorOf( edit.translate ) ) + # Check readOnly is respected + + plane["name"].setValue( "plane2" ) + + Gaffer.MetadataAlgo.setReadOnly( plane["transform"]["translate"], True ) + + # We still allow 'editing' of existing plugs, but the handles should take care of disabling themselves. + selection = GafferSceneUI.TransformTool.Selection( plane["out"], "/plane2", Gaffer.Context(), None ) + self.assertTrue( selection.editable() ) + self.assertEqual( selection.warning(), "\"Plane.transform.translate\" is locked" ) + + Gaffer.MetadataAlgo.setReadOnly( editScope, True ) + + selection = GafferSceneUI.TransformTool.Selection( editScope["out"], "/plane2", Gaffer.Context(), editScope ) + self.assertFalse( selection.editable() ) + self.assertEqual( selection.warning(), "\"EditScope\" is locked" ) + with six.assertRaisesRegex( self, RuntimeError, "Selection is not editable" ) : + self.assertIsNone( selection.acquireTransformEdit() ) + + Gaffer.MetadataAlgo.setReadOnly( editScope, False ) + + selection = GafferSceneUI.TransformTool.Selection( editScope["out"], "/plane2", Gaffer.Context(), editScope ) + edit = selection.acquireTransformEdit() + + Gaffer.MetadataAlgo.setReadOnly( edit.translate.ancestor( Gaffer.Spreadsheet.RowsPlug ).source(), True ) + + plane["name"].setValue( "plane3" ) + + selection = GafferSceneUI.TransformTool.Selection( editScope["out"], "/plane3", Gaffer.Context(), editScope ) + self.assertFalse( selection.editable() ) + self.assertEqual( selection.warning(), "\"EditScope.TransformEdits.edits\" is locked" ) + with six.assertRaisesRegex( self, RuntimeError, "Selection is not editable" ) : + self.assertIsNone( selection.acquireTransformEdit() ) + def testDontEditUpstreamOfReference( self ) : script = Gaffer.ScriptNode() @@ -244,6 +331,7 @@ def testDontEditUpstreamOfReference( self ) : self.assertEqual( selection.upstreamScene(), script["reference"]["transform"]["out"] ) self.assertEqual( selection.upstreamPath(), "/plane" ) self.assertEqual( selection.upstreamContext()["scene:path"], IECore.InternedStringVectorData( [ "plane" ] ) ) + self.assertEqual( selection.warning(), "Transform is locked as it is inside \"reference\" which disallows edits to its children" ) self.assertFalse( selection.editable() ) if __name__ == "__main__": diff --git a/python/GafferSceneUITest/TranslateToolTest.py b/python/GafferSceneUITest/TranslateToolTest.py index 3eb83cd115a..c2bf93e2299 100644 --- a/python/GafferSceneUITest/TranslateToolTest.py +++ b/python/GafferSceneUITest/TranslateToolTest.py @@ -670,7 +670,7 @@ def testEditAncestorIfSelectionNotTransformable( self ) : self.assertEqual( len( selection ), 1 ) self.assertEqual( selection[0].editTarget(), script["sceneReader"]["transform"] ) self.assertEqual( selection[0].path(), "/group" ) - self.assertEqual( selection[0].warning(), "Editing parent location" ) + self.assertEqual( selection[0].warning(), "Editing parent location \"/group\"" ) def testSelectionRefersToFirstPublicPlug( self ) : diff --git a/src/GafferSceneUI/TransformTool.cpp b/src/GafferSceneUI/TransformTool.cpp index 5e0da0b68d9..a31a4c3f302 100644 --- a/src/GafferSceneUI/TransformTool.cpp +++ b/src/GafferSceneUI/TransformTool.cpp @@ -104,20 +104,6 @@ int filterResult( const FilterPlug *filter, const ScenePlug *scene ) return filter->getValue(); } -bool ancestorMakesChildNodesReadOnly( const Node *node ) -{ - node = node->parent(); - while( node ) - { - if( MetadataAlgo::getChildNodesAreReadOnly( node ) ) - { - return true; - } - node = node->parent(); - } - return false; -} - // Similar to `plug->source()`, but able to traverse through // Spreadsheet outputs to find the appropriate input row. V3fPlug *spreadsheetAwareSource( V3fPlug *plug, std::string &failureReason ) @@ -203,6 +189,11 @@ class HandlesGadget : public Gadget // TransformTool::Selection ////////////////////////////////////////////////////////////////////////// +/// \todo Work out how to avoid baking graph component names into message +/// strings. We decided to put up with the stale name bugs here as the benefit +/// of better informing user outweighs the (hopefully less frequent) times +/// that the message is out of date. + TransformTool::Selection::Selection() : m_editable( false ) { @@ -261,17 +252,26 @@ TransformTool::Selection::Selection( if( editScope && !editScopeFound ) { - m_warning = "EditScope not in history"; + m_warning = "The target EditScope \"" + displayName( editScope.get() ) + "\" is not in the scene history"; m_editable = false; return; } if( m_path.size() != path.size() ) { - m_warning = "Editing parent location"; + std::string pathString; + GafferScene::ScenePlug::pathToString( m_path, pathString ); + m_warning = "Editing parent location \"" + pathString + "\""; } } +void TransformTool::Selection::initFromHistory( const GafferScene::SceneAlgo::History *history ) +{ + m_upstreamScene = history->scene; + m_upstreamPath = history->context->get( ScenePlug::scenePathContextName ); + m_upstreamContext = history->context; +} + void TransformTool::Selection::initFromSceneNode( const GafferScene::SceneAlgo::History *history ) { // Check for SceneNode and return if there isn't one or it is disabled. @@ -368,9 +368,7 @@ void TransformTool::Selection::initFromSceneNode( const GafferScene::SceneAlgo:: // We found the TransformPlug and the upstream scene which authors the transform. // Recording this will terminate the search in `initWalk()`. - m_upstreamScene = history->scene; - m_upstreamPath = history->context->get( ScenePlug::scenePathContextName ); - m_upstreamContext = history->context; + initFromHistory( history ); // Now figure out if we're allowed to edit the transform, and set ourselves up // for editing if we are. @@ -388,76 +386,165 @@ void TransformTool::Selection::initFromSceneNode( const GafferScene::SceneAlgo:: return; } - if( - ancestorMakesChildNodesReadOnly( transformEdit.translate->node() ) || - ancestorMakesChildNodesReadOnly( transformEdit.rotate->node() ) || - ancestorMakesChildNodesReadOnly( transformEdit.scale->node() ) || - ancestorMakesChildNodesReadOnly( transformEdit.pivot->node() ) - ) + const auto readOnlyAncestors = { + MetadataAlgo::readOnlyReason( transformEdit.translate.get() ), + MetadataAlgo::readOnlyReason( transformEdit.rotate.get() ), + MetadataAlgo::readOnlyReason( transformEdit.scale.get() ), + MetadataAlgo::readOnlyReason( transformEdit.pivot.get() ), + }; + + for( auto graphComponent : readOnlyAncestors ) { - // Inside a Reference node or similar. Unlike a regular read-only - // status, the user has no chance of unlocking this node to edit it. - m_warning = "Transform is authored by a read-only node"; - return; + // Unlike normal read-only nodes, the user won't be able to simply + // unlock a reference node, so we disable editing. + if( const Node *node = runTimeCast( graphComponent ) ) + { + if( Gaffer::MetadataAlgo::getChildNodesAreReadOnly( node ) ) + { + m_warning = "Transform is locked as it is inside \"" + displayName( node ) + "\" which disallows edits to its children"; + return; + } + } + + if( graphComponent ) + { + m_warning = "\"" + displayName( graphComponent ) + "\" is locked"; + break; + } } m_editable = true; m_transformEdit = transformEdit; } -void TransformTool::Selection::initWalk( const GafferScene::SceneAlgo::History *history, bool &editScopeFound ) +void TransformTool::Selection::initFromEditScope( const GafferScene::SceneAlgo::History *history ) { - // See if we're at the EditScope. We look for this even after - // `initFromSceneNode()` has succeeded, so we can differentiate between - // the EditScope not being in the history at all, or it being - // overridden downstream. + initFromHistory( history ); + + Context::Scope scopedContext( m_upstreamContext.get() ); + + if( !m_editScope->enabledPlug()->getValue() ) + { + m_warning = "The target EditScope \"" + displayName( m_editScope.get() ) + "\" is disabled"; + return; + } + + if( const GraphComponent *readOnlyComponent = EditScopeAlgo::transformEditReadOnlyReason( m_editScope.get(), m_upstreamPath ) ) + { + m_warning = "\"" + displayName( readOnlyComponent ) + "\" is locked"; + return; + } + + m_editable = true; + + m_transformEdit = EditScopeAlgo::acquireTransformEdit( m_editScope.get(), m_upstreamPath, /* createIfNeccesary = */ false ); + + ScenePlug::ScenePath spacePath = m_upstreamPath; + spacePath.pop_back(); + m_transformSpace = m_upstreamScene->fullTransform( spacePath ); +} + +void TransformTool::Selection::initWalk( const GafferScene::SceneAlgo::History *history, bool &editScopeFound, const GafferScene::SceneAlgo::History *editScopeOutHistory ) +{ + // Walk the history looking for a suitable node to edit. + // Transform tools only support editing the last node to author the targets + // transform (otherwise manipulators may be incorrectly placed or the + // transform overwritten). + Node *node = history->scene->node(); - if( !editScopeFound && node == m_editScope && history->scene == m_editScope->outPlug() ) + + if( !m_upstreamScene ) { - editScopeFound = true; - if( !m_upstreamScene ) + // 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 ) { - m_upstreamScene = history->scene; - m_upstreamPath = history->context->get( ScenePlug::scenePathContextName ); - m_upstreamContext = history->context; - m_transformEdit = EditScopeAlgo::acquireTransformEdit( m_editScope.get(), m_upstreamPath, /* createIfNecessary = */ false ); - Context::Scope scopedContext( history->context.get() ); - if( m_editScope->enabledPlug()->getValue() ) + editScopeFound = m_upstreamScene->ancestor() == m_editScope; + } + } + + // If the user has requested an edit scope, and we've not found it yet, + // check this entry. We do this regardless of whether we already have an + // edit so we can differentiate between a missing edit scope and one that + // is overridden downstream. + if( m_editScope && !editScopeFound ) + { + EditScope *editScope = runTimeCast( node ); + + if( editScope && editScope == m_editScope ) + { + if( m_upstreamScene ) { - m_editable = true; + // If we're here with an existing edit, then it means it is + // downstream of the requested scope. + editScopeFound = true; + + m_warning = "The target EditScope \"" + displayName( m_editScope.get() ) + "\" is overridden downstream by \"" + displayName( m_upstreamScene->node() ) + "\""; + m_editable = false; } - else + else if( history->scene == editScope->outPlug() ) { - m_warning = "EditScope disabled"; + // We only consider using an EditScope to author new edits on + // the way in (from the Node Graph perspective). This allows nodes + // inside to take precedence, however we need to use the + // history from the scopes output, so keep track of it for the + // rest of the walk. + editScopeOutHistory = history; } + else if( history->scene == editScope->inPlug() ) + { + editScopeFound = true; - ScenePlug::ScenePath spacePath = m_upstreamPath; - spacePath.pop_back(); - m_transformSpace = m_upstreamScene->fullTransform( spacePath ); - } - else - { - m_editable = false; - m_warning = "EditScope overridden downstream"; + if( editScopeOutHistory ) + { + initFromEditScope( editScopeOutHistory ); + } + else + { + // This can happen if the viewed node is inside the chosen EditScope + m_warning = "The output of the target EditScope \"" + displayName( m_editScope.get() ) + "\" is not in the scene history"; + m_editable = false; + } + } } - return; } - // See if there's an editable SceneNode here. We only - // look for these if we haven't already found the node - // that authored the transform. - if( !m_upstreamScene ) + if( initRequirementsSatisfied( editScopeFound ) ) { - initFromSceneNode( history ); + return; } - // Keep looking upstream. for( const auto &p : history->predecessors ) { - initWalk( p.get(), editScopeFound ); + initWalk( p.get(), editScopeFound, editScopeOutHistory ); + + if( initRequirementsSatisfied( editScopeFound ) ) + { + return; + } + } + + // If we get to here, we've exhausted all upstream history, without finding + // the input to the chosen scope. This can happen if the object is created + // inside a nested edit scope - as we can't use the creation node as it is + // in another scope. The requested scope is still valid though. + if( editScopeOutHistory && history->scene == editScopeOutHistory->scene ) + { + editScopeFound = true; + initFromEditScope( editScopeOutHistory ); } } +bool TransformTool::Selection::initRequirementsSatisfied( bool editScopeFound ) +{ + // If we don't have an EditScope specified, having a node to edit is enough. + // If we do, we need to make sure we have found it. + return ( m_upstreamScene && !m_editScope ) || ( m_editScope && editScopeFound ); +} + const GafferScene::ScenePlug *TransformTool::Selection::scene() const { return m_scene.get(); @@ -524,7 +611,7 @@ const EditScope *TransformTool::Selection::editScope() const Gaffer::GraphComponent *TransformTool::Selection::editTarget() const { throwIfNotEditable(); - if( m_editScope ) + if( m_editScope && !m_transformEdit ) { return m_editScope.get(); } @@ -719,6 +806,11 @@ Imath::M44f TransformTool::Selection::orientedTransform( Orientation orientation return result; } +std::string TransformTool::Selection::displayName( const GraphComponent *component ) +{ + return component->relativeName( component->ancestor() ); +} + ////////////////////////////////////////////////////////////////////////// // TransformTool ////////////////////////////////////////////////////////////////////////// @@ -923,7 +1015,7 @@ void TransformTool::plugDirtied( const Gaffer::Plug *plug ) void TransformTool::metadataChanged( IECore::InternedString key ) { - if( !MetadataAlgo::readOnlyAffectedByChange( key ) || m_handlesDirty ) + if( !MetadataAlgo::readOnlyAffectedByChange( key ) ) { return; } @@ -935,10 +1027,19 @@ void TransformTool::metadataChanged( IECore::InternedString key ) // so that a hidden Viewer has no overhead, so we just assume the worst // for now and do a more accurate analysis in `updateHandles()`. - m_handlesDirty = true; - view()->viewportGadget()->renderRequestSignal()( - view()->viewportGadget() - ); + if( !m_selectionDirty ) + { + m_selectionDirty = true; + selectionChangedSignal()( *this ); + } + + if( !m_handlesDirty ) + { + m_handlesDirty = true; + view()->viewportGadget()->renderRequestSignal()( + view()->viewportGadget() + ); + } } void TransformTool::updateSelection() const