-
Notifications
You must be signed in to change notification settings - Fork 202
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
MAYA-106089: Undo/Redo support for USD data model. #942
Conversation
@aloysbaillet You may be interested to have a look at this change. |
for (const auto& childPrim : parentPrim.GetChildren()) { | ||
ufe::applyCommandRestriction(childPrim, "reorder"); | ||
break; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ppt-adsk this change is part of the fix for internal MAYA-106089.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ppt-adsk Added UFE guard to protect the code against PR120. I will clean up the temporary guards once PR121 is out.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is great! Thanks @HamedSabri-adsk and @kxl-adsk !
It would be great to also add a test with a temporary custom MPxCommand (maybe in python) to see how the code works when called from the doIt
.
const TfToken& keyPath, | ||
const SdfAbstractDataConstValue& value) | ||
{ | ||
throw std::logic_error(std::string { __func__ } + "is not implemented !!!"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A would recommend using a TF_ERROR macro instead as USD is not a big fan of exceptions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@aloysbaillet Good point. Addressed in 9836089
…ey() method. AL's testTranslators.py has a call to SetVariantSelection() that triggers the _OnSetFieldDictValueByKey.
…ce these test cases work with time sample.
TypeError: unbound method Boost.Python.function object must be called with UsdUndoManager instance as first argument.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! A few nit-picks here are there, but it looks solid.
|
||
static uint32_t _undoBlockDepth; | ||
|
||
UsdUndoableItem* _undoItem; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit-pick: pointer itself should be const (not the pointed-to data):
UsdUndoableItem* const _undoItem;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, if you initialized the pointer here as nullptr:
UsdUndoableItem* const _undoItem{nullptr};
you could simply define the default constructor in the declaration at line 40:
UsdUndoBlock() = default;
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ppt-adsk Right but we do want to define default constructor in C++ to use delegate constructor. This is because, we want _undoBlockDepth increment also happens in default constructor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great point about the default constructor, must use delegating constructor, missed that.
#define MAYAUSD_UNDO_UNDOMANAGER_H | ||
|
||
#include "ForwardDeclares.h" | ||
#include "UsdUndoableItem.h" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Don't need this one, could be fwd declared.
- Fix typo in readme - Add anonymous namespace - Remove unused include
In this test, MayaUsdProxyStageSetNotice will be send out with a NULL stage object. See line 367 in test_ProxyShapeImport.cpp Added a check to make sure the stage object received from MayaUsdProxyStageSetNotice is always valid.
…ll clean up this if guard once PR121 is out.
|
||
box.Swap(vec); | ||
data->Set(parentPath, fieldName, box); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kxl-adsk Added a function template for the new _PopChild method since we are going to use it in couple of places with different value type. And many thanks again for helping out with this.
void _OnSetFieldDictValueByKeyImpl( | ||
const SdfPath& path, | ||
const TfToken& fieldName, | ||
const TfToken& keyPath); | ||
void _OnSetTimeSampleImpl(const SdfPath& path, double time); | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Super ugly after the clang-format is applied!!!!!! Something to fix at later time.
@@ -101,7 +118,13 @@ void UsdUndoStateDelegate::invertPushTokenChild( | |||
TF_DEBUG(USDMAYA_UNDOSTATEDELEGATE) | |||
.Msg("Inverting push field '%s' of '%s'\n", fieldName.GetText(), parentPath.GetText()); | |||
|
|||
#if 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debugging code?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@@ -114,7 +137,13 @@ void UsdUndoStateDelegate::invertPushPathChild( | |||
TF_DEBUG(USDMAYA_UNDOSTATEDELEGATE) | |||
.Msg("Inverting push field '%s' of '%s'\n", fieldName.GetText(), parentPath.GetText()); | |||
|
|||
#if 1 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Debugging?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking very good, @HamedSabri-adsk. I found one issue with notice registration in stage subject that will have to be fixed.
lib/mayaUsd/ufe/StagesSubject.cpp
Outdated
@@ -306,6 +329,10 @@ void StagesSubject::onStageSet(const MayaUsdProxyStageSetNotice& notice) | |||
StagesSubject::Ptr me(this); | |||
for (auto stage : ProxyShapeHandler::getAllStages()) { | |||
fStageListeners[stage] = TfNotice::Register(me, &StagesSubject::stageChanged, stage); | |||
#if UFE_PREVIEW_VERSION_NUM >= 2029 | |||
fStageListeners[stage] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
fStageListeners
is a map that holds the registration key used to revoke the listener. What you just did is overriding the registration key we set just two lines above for stageChanged. You have to keep both keys.
@ppt-adsk Additionally, I'm wondering why we don't revoke all the listeners in stage subject destructor?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kxl-adsk we should.
|
||
bool UsdUndoBlockCmd::isUndoable() const { return true; } | ||
|
||
void* UsdUndoBlockCmd::creator() { return new UsdUndoBlockCmd(std::move(argUndoItem)); } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Have a look at the constructor signature:
UsdUndoBlockCmd(const UsdUndoableItem& undoableItem);
I don't believe that you will avoid the copy by passing rvalue, since the signature is expecting const lvalue. Changing the signature to pass by value will avoid copy.
UsdUndoBlockCmd(UsdUndoableItem undoableItem);
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
|
||
SetField(path, fieldName, inverse); | ||
|
||
_setMessageAlreadyShowed = false; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I was hoping you won't pick this pattern as a solution to the problem. Consider improving because it's very possible that this reporting will break with time when someone adds more code in here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, I will improve it later. The reason didn't do it was because I was very lazy last night and this meant changing more code. Should be a trivial change.
…traversing an stage. Storing both keys in an array so they can be revoked later.
lib/mayaUsd/undo/UsdUndoBlock.cpp
Outdated
@@ -83,7 +83,7 @@ void UsdUndoBlockCmd::execute(const UsdUndoableItem& undoableItem) | |||
argUndoItem = UsdUndoableItem(); | |||
} | |||
|
|||
UsdUndoBlockCmd::UsdUndoBlockCmd(const UsdUndoableItem& undoableItem) | |||
UsdUndoBlockCmd::UsdUndoBlockCmd(UsdUndoableItem undoableItem) | |||
: _undoItem(undoableItem) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aren't we taking two copies here if we're not std::move'ing into _undoItem, one for the argument temporary, and one for the data member? Perhaps we should be passing by rvalue reference?
http://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#f18-for-will-move-from-parameters-pass-by-x-and-stdmove-the-parameter
Maybe these are things we can tweak later, and should not stop us from merging this branch.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Agreed Pierre. There are many variations and honestly not a huge deal here.
https://www.nextptr.com/tutorial/ta1211389378/beware-of-using-stdmove-on-a-const-lvalue
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed, let's be explicit about the intent of move and change line 87 to (I would hope compiler can deduce it, but well)
_undoItem(std::move(undoableItem));
This way at worst we get only one copy in case:
UsdUndoableItem myItems;
UsdUndoBlockCmd myCmd(myItems);
and zero-copy in case of
UsdUndoableItem myItems;
UsdUndoBlockCmd myCmd(std::move(myItems));
``` warning C4251: 'MayaUsd_v0::UsdUndoableItem::_invertFuncs': class 'std::vector<MayaUsd_v0::UsdUndoableItem::InvertFunc,std::allocator<_Ty>>' needs to have dll-interface to be used by clients of class 'MayaUsd_v0::UsdUndoableItem' ```
This PR introduces the mechanism for restoring USD data model to it's correct state after undo/redo calls. Please see the README.md page for more information.