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

MAYA-106376: fix undo/redo re-parenting crash due to accessing a stale prim. #772

Merged
merged 3 commits into from
Sep 14, 2020

Conversation

HamedSabri-adsk
Copy link
Contributor

This PR addresses the random undo/redo crash after performing multiple re-parenting operations.

@HamedSabri-adsk HamedSabri-adsk added ufe-usd Related to UFE-USD plugin in Maya-Usd workflows Related to in-context workflows labels Sep 11, 2020
@HamedSabri-adsk HamedSabri-adsk changed the title MAYA-106376: fix undo/redo crash due to accessing a stale prim. MAYA-106376: fix undo/redo re-parenting crash due to accessing a stale prim. Sep 11, 2020
Copy link
Collaborator

@ppt-adsk ppt-adsk left a comment

Choose a reason for hiding this comment

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

Good catch! In general, any command which stores UFE scene items (or USD prims, same issue) is suspicious.

I think you should simply remove the _ufeSrcItem and _ufeDstItem data members, because they are no longer useful, and doing this would completely remove the possibility of relying on stale prims. Just use local variables where UFE scene items are needed.

@@ -124,16 +124,20 @@ bool UsdUndoInsertChildCommand::insertChildRedo()
bool status = SdfCopySpec(_childLayer, _usdSrcPath, _parentLayer, _usdDstPath);
if (status)
{
// we shouldn't rely on UsdSceneItem to access the UsdPrim since
// it could be stale. Instead we should get the USDPrim from the Ufe::Path
const auto& desPrimUsd = ufePathToPrim(_ufeDstPath);
Copy link
Collaborator

Choose a reason for hiding this comment

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

Same a line 164: this will give the wrong stage at line 134.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Naming nit-pick: should use "dst" or "des" prefix for "destination", but not both.

@HamedSabri-adsk
Copy link
Contributor Author

I think you should simply remove the _ufeSrcItem and _ufeDstItem data members, because they are no longer useful, and doing this would completely remove the possibility of relying on stale prims. Just use local variables where UFE scene items are needed.

Good point but we can't remove _ufeDstItem since it needs to be passed to insertedChild().

For now, I removed _ufeSrcItem since it serves no purpose to be stored. Addressed in c29d1e4

@HamedSabri-adsk HamedSabri-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Sep 14, 2020
@kxl-adsk kxl-adsk merged commit c24b29f into dev Sep 14, 2020
@kxl-adsk kxl-adsk deleted the sabrih/MAYA-106376/undo_crash_fix branch September 14, 2020 19:29
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 ufe-usd Related to UFE-USD plugin in Maya-Usd workflows Related to in-context workflows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants