EMSUSD-998 better universal manip undo redo

Fix the complicated handling of undo and redo vs the universal manipulator. The
code failed to do the right thing when half of the attributes are already
existed or when more complex composite manipulations were done, for example
when scaling with the universal manipulator.

We make the transformation commands more robust vs the ordering of calls to
support all use cases. They still handle the bug related to object-space
translation redo: when redoing an object-space translation, Maya passes in
incorrect values to the command that must be ignored.

Apply fix to all transform commands. This way they all use the same pattern
from UsdSetXformOpUndoableCommandBase to manage any order of calls to
execute/set/undo/redo: use in the CommonAPI commands, matrix commands
and Maya tranform-stack commands.

lib/mayaUsd/ufe/UsdSetXformOpUndoableCommandBase.cpp

@@ -21,68 +21,127 @@
 
 PXR_NAMESPACE_USING_DIRECTIVE
 
-namespace {
-void warnUnimplemented(const char* msg) { TF_WARN("Illegal call to unimplemented %s", msg); }
-} // namespace
-
 namespace MAYAUSD_NS_DEF {
 namespace ufe {
 
-template <typename T>
-UsdSetXformOpUndoableCommandBase<T>::UsdSetXformOpUndoableCommandBase(
-    const Ufe::Path&   path,
-    const UsdTimeCode& writeTime)
+UsdSetXformOpUndoableCommandBase::UsdSetXformOpUndoableCommandBase(
+    const PXR_NS::VtValue&     value,
+    const Ufe::Path&           path,
+    const PXR_NS::UsdTimeCode& writeTime)
     : Ufe::SetVector3dUndoableCommand(path)
-    , _readTime(getTime(path)) // Always read from proxy shape time.
     , _writeTime(writeTime)
+    , _newOpValue(value)
+    , _isPrepared(false)
+    , _canUpdateValue(true)
+    , _opCreated(false)
+{
+}
+
+UsdSetXformOpUndoableCommandBase::UsdSetXformOpUndoableCommandBase(
+    const Ufe::Path&   path,
+    const UsdTimeCode& writeTime)
+    : UsdSetXformOpUndoableCommandBase({}, path, writeTime)
+{
+}
+
+void UsdSetXformOpUndoableCommandBase::execute()
+{
+    // Create the attribute and cache the initial value,
+    // if this is the first time we're executed, or redo
+    // the attribute creation.
+    recreateOpIfNeeded();
+
+    // Set the new value.
+    prepareAndSet(_newOpValue);
+    _canUpdateValue = true;
+}
+
+void UsdSetXformOpUndoableCommandBase::undo()
+{
+    // If the command was never called at all, do nothing.
+    // Maya can start by calling undo.
+    if (!_isPrepared)
+        return;
+
+    // Restore the initial value and potentially remove the creatd attributes.
+    setValue(_initialOpValue, _writeTime);
+    removeOpIfNeeded();
+    _canUpdateValue = false;
+}
+
+void UsdSetXformOpUndoableCommandBase::redo()
 {
+    // Redo the attribute creation if the attribute was already created
+    // but then undone.
+    recreateOpIfNeeded();
+
+    // Set the new value, potentially creating the attribute if it
+    // did not exists or caching the initial value if this is the
+    // first time the command is executed, redone or undone.
+    prepareAndSet(_newOpValue);
+    _canUpdateValue = true;
 }
 
-template <typename T> void UsdSetXformOpUndoableCommandBase<T>::execute()
+void UsdSetXformOpUndoableCommandBase::updateNewValue(const VtValue& v)
 {
-    warnUnimplemented("UsdSetXformOpUndoableCommandBase::execute()");
+    // Redo the attribute creation if the attribute was already created
+    // but then undone.
+    recreateOpIfNeeded();
+
+    // Update the value that will be set.
+    if (_canUpdateValue)
+        _newOpValue = v;
+
+    // Set the new value, potentially creating the attribute if it
+    // did not exists or caching the initial value if this is the
+    // first time the command is executed, redone or undone.
+    prepareAndSet(_newOpValue);
+    _canUpdateValue = true;
 }
 
-template <typename T> void UsdSetXformOpUndoableCommandBase<T>::undo()
+void UsdSetXformOpUndoableCommandBase::prepareAndSet(const VtValue& v)
 {
-    if (_state == kInitial) {
-        // Spurious call from Maya, ignore.
-        _state = kInitialUndoCalled;
+    if (v.IsEmpty())
         return;
-    }
-    _undoableItem.undo();
-    _state = kUndone;
+
+    prepareOpIfNeeded();
+    setValue(v, _writeTime);
 }
 
-template <typename T> void UsdSetXformOpUndoableCommandBase<T>::redo()
+void UsdSetXformOpUndoableCommandBase::prepareOpIfNeeded()
 {
-    warnUnimplemented("UsdSetXformOpUndoableCommandBase::redo()");
+    if (_isPrepared)
+        return;
+
+    createOpIfNeeded(_opCreationUndo);
+    _initialOpValue = getValue(_writeTime);
+    _isPrepared = true;
+    _opCreated = true;
 }
 
-template <typename T> void UsdSetXformOpUndoableCommandBase<T>::handleSet(const T& v)
+void UsdSetXformOpUndoableCommandBase::recreateOpIfNeeded()
 {
-    if (_state == kInitialUndoCalled) {
-        // Spurious call from Maya, ignore.  Otherwise, we set a value that
-        // is identical to the previous, the UsdUndoBlock does not capture
-        // any invertFunc's, and subsequent undo() calls undo nothing.
-        _state = kInitial;
-    } else if (_state == kInitial) {
-        UsdUndoBlock undoBlock(&_undoableItem);
-        setValue(v);
-        _state = kExecute;
-    } else if (_state == kExecute) {
-        setValue(v);
-    } else if (_state == kUndone) {
-        _undoableItem.redo();
-        _state = kRedone;
-    }
+    if (!_isPrepared)
+        return;
+
+    if (_opCreated)
+        return;
+
+    _opCreationUndo.redo();
+    _opCreated = true;
 }
 
-// Explicit instantiation for transform ops that can be set from matrices and
-// vectors.
-template class UsdSetXformOpUndoableCommandBase<GfVec3f>;
-template class UsdSetXformOpUndoableCommandBase<GfVec3d>;
-template class UsdSetXformOpUndoableCommandBase<GfMatrix4d>;
+void UsdSetXformOpUndoableCommandBase::removeOpIfNeeded()
+{
+    if (!_isPrepared)
+        return;
+
+    if (!_opCreated)
+        return;
+
+    _opCreationUndo.undo();
+    _opCreated = false;
+}
 
 } // namespace ufe
 } // namespace MAYAUSD_NS_DEF

lib/mayaUsd/ufe/UsdSetXformOpUndoableCommandBase.h

@@ -19,6 +19,7 @@
 
 #include <usdUfe/undo/UsdUndoableItem.h>
 
+#include <pxr/base/vt/value.h>
 #include <pxr/usd/usd/timeCode.h>
 
 #include <ufe/transform3dUndoableCommands.h>
@@ -31,48 +32,83 @@ namespace ufe {
 // Helper class to factor out common code for translate, rotate, scale
 // undoable commands.  It is templated on the type of the transform op.
 //
-// Developing commands to work with Maya TRS commands is made more difficult
-// because Maya calls undo(), but never calls redo(): it simply calls set()
-// with the new value again.  We must distinguish cases where set() must
-// capture state, so that undo() can completely remove any added primSpecs or
-// attrSpecs.  This class implements state tracking to allow this: state is
-// saved on transition between kInitial and kExecute.
-// UsdTransform3dMayaXformStack has a state machine based implementation that
-// avoids conditionals, but UsdSetXformOpUndoableCommandBase is less invasive
-// from a development standpoint.
-
-template <typename T>
+// We must do a careful dance due to historic reasons and the way Maya handle
+// interactive commands:
+//
+//     - These commands can be wrapped inside other commands which may
+//       use their own UsdUndoBlock. In particular, we must not try to
+//       undo an attribute creation if it was not yet created.
+//
+//     - Maya can call undo and set-value before first executing the
+//       command. In particular, when using manipualtion tools, Maya
+//       will usually do loops of undo/set-value/redo, thus beginning
+//       by undoing a command that was never executed.
+//
+//     - As a general rule, when undoing, we want to remove any attributes
+//       that were created when first executed.
+//
+//     - When redoing some commands after an undo, Maya will update the
+//       value to be set with an incorrect value when operating in object
+//       space, which must be ignored.
+//
+// Those things are what the prepare-op/recreate-op/remove-op functions are
+// aimed to support. Also, we must only capture the initial value the first
+// time thevalue is modified, to support both the inital undo/set-value and
+// avoid losing the initial value on repeat set-value.
 class UsdSetXformOpUndoableCommandBase : public Ufe::SetVector3dUndoableCommand
 {
-    const PXR_NS::UsdTimeCode _readTime;
-    const PXR_NS::UsdTimeCode _writeTime;
-    UsdUfe::UsdUndoableItem   _undoableItem;
-    enum State
-    {
-        kInitial,
-        kInitialUndoCalled,
-        kExecute,
-        kUndone,
-        kRedone
-    };
-    State _state { kInitial };
-
 public:
+    UsdSetXformOpUndoableCommandBase(
+        const PXR_NS::VtValue&     newValue,
+        const Ufe::Path&           path,
+        const PXR_NS::UsdTimeCode& writeTime);
     UsdSetXformOpUndoableCommandBase(const Ufe::Path& path, const PXR_NS::UsdTimeCode& writeTime);
 
     // Ufe::UndoableCommand overrides.
-    // No-op: Maya calls set() rather than execute().
     void execute() override;
     void undo() override;
-    // No-op: Maya calls set() rather than redo().
     void redo() override;
 
-    PXR_NS::UsdTimeCode readTime() const { return _readTime; }
     PXR_NS::UsdTimeCode writeTime() const { return _writeTime; }
 
-    virtual void setValue(const T&) = 0;
+protected:
+    // Create the XformOp attributes if they do not exists.
+    // The attribute creation must be capture in the UsdUndoableItem by using a
+    // UsdUndoBlock, so that removeOpIfNeeded and recreateOpIfNeeded can undo
+    // and redo the attribute creation if needed.
+    virtual void createOpIfNeeded(UsdUndoableItem&) = 0;
+
+    // Get the attribute at the given time.
+    virtual PXR_NS::VtValue getValue(const PXR_NS::UsdTimeCode& time) const = 0;
 
-    void handleSet(const T& v);
+    // Set the attribute at the given time. The value is guaranteed to either be
+    // the initial value that was returned by the getValue function above or a
+    // new value passed to the updateNewValue function below. So you are guaranteed
+    // that the type contained in the VtValue is the type you want.
+    virtual void setValue(const PXR_NS::VtValue&, const PXR_NS::UsdTimeCode& time) = 0;
+
+    // Function called by sub-classed when they want to set a new value.
+    void updateNewValue(const PXR_NS::VtValue& v);
+
+private:
+    void prepareAndSet(const PXR_NS::VtValue&);
+
+    // Create the XformOp attributes if they do not exists and cache the initial value.
+    void prepareOpIfNeeded();
+
+    // Recreate the attribute after being removed if it was created.
+    void recreateOpIfNeeded();
+
+    // Remove the attribute if it was created.
+    void removeOpIfNeeded();
+
+    const PXR_NS::UsdTimeCode _writeTime;
+    PXR_NS::VtValue           _initialOpValue;
+    PXR_NS::VtValue           _newOpValue;
+    UsdUndoableItem           _opCreationUndo;
+    bool                      _isPrepared;
+    bool                      _canUpdateValue;
+    bool                      _opCreated;
 };
 
 } // namespace ufe