Fix duplication of homonyms

lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.cpp

@@ -30,6 +30,7 @@
 #include <pxr/usd/usd/relationship.h>
 #include <pxr/usd/usd/stage.h>
 
+#include <ufe/hierarchy.h>
 #include <ufe/path.h>
 
 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<UsdSceneItem>(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<UsdSceneItem>(item);
+        if (!usdItem) {
+            continue;
         }
+        _sourceItems.push_back(usdItem);
     }
 }
 
@@ -70,24 +72,32 @@ UsdUndoDuplicateSelectionCommand::Ptr UsdUndoDuplicateSelectionCommand::create(
     const Ufe::Selection&       selection,
     const Ufe::ValueDictionary& duplicateOptions)
 {
-    auto retVal = std::make_shared<UsdUndoDuplicateSelectionCommand>(selection, duplicateOptions);
-    if (retVal->_perItemCommands.empty()) {
-        // Could not find any item from this runtime in the selection.
+    UsdUndoDuplicateSelectionCommand::Ptr retVal
+        = std::make_shared<UsdUndoDuplicateSelectionCommand>(selection, duplicateOptions);
+
+    if (retVal->_sourceItems.empty()) {
         return {};
     }
+
     return retVal;
 }
 
 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<UsdSceneItem>(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(

lib/mayaUsd/ufe/UsdUndoDuplicateSelectionCommand.h

@@ -17,6 +17,7 @@
 #define MAYAUSD_UFE_USDUNDODUPLICATESELECTIONCOMMAND_H
 
 #include <mayaUsd/base/api.h>
+#include <mayaUsd/ufe/UsdSceneItem.h>
 #include <mayaUsd/ufe/UsdUndoDuplicateCommand.h>
 #include <mayaUsd/undo/UsdUndoableItem.h>
 
@@ -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<UsdSceneItem::Ptr> _sourceItems;
+
     using CommandMap = std::unordered_map<Ufe::Path, UsdUndoDuplicateCommand::Ptr>;
     CommandMap _perItemCommands;
 

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)