Skip to content
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

Matrix op, common API undo / redo. #1586

Merged
merged 2 commits into from
Jul 28, 2021

Conversation

ppt-adsk
Copy link
Collaborator

@ppt-adsk ppt-adsk commented Jul 19, 2021

The goal of this pull request is to bring matrix op and common API Transform3d interfaces to the new UndoManager undo / redo capability. This will completely remove on undo any attrSpec's and primSpec's added during Transform3d edits. This capability was already present in Maya transform stacks and Maya fallback transform stacks.

@@ -247,7 +252,7 @@ void StagesSubject::stageChanged(
const TfToken nameToken = changedPath.GetNameToken();
auto usdPrimPathStr = changedPath.GetPrimPath().GetString();
auto ufePath = stagePath(sender) + Ufe::PathSegment(usdPrimPathStr, g_USDRtid, '/');
if (nameToken == UsdGeomTokens->xformOpOrder) {
if (isTransformChange(nameToken)) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Info-only changes had a different test from resync changes, which caused a bug during development of this branch, as we were missing a notification on undo. Factored out the correct notification test into a function, and used in in both places.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ppt-adsk To be clear, the bug was due to the fact that the check for UsdGeomXformOp::IsXformOp(nameToken) was missing?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct.

}

void undo() override { _commonAPI.SetScale(_prevS, _time); }
void redo() override { _commonAPI.SetScale(_s, _time); }
void setValue(const GfVec3f& v) override { _commonAPI.SetScale(v, writeTime()); }

// Executes the command by setting the rotation onto the transform op.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ppt-adsk typo: "setting the rotation" ----> "setting the scale"

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Darned copy-paste... Good catch! Actually, this comment and the ones in the other classes were really low value, I just removed them locally.

@HamedSabri-adsk
Copy link
Contributor

@ppt-adsk

While testing this PR, I noticed getting an error message when doing group undo/redo. The data model looks fine but the error message annoys me. I wonder if it is something to do with my recent group changes or if it is something else. I am going to look into today.

Error: Illegal handleUndo() call in UsdTRSUndoableCmdBase for state 'struct MayaUsd_v0::ufe::anonymous namespace'::UsdTRSUndoableCmdBase::UndoneState'.`

@@ -40,6 +41,8 @@ namespace {
using namespace MayaUsd;
using namespace MayaUsd::ufe;

void warnUnimplemented(const char* msg) { TF_WARN("Illegal call to unimplemented %s", msg); }
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ppt-adsk I don't see warnUnimplemented is being used?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Another good catch! Refactored to UsdSetXformOpUndoableCommandBase.cpp, but forgot to remove the original. Removed locally.

@ppt-adsk
Copy link
Collaborator Author

@ppt-adsk

While testing this PR, I noticed getting an error message when doing group undo/redo. The data model looks fine but the error message annoys me. I wonder if it is something to do with my recent group changes or if it is something else. I am going to look into today.

Error: Illegal handleUndo() call in UsdTRSUndoableCmdBase for state 'struct MayaUsd_v0::ufe::anonymous namespace'::UsdTRSUndoableCmdBase::UndoneState'.`

O.K., that sounds great, as the assert says this really should not happen: a call to undo() is being generated while we're already in the undone state.

Copy link
Contributor

@HamedSabri-adsk HamedSabri-adsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@ppt-adsk
Copy link
Collaborator Author

Pre-flight failed only on macOS Python 3 master, on a bizarre licensing error:

File "/Library/Frameworks/Python.framework/Versions/2.7/lib/python2.7/shutil.py", line 97, in copyfile
with open(dst, 'wb') as fdst:
IOError: [Errno 2] No such file or directory: '/local/S/jenkins/workspace/shared/Artifactory/OSX/rundev-master/202107191347/runtime/1a4b6f0/runTime/Maya.app/Contents/Licensing/melicense.lic'

This is clearly a build infrastructure issue, so this branch is ready to merge.

@ppt-adsk ppt-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Jul 20, 2021
@kxl-adsk kxl-adsk merged commit 629610e into dev Jul 28, 2021
@kxl-adsk kxl-adsk deleted the tremblp/MAYA-108709/all_transform3d_use_undo_manager branch July 28, 2021 13:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge Development process is finished, PR is ready for merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants