From afc74239dabe1918028bcfcad03a45cad6db55b1 Mon Sep 17 00:00:00 2001 From: Jerry Gamache Date: Fri, 25 Nov 2022 14:34:29 -0500 Subject: [PATCH 1/4] Fix duplication of homonyms Fixes: Two items that would resolve to the same path after name conflict resolution actually end up being duplicated into the same name. So duplicating "bob1" and "bob2" in the same selection would result in a single "bob3" and a missing "bob4". The code now creates and executes sub-duplication tasks in the main loop to make sure the name conflict resolution sees the complete picture. --- .../ufe/UsdUndoDuplicateSelectionCommand.cpp | 57 ++++++++++++------- .../ufe/UsdUndoDuplicateSelectionCommand.h | 3 + test/lib/ufe/testDuplicateCmd.py | 22 +++++++ 3 files changed, 61 insertions(+), 21 deletions(-) diff --git a/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp b/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp index eb7b5886b8..e1f0ad5839 100644 --- a/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp +++ b/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp @@ -50,18 +50,8 @@ UsdUndoDuplicateSelectionCommand::UsdUndoDuplicateSelectionCommand( const Ufe::ValueDictionary& duplicateOptions) : Ufe::SelectionUndoableCommand() , _copyExternalInputs(shouldConnectExternalInputs(duplicateOptions)) + , _sourceSelection(selection) { - // TODO: MAYA-125854. If duplicating /a/b and /a/b/c, it would make sense to order the - // operations by SdfPath, and always check if the previously processed path is a prefix of the - // one currently being processed. In that case, a duplicate task is not necessary because the - // resulting SceneItem should be built by using SdfPath::ReplacePrefix on the current item to - // get its location in the previously duplicated parent item. - for (auto&& item : selection) { - if (UsdSceneItem::Ptr usdItem = std::dynamic_pointer_cast(item)) { - // Currently unordered_map since we need to streamline the targetItem override. - _perItemCommands[item->path()] = UsdUndoDuplicateCommand::create(usdItem); - } - } } UsdUndoDuplicateSelectionCommand::~UsdUndoDuplicateSelectionCommand() { } @@ -70,24 +60,46 @@ UsdUndoDuplicateSelectionCommand::Ptr UsdUndoDuplicateSelectionCommand::create( const Ufe::Selection& selection, const Ufe::ValueDictionary& duplicateOptions) { - auto retVal = std::make_shared(selection, duplicateOptions); - if (retVal->_perItemCommands.empty()) { - // Could not find any item from this runtime in the selection. - return {}; + bool canDuplicate = false; + for (auto&& item : selection) { + UsdSceneItem::Ptr usdItem = std::dynamic_pointer_cast(item); + if (usdItem) { + canDuplicate = true; + break; + } } - return retVal; + if (canDuplicate) { + return std::make_shared(selection, duplicateOptions); + } + return {}; } void UsdUndoDuplicateSelectionCommand::execute() { UsdUndoBlock undoBlock(&_undoableItem); - for (auto&& duplicateItem : _perItemCommands) { - duplicateItem.second->execute(); + // TODO: MAYA-125854. If duplicating /a/b and /a/b/c, it would make sense to order the + // operations by SdfPath, and always check if the previously processed path is a prefix of the + // one currently being processed. In that case, a duplicate task is not necessary because the + // resulting SceneItem should be built by using SdfPath::ReplacePrefix on the current item to + // get its location in the previously duplicated parent item. + for (auto&& item : _sourceSelection) { + UsdSceneItem::Ptr usdItem = std::dynamic_pointer_cast(item); + if (!usdItem) { + continue; + } + + // Need to create and execute. If we create all before executing any, then the collision + // resolution on names will merge bob1 and bob2 into a single bob3 instead of creating a + // bob3 and a bob4. + auto duplicateCmd = UsdUndoDuplicateCommand::create(usdItem); + duplicateCmd->execute(); - auto dstSceneItem = duplicateItem.second->duplicatedItem(); - PXR_NS::UsdPrim srcPrim = ufePathToPrim(duplicateItem.first); - PXR_NS::UsdPrim dstPrim = std::dynamic_pointer_cast(dstSceneItem)->prim(); + // Currently unordered_map since we need to streamline the targetItem override. + _perItemCommands[item->path()] = duplicateCmd; + + PXR_NS::UsdPrim srcPrim = usdItem->prim(); + PXR_NS::UsdPrim dstPrim = duplicateCmd->duplicatedItem()->prim(); Ufe::Path stgPath = stagePath(dstPrim.GetStage()); auto stageIt = _duplicatesMap.find(stgPath); @@ -100,6 +112,9 @@ void UsdUndoDuplicateSelectionCommand::execute() stageIt->second.insert({ srcPrim.GetPath(), dstPrim.GetPath() }); } + // We no longer require the source selection: + _sourceSelection.clear(); + // Fixups were grouped by stage. for (const auto& stageData : _duplicatesMap) { PXR_NS::UsdStageWeakPtr stage(getStage(stageData.first)); diff --git a/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.h b/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.h index 788706cf9e..5e9fde0b70 100644 --- a/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.h +++ b/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.h @@ -62,6 +62,9 @@ class MAYAUSD_CORE_PUBLIC UsdUndoDuplicateSelectionCommand : public Ufe::Selecti UsdUndoableItem _undoableItem; const bool _copyExternalInputs; + // Transient list of items to duplicate. Needed by execute. + Ufe::Selection _sourceSelection; + using CommandMap = std::unordered_map; CommandMap _perItemCommands; diff --git a/test/lib/ufe/testDuplicateCmd.py b/test/lib/ufe/testDuplicateCmd.py index 7847af78da..0e91f88f3d 100644 --- a/test/lib/ufe/testDuplicateCmd.py +++ b/test/lib/ufe/testDuplicateCmd.py @@ -464,6 +464,28 @@ def testUfeDuplicateCommandAPI(self): self.assertIsNotNone(plane7Item) self.assertEqual(plane7Item, duplicatedItem) + @unittest.skipIf(os.getenv('UFE_PREVIEW_VERSION_NUM', '0000') < '4041', 'Test only available in UFE preview version 0.4.41 and greater') + def testUfeDuplicateHomonyms(self): + '''Test that duplicating two items with similar names end up in two new duplicates.''' + testFile = testUtils.getTestScene('MaterialX', 'BatchOpsTestScene.usda') + shapeNode,shapeStage = mayaUtils.createProxyFromFile(testFile) + + geomItem1 = ufeUtils.createUfeSceneItem(shapeNode, '/pPlane1') + self.assertIsNotNone(geomItem1) + geomItem2 = ufeUtils.createUfeSceneItem(shapeNode, '/pPlane2') + self.assertIsNotNone(geomItem2) + + batchOpsHandler = ufe.RunTimeMgr.instance().batchOpsHandler(geomItem1.runTimeId()) + self.assertIsNotNone(batchOpsHandler) + + sel = ufe.Selection() + sel.append(geomItem1) + sel.append(geomItem2) + cmd = batchOpsHandler.duplicateSelectionCmd(sel, {"inputConnections": False}) + cmd.execute() + + self.assertNotEqual(cmd.targetItem(geomItem1.path()).path(), cmd.targetItem(geomItem2.path()).path()) + if __name__ == '__main__': unittest.main(verbosity=2) From a7efa928a90c9512712d1bd1427562cd44b01f66 Mon Sep 17 00:00:00 2001 From: Jerry Gamache Date: Tue, 29 Nov 2022 15:18:51 -0500 Subject: [PATCH 2/4] MAYA-125854 Skip duplicating descendants As inspired by a review comment. --- .../ufe/UsdUndoDuplicateSelectionCommand.cpp | 71 ++++++++++++------- .../ufe/UsdUndoDuplicateSelectionCommand.h | 3 +- test/lib/ufe/testDuplicateCmd.py | 47 ++++++++++++ 3 files changed, 93 insertions(+), 28 deletions(-) diff --git a/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp b/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp index e1f0ad5839..8cd2d4cfe7 100644 --- a/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp +++ b/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp @@ -30,6 +30,7 @@ #include #include +#include #include namespace MAYAUSD_NS_DEF { @@ -50,8 +51,19 @@ UsdUndoDuplicateSelectionCommand::UsdUndoDuplicateSelectionCommand( const Ufe::ValueDictionary& duplicateOptions) : Ufe::SelectionUndoableCommand() , _copyExternalInputs(shouldConnectExternalInputs(duplicateOptions)) - , _sourceSelection(selection) { + _sourceItems.reserve(selection.size()); + for (auto&& item : selection) { + if (selection.containsAncestor(item->path())) { + // MAYA-125854: Skip the descendant, it will get duplicated with the ancestor. + continue; + } + UsdSceneItem::Ptr usdItem = std::dynamic_pointer_cast(item); + if (!usdItem) { + continue; + } + _sourceItems.push_back(usdItem); + } } UsdUndoDuplicateSelectionCommand::~UsdUndoDuplicateSelectionCommand() { } @@ -60,35 +72,21 @@ UsdUndoDuplicateSelectionCommand::Ptr UsdUndoDuplicateSelectionCommand::create( const Ufe::Selection& selection, const Ufe::ValueDictionary& duplicateOptions) { - bool canDuplicate = false; - for (auto&& item : selection) { - UsdSceneItem::Ptr usdItem = std::dynamic_pointer_cast(item); - if (usdItem) { - canDuplicate = true; - break; - } - } - if (canDuplicate) { - return std::make_shared(selection, duplicateOptions); + UsdUndoDuplicateSelectionCommand::Ptr retVal + = std::make_shared(selection, duplicateOptions); + + if (retVal->_sourceItems.empty()) { + return {}; } - return {}; + + return retVal; } void UsdUndoDuplicateSelectionCommand::execute() { UsdUndoBlock undoBlock(&_undoableItem); - // TODO: MAYA-125854. If duplicating /a/b and /a/b/c, it would make sense to order the - // operations by SdfPath, and always check if the previously processed path is a prefix of the - // one currently being processed. In that case, a duplicate task is not necessary because the - // resulting SceneItem should be built by using SdfPath::ReplacePrefix on the current item to - // get its location in the previously duplicated parent item. - for (auto&& item : _sourceSelection) { - UsdSceneItem::Ptr usdItem = std::dynamic_pointer_cast(item); - if (!usdItem) { - continue; - } - + for (auto&& usdItem : _sourceItems) { // Need to create and execute. If we create all before executing any, then the collision // resolution on names will merge bob1 and bob2 into a single bob3 instead of creating a // bob3 and a bob4. @@ -96,7 +94,7 @@ void UsdUndoDuplicateSelectionCommand::execute() duplicateCmd->execute(); // Currently unordered_map since we need to streamline the targetItem override. - _perItemCommands[item->path()] = duplicateCmd; + _perItemCommands[usdItem->path()] = duplicateCmd; PXR_NS::UsdPrim srcPrim = usdItem->prim(); PXR_NS::UsdPrim dstPrim = duplicateCmd->duplicatedItem()->prim(); @@ -113,7 +111,7 @@ void UsdUndoDuplicateSelectionCommand::execute() } // We no longer require the source selection: - _sourceSelection.clear(); + _sourceItems.clear(); // Fixups were grouped by stage. for (const auto& stageData : _duplicatesMap) { @@ -164,11 +162,30 @@ void UsdUndoDuplicateSelectionCommand::execute() Ufe::SceneItem::Ptr UsdUndoDuplicateSelectionCommand::targetItem(const Ufe::Path& sourcePath) const { + // Perfect match: CommandMap::const_iterator it = _perItemCommands.find(sourcePath); - if (it == _perItemCommands.cend()) { + if (it != _perItemCommands.cend()) { + return it->second->duplicatedItem(); + } + + // MAYA-125854: If we do not find that exact path, see if it is a descendant of a duplicated + // ancestor. We will stop at the segment boundary. + Ufe::Path path = sourcePath; + size_t numSegments = sourcePath.getSegments().size(); + if (!numSegments) { return {}; } - return it->second->duplicatedItem(); + + while (numSegments == path.getSegments().size()) { + CommandMap::const_iterator it = _perItemCommands.find(path); + if (it != _perItemCommands.cend() && it->second->duplicatedItem()) { + Ufe::Path duplicatedChildPath + = sourcePath.reparent(path, it->second->duplicatedItem()->path()); + return Ufe::Hierarchy::createItem(duplicatedChildPath); + } + path = path.pop(); + } + return {}; } bool UsdUndoDuplicateSelectionCommand::updateSdfPathVector( diff --git a/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.h b/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.h index 5e9fde0b70..de3084a6d5 100644 --- a/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.h +++ b/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.h @@ -17,6 +17,7 @@ #define MAYAUSD_UFE_USDUNDODUPLICATESELECTIONCOMMAND_H #include +#include #include #include @@ -63,7 +64,7 @@ class MAYAUSD_CORE_PUBLIC UsdUndoDuplicateSelectionCommand : public Ufe::Selecti const bool _copyExternalInputs; // Transient list of items to duplicate. Needed by execute. - Ufe::Selection _sourceSelection; + std::vector _sourceItems; using CommandMap = std::unordered_map; CommandMap _perItemCommands; diff --git a/test/lib/ufe/testDuplicateCmd.py b/test/lib/ufe/testDuplicateCmd.py index 0e91f88f3d..d974feb529 100644 --- a/test/lib/ufe/testDuplicateCmd.py +++ b/test/lib/ufe/testDuplicateCmd.py @@ -486,6 +486,53 @@ def testUfeDuplicateHomonyms(self): self.assertNotEqual(cmd.targetItem(geomItem1.path()).path(), cmd.targetItem(geomItem2.path()).path()) + @unittest.skipIf(os.getenv('UFE_PREVIEW_VERSION_NUM', '0000') < '4041', 'Test only available in UFE preview version 0.4.41 and greater') + def testUfeDuplicateDescendants(self): + '''MAYA-125854: Test that duplicating a descendant of a selected ancestor results in the + duplicate from the ancestor.''' + testFile = testUtils.getTestScene('MaterialX', 'BatchOpsTestScene.usda') + shapeNode,shapeStage = mayaUtils.createProxyFromFile(testFile) + + # Take 3 items that are in a hierarchical relationship. + shaderItem1 = ufeUtils.createUfeSceneItem(shapeNode, '/pPlane2/mtl/ss2SG') + self.assertIsNotNone(shaderItem1) + geomItem = ufeUtils.createUfeSceneItem(shapeNode, '/pPlane2') + self.assertIsNotNone(geomItem) + shaderItem2 = ufeUtils.createUfeSceneItem(shapeNode, '/pPlane2/mtl/ss2SG/MayaNG_ss2SG/MayaConvert_file2_MayafileTexture') + self.assertIsNotNone(shaderItem2) + + batchOpsHandler = ufe.RunTimeMgr.instance().batchOpsHandler(geomItem.runTimeId()) + self.assertIsNotNone(batchOpsHandler) + + # Put then in a selection, making sure one child item is first, and that another child item is last. + sel = ufe.Selection() + sel.append(shaderItem1) + sel.append(geomItem) + sel.append(shaderItem2) + cmd = batchOpsHandler.duplicateSelectionCmd(sel, {"inputConnections": False}) + cmd.execute() + + duplicatedGeomItem = cmd.targetItem(geomItem.path()) + self.assertEqual(ufe.PathString.string(duplicatedGeomItem.path()), "|stage|stageShape,/pPlane7" ) + + # Make sure the duplicated shader items are descendants of the duplicated geom pPlane7. + sel.clear() + sel.append(duplicatedGeomItem) + duplicatedShaderItem1 = cmd.targetItem(shaderItem1.path()) + self.assertEqual(ufe.PathString.string(duplicatedShaderItem1.path()), + "|stage|stageShape,/pPlane7/mtl/ss2SG" ) + self.assertTrue(sel.containsAncestor(duplicatedShaderItem1.path())) + + duplicatedShaderItem2 = cmd.targetItem(shaderItem2.path()) + self.assertEqual(ufe.PathString.string(duplicatedShaderItem2.path()), + "|stage|stageShape,/pPlane7/mtl/ss2SG/MayaNG_ss2SG/MayaConvert_file2_MayafileTexture" ) + self.assertTrue(sel.containsAncestor(duplicatedShaderItem2.path())) + + # Test that the ancestor search terminates correctly: + nonDuplicatedGeomItem = ufeUtils.createUfeSceneItem(shapeNode, '/pPlane1') + self.assertIsNotNone(nonDuplicatedGeomItem) + self.assertIsNone(cmd.targetItem(nonDuplicatedGeomItem.path())) + if __name__ == '__main__': unittest.main(verbosity=2) From 3210a00f6f83a60fe405849c6e8a43c247b2176b Mon Sep 17 00:00:00 2001 From: Jerry Gamache Date: Tue, 29 Nov 2022 15:24:18 -0500 Subject: [PATCH 3/4] Fixing anticipated build break on Unix platforms --- lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp b/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp index 8cd2d4cfe7..7edcb06587 100644 --- a/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp +++ b/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp @@ -177,7 +177,7 @@ Ufe::SceneItem::Ptr UsdUndoDuplicateSelectionCommand::targetItem(const Ufe::Path } while (numSegments == path.getSegments().size()) { - CommandMap::const_iterator it = _perItemCommands.find(path); + it = _perItemCommands.find(path); if (it != _perItemCommands.cend() && it->second->duplicatedItem()) { Ufe::Path duplicatedChildPath = sourcePath.reparent(path, it->second->duplicatedItem()->path()); From 9a2ea3bfa16769370599ba54b5c22c0e2ef87753 Mon Sep 17 00:00:00 2001 From: Jerry Gamache Date: Thu, 1 Dec 2022 10:03:03 -0500 Subject: [PATCH 4/4] Fix bad grammar --- test/lib/ufe/testDuplicateCmd.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/lib/ufe/testDuplicateCmd.py b/test/lib/ufe/testDuplicateCmd.py index d974feb529..9fad2ac28c 100644 --- a/test/lib/ufe/testDuplicateCmd.py +++ b/test/lib/ufe/testDuplicateCmd.py @@ -504,7 +504,7 @@ def testUfeDuplicateDescendants(self): batchOpsHandler = ufe.RunTimeMgr.instance().batchOpsHandler(geomItem.runTimeId()) self.assertIsNotNone(batchOpsHandler) - # Put then in a selection, making sure one child item is first, and that another child item is last. + # Put them in a selection, making sure one child item is first, and that another child item is last. sel = ufe.Selection() sel.append(shaderItem1) sel.append(geomItem)