diff --git a/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp b/lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp index eb7b5886b8..7edcb06587 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 { @@ -51,16 +52,17 @@ UsdUndoDuplicateSelectionCommand::UsdUndoDuplicateSelectionCommand( : Ufe::SelectionUndoableCommand() , _copyExternalInputs(shouldConnectExternalInputs(duplicateOptions)) { - // 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. + _sourceItems.reserve(selection.size()); 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); + 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); } } @@ -70,11 +72,13 @@ 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. + UsdUndoDuplicateSelectionCommand::Ptr retVal + = std::make_shared(selection, duplicateOptions); + + if (retVal->_sourceItems.empty()) { return {}; } + return retVal; } @@ -82,12 +86,18 @@ void UsdUndoDuplicateSelectionCommand::execute() { UsdUndoBlock undoBlock(&_undoableItem); - for (auto&& duplicateItem : _perItemCommands) { - duplicateItem.second->execute(); + 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. + auto duplicateCmd = UsdUndoDuplicateCommand::create(usdItem); + duplicateCmd->execute(); + + // Currently unordered_map since we need to streamline the targetItem override. + _perItemCommands[usdItem->path()] = duplicateCmd; - auto dstSceneItem = duplicateItem.second->duplicatedItem(); - PXR_NS::UsdPrim srcPrim = ufePathToPrim(duplicateItem.first); - PXR_NS::UsdPrim dstPrim = std::dynamic_pointer_cast(dstSceneItem)->prim(); + 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 +110,9 @@ void UsdUndoDuplicateSelectionCommand::execute() stageIt->second.insert({ srcPrim.GetPath(), dstPrim.GetPath() }); } + // We no longer require the source selection: + _sourceItems.clear(); + // Fixups were grouped by stage. for (const auto& stageData : _duplicatesMap) { PXR_NS::UsdStageWeakPtr stage(getStage(stageData.first)); @@ -149,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()) { + 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 788706cf9e..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 @@ -62,6 +63,9 @@ class MAYAUSD_CORE_PUBLIC UsdUndoDuplicateSelectionCommand : public Ufe::Selecti UsdUndoableItem _undoableItem; const bool _copyExternalInputs; + // Transient list of items to duplicate. Needed by execute. + 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 7847af78da..9fad2ac28c 100644 --- a/test/lib/ufe/testDuplicateCmd.py +++ b/test/lib/ufe/testDuplicateCmd.py @@ -464,6 +464,75 @@ 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()) + + @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 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) + 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)