EMSUSD-189 fix undo of the stage creation command

lib/mayaUsd/ufe/UsdUndoCreateStageWithNewLayerCommand.cpp

@@ -17,8 +17,10 @@
 
 #include <mayaUsd/ufe/UsdUndoRenameCommand.h>
 #include <mayaUsd/ufe/Utils.h>
+#include <mayaUsd/undo/OpUndoItemRecorder.h>
 #include <mayaUsd/undo/OpUndoItems.h>
 
+#include <maya/MDagModifier.h>
 #include <ufe/hierarchy.h>
 
 PXR_NAMESPACE_USING_DIRECTIVE
@@ -30,9 +32,6 @@ UsdUndoCreateStageWithNewLayerCommand::UsdUndoCreateStageWithNewLayerCommand(
     const Ufe::SceneItem::Ptr& parentItem)
     : _parentItem(nullptr)
     , _insertedChild(nullptr)
-    , _createTransformDagMod(MDagModifierUndoItem::create("Create transform"))
-    , _createProxyShapeDagMod(MDagModifierUndoItem::create("Create Stage with new Layer"))
-    , _success(false)
 {
     if (!TF_VERIFY(parentItem))
         return;
@@ -63,148 +62,124 @@ void UsdUndoCreateStageWithNewLayerCommand::execute()
         return;
     }
 
-    bool createTransformSuccess = false;
+    bool success = false;
     try {
-        // Get a MObject from the parent scene item.
-        // Note: If and only if the parent is the world node, MDagPath::transform() will set status
-        // to kInvalidParameter. In this case MObject::kNullObj is returned, which is a valid parent
-        // object. Thus, kInvalidParameter will not be treated as a failure.
-        MStatus  status;
-        MDagPath parentDagPath = MayaUsd::ufe::ufeToDagPath(_parentItem->path());
-        MObject  parentObject = parentDagPath.transform(&status);
-        if (status != MStatus::kInvalidParameter && MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-
-        // Create a transform node.
-        // Note: It would be possible to create the transform and the proxy shape in one doIt() call
-        // of a single MDagModifier. However, doing so causes notifications to be sent in a
-        // different order, which triggers a `TF_VERIFY(g_StageMap.isDirty())` in
-        // StagesSubject::onStageSet(). Using a separate MDagModifier to create the transform seems
-        // more robust and avoids triggering the TF_VERIFY.
-        MObject transformObj;
-        transformObj = _createTransformDagMod.createNode("transform", parentObject, &status);
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-        TF_VERIFY(!transformObj.isNull());
-        status = _createTransformDagMod.doIt();
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-        createTransformSuccess = true;
-
-        // Create a proxy shape.
-        MObject proxyShape;
-        proxyShape = _createProxyShapeDagMod.createNode("mayaUsdProxyShape", transformObj, &status);
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-        TF_VERIFY(!proxyShape.isNull());
-
-        // Rename the transform and the proxy shape.
-        // Note: The transform is renamed twice. The first rename operation renames it from its
-        // default name "transform1" to "stage1". The number-suffix will be automatically
-        // incremented if necessary. The second rename operation renames it from "stageX" to
-        // "stage1". This doesn't do anything for the transform itself but it will adjust the
-        // number-suffix of the proxy shape according to the suffix of the transform, because they
-        // now share the common prefix "stage".
-        status = _createProxyShapeDagMod.renameNode(proxyShape, "stageShape1");
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-        status = _createProxyShapeDagMod.renameNode(transformObj, "stage1");
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-        status = _createProxyShapeDagMod.renameNode(transformObj, "stage1");
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-
-        // Get the global `time1` object and its `outTime` attribute.
-        MSelectionList selection;
-        selection.add("time1");
-        MObject time1;
-        status = selection.getDependNode(0, time1);
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-        MFnDependencyNode time1DepNodeFn(time1, &status);
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-        MObject time1OutTimeAttr = time1DepNodeFn.attribute("outTime", &status);
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-
-        // Get the `time` attribute of the newly created mayaUsdProxyShape.
-        MDagPath proxyShapeDagPath;
-        status = MDagPath::getAPathTo(proxyShape, proxyShapeDagPath);
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-        MFnDependencyNode proxyShapeDepNodeFn(proxyShapeDagPath.node(), &status);
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-        MObject proxyShapeTimeAttr = proxyShapeDepNodeFn.attribute("time", &status);
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-
-        // Connect `time1.outTime` to `proxyShapde.time`.
-        status = _createProxyShapeDagMod.connect(
-            time1, time1OutTimeAttr, proxyShape, proxyShapeTimeAttr);
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-
-        // Execute the operations.
-        status = _createProxyShapeDagMod.doIt();
-        if (MFAIL(status)) {
-            throw std::runtime_error("");
-        }
-        _success = true;
-
-        // Create a UFE scene item for the newly created mayaUsdProxyShape.
-        Ufe::Path proxyShapeUfePath = MayaUsd::ufe::dagPathToUfe(proxyShapeDagPath);
-        _insertedChild = Ufe::Hierarchy::createItem(proxyShapeUfePath);
-
-        // Refresh the cache of the stage map.
-        // When creating the proxy shape, the stage map gets dirtied and cleaned. Afterwards, the
-        // proxy shape is renamed. The stage map does not observe the Maya data model, so renaming
-        // does not dirty the stage map again. Thus, the cache is in an invalid state, where it
-        // contains the path of the proxy shape before it was renamed. Calling getProxyShape()
-        // refreshes the cache. See comments within UsdStageMap::proxyShape() for more details.
-        getProxyShape(proxyShapeUfePath);
+        OpUndoItemRecorder undoRecorder(_undoItemList);
+        success = executeWithinUndoRecorder();
     } catch (const std::exception&) {
-        if (createTransformSuccess) {
-            _createTransformDagMod.undoIt();
-        }
+        _undoItemList.undo();
+        throw;
+    }
+
+    if (!success) {
+        _undoItemList.undo();
     }
 }
 
-void UsdUndoCreateStageWithNewLayerCommand::undo()
+bool UsdUndoCreateStageWithNewLayerCommand::executeWithinUndoRecorder()
 {
-    if (_success) {
-        _createProxyShapeDagMod.undoIt();
-        _createTransformDagMod.undoIt();
-    }
+    // Get a MObject from the parent scene item.
+    // Note: If and only if the parent is the world node, MDagPath::transform() will set status
+    // to kInvalidParameter. In this case MObject::kNullObj is returned, which is a valid parent
+    // object. Thus, kInvalidParameter will not be treated as a failure.
+    MStatus  status;
+    MDagPath parentDagPath = MayaUsd::ufe::ufeToDagPath(_parentItem->path());
+    MObject  parentObject = parentDagPath.transform(&status);
+    if (status != MStatus::kInvalidParameter && MFAIL(status))
+        return false;
+
+    MDagModifier& dagMod = MDagModifierUndoItem::create("Create stage with new Layer");
+
+    // Create a transform node.
+    // Note: It would be possible to create the transform and the proxy shape in one doIt() call
+    // of a single MDagModifier. However, doing so causes notifications to be sent in a
+    // different order, which triggers a `TF_VERIFY(g_StageMap.isDirty())` in
+    // StagesSubject::onStageSet(). Using a separate MDagModifier to create the transform seems
+    // more robust and avoids triggering the TF_VERIFY.
+    MObject transformObj;
+    transformObj = dagMod.createNode("transform", parentObject);
+    if (transformObj.isNull())
+        return false;
+
+    if (!dagMod.doIt())
+        return false;
+
+    // Create a proxy shape.
+    MObject proxyShape;
+    proxyShape = dagMod.createNode("mayaUsdProxyShape", transformObj);
+    if (proxyShape.isNull())
+        return false;
+
+    // Rename the transform and the proxy shape.
+    // Note: The transform is renamed twice. The first rename operation renames it from its
+    // default name "transform1" to "stage1". The number-suffix will be automatically
+    // incremented if necessary. The second rename operation renames it from "stageX" to
+    // "stage1". This doesn't do anything for the transform itself but it will adjust the
+    // number-suffix of the proxy shape according to the suffix of the transform, because they
+    // now share the common prefix "stage".
+    if (!dagMod.renameNode(proxyShape, "stageShape1"))
+        return false;
+    if (!dagMod.renameNode(transformObj, "stage1"))
+        return false;
+    if (!dagMod.renameNode(transformObj, "stage1"))
+        return false;
+
+    // Get the global `time1` object and its `outTime` attribute.
+    MSelectionList selection;
+    selection.add("time1");
+    MObject time1;
+    if (!selection.getDependNode(0, time1))
+        return false;
+    MFnDependencyNode time1DepNodeFn(time1, &status);
+    if (MFAIL(status))
+        return false;
+    MObject time1OutTimeAttr = time1DepNodeFn.attribute("outTime");
+    if (time1OutTimeAttr.isNull())
+        return false;
+
+    // Get the `time` attribute of the newly created mayaUsdProxyShape.
+    MDagPath proxyShapeDagPath;
+    if (!MDagPath::getAPathTo(proxyShape, proxyShapeDagPath))
+        return false;
+    MFnDependencyNode proxyShapeDepNodeFn(proxyShapeDagPath.node(), &status);
+    if (MFAIL(status))
+        return false;
+    MObject proxyShapeTimeAttr = proxyShapeDepNodeFn.attribute("time");
+    if (proxyShapeTimeAttr.isNull())
+        return false;
+
+    // Connect `time1.outTime` to `proxyShapde.time`.
+    if (!dagMod.connect(time1, time1OutTimeAttr, proxyShape, proxyShapeTimeAttr))
+        return false;
+
+    // Execute the operations.
+    if (!dagMod.doIt())
+        return false;
+
+    // Create a UFE scene item for the newly created mayaUsdProxyShape.
+    Ufe::Path proxyShapeUfePath = MayaUsd::ufe::dagPathToUfe(proxyShapeDagPath);
+    _insertedChild = Ufe::Hierarchy::createItem(proxyShapeUfePath);
+
+    // Refresh the cache of the stage map.
+    // When creating the proxy shape, the stage map gets dirtied and cleaned. Afterwards, the
+    // proxy shape is renamed. The stage map does not observe the Maya data model, so renaming
+    // does not dirty the stage map again. Thus, the cache is in an invalid state, where it
+    // contains the path of the proxy shape before it was renamed. Calling getProxyShape()
+    // refreshes the cache. See comments within UsdStageMap::proxyShape() for more details.
+    getProxyShape(proxyShapeUfePath);
+
+    return true;
 }
 
+void UsdUndoCreateStageWithNewLayerCommand::undo() { _undoItemList.undo(); }
+
 void UsdUndoCreateStageWithNewLayerCommand::redo()
 {
-    if (_success) {
-        _createTransformDagMod.doIt();
-        _createProxyShapeDagMod.doIt();
-
-        // Refresh the cache of the stage map.
-        if (_insertedChild) {
-            getProxyShape(_insertedChild->path());
-        }
-    }
+    _undoItemList.redo();
+
+    // Refresh the cache of the stage map.
+    if (_insertedChild)
+        getProxyShape(_insertedChild->path());
 }
 
 } // namespace ufe

lib/mayaUsd/ufe/UsdUndoCreateStageWithNewLayerCommand.h

@@ -17,8 +17,8 @@
 #define MAYAUSD_UFE_CREATESTAGEWITHNEWLAYERCOMMAND_H
 
 #include <mayaUsd/base/api.h>
+#include <mayaUsd/undo/OpUndoItemList.h>
 
-#include <maya/MDagModifier.h>
 #include <ufe/undoableCommand.h>
 
 namespace MAYAUSD_NS_DEF {
@@ -60,12 +60,14 @@ class MAYAUSD_CORE_PUBLIC UsdUndoCreateStageWithNewLayerCommand
     void redo() override;
 
 private:
+    // Executes the command, called within a undo recorder.
+    // Returns true on success.
+    bool executeWithinUndoRecorder();
+
     Ufe::SceneItem::Ptr _parentItem;
     Ufe::SceneItem::Ptr _insertedChild;
 
-    MDagModifier& _createTransformDagMod;
-    MDagModifier& _createProxyShapeDagMod;
-    bool          _success;
+    OpUndoItemList _undoItemList;
 }; // UsdUndoCreateStageWithNewLayerCommand
 
 } // namespace ufe

test/lib/testMayaUsdCreateStageCommands.py

@@ -21,10 +21,16 @@
 import unittest
 
 import testUtils
+from ufeUtils import ufeFeatureSetVersion
 
 from maya import cmds
 import maya.mel as mel
 
+import mayaUsd.lib
+import mayaUsd.ufe
+
+import ufe
+
 class MayaUsdCreateStageCommandsTestCase(unittest.TestCase):
     """Test the MEL commands that are used to create a USD stage."""
 
@@ -87,4 +93,41 @@ def testCreateStageFromFile(self):
         self.assertEqual(filePathAttrRel, 'top_layer.usda')
 
         # Restore mayaUsd_MakePathRelativeToSceneFile
-        cmds.optionVar(iv=('mayaUsd_MakePathRelativeToSceneFile', 0))
+        cmds.optionVar(iv=('mayaUsd_MakePathRelativeToSceneFile', 0))
+
+    @unittest.skipUnless(ufeFeatureSetVersion() >= 4, 'Test only available in UFE v4 or greater.')
+    def testCreateStageWithCommand(self):
+        '''
+        Create a stage with a new layer using the command exposed by a Python wrapper.
+        '''
+
+        stageUfePathStr = mayaUsd.ufe.createStageWithNewLayer("|world")
+        self.assertIsNotNone(stageUfePathStr)
+
+        stageUfePath = ufe.PathString.path(stageUfePathStr)
+        stageUfeSceneItem = ufe.Hierarchy.createItem(stageUfePath)
+        self.assertIsNotNone(stageUfeSceneItem)
+        stageUfeSceneItem = None
+
+        # Create a poly-sphere and copy it to the stage.
+        # We had a bug where undo would crash when undoing past this, so this is the goal
+        # of having this here when testing undo/redo below.
+        cmds.CreatePolygonSphere()
+        sphereNode = cmds.ls(sl=True,l=True)[0]
+        self.assertIsNotNone(sphereNode)
+        with mayaUsd.lib.OpUndoItemList():
+            self.assertTrue(mayaUsd.lib.PrimUpdaterManager.duplicate(
+                sphereNode, stageUfePathStr))
+
+        # Note: commands execute other commands. To get to the point where
+        #       the stage no longer exist, we need 6 undo.
+        undoCountToGetRidOfStage = 6
+        for _ in range(undoCountToGetRidOfStage):
+            cmds.undo()
+        stageUfeSceneItem = ufe.Hierarchy.createItem(stageUfePath)
+        self.assertIsNone(stageUfeSceneItem)
+
+        for _ in range(undoCountToGetRidOfStage):
+            cmds.redo()
+        stageUfeSceneItem = ufe.Hierarchy.createItem(stageUfePath)
+        self.assertIsNotNone(stageUfeSceneItem)