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.

Sign up for GitHub

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

Jump to bottom

MAYA-111834 - Crash when transforming multiple objects with duplicate stage #1554

Merged
merged 4 commits into from
Jul 14, 2021
Merged

MAYA-111834 - Crash when transforming multiple objects with duplicate stage #1554

Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
8e3081d
MAYA-111834 - Crash when transforming multiple objects with duplicate…
seando-adsk Jul 6, 2021
d788538
MAYA-111834 - Crash when transforming multiple objects with duplicate…
seando-adsk Jul 13, 2021
3f8b445
MAYA-111834 - Crash when transforming multiple objects with duplicate…
seando-adsk Jul 13, 2021
f272210
MAYA-111834 - Crash when transforming multiple objects with duplicate…
seando-adsk Jul 13, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
40 changes: 36 additions & 4 deletions lib/mayaUsd/ufe/UsdTransform3dMayaXformStack.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -476,8 +476,13 @@ UsdTransform3dMayaXformStack::rotateCmd(double x, double y, double z)
return UsdGeomXformOp();
}

// See comment in setVector3dCmd().
auto r = xformable.AddRotateXYZOp(UsdGeomXformOp::PrecisionFloat, opSuffix);
TF_AXIOM(r);
if (!TF_VERIFY(
r,
"RotateXYZOp already exists for %s",
usdSceneItem->path().string().c_str()))
return UsdGeomXformOp();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

See my comment down below. Same change made for Scale/Rotate/Translate/Pivot.

r.Set(v);
auto result = setXformOpOrderFn(xformable);
TF_AXIOM(result);
Expand Down Expand Up @@ -530,8 +535,13 @@ Ufe::ScaleUndoableCommand::Ptr UsdTransform3dMayaXformStack::scaleCmd(double x,
return UsdGeomXformOp();
}

// See comment in setVector3dCmd().
auto s = xformable.AddScaleOp(UsdGeomXformOp::PrecisionFloat, opSuffix);
TF_AXIOM(s);
if (!TF_VERIFY(
s,
"ScaleOp already exists for %s",
usdSceneItem->path().string().c_str()))
return UsdGeomXformOp();
s.Set(v);
auto result = setXformOpOrderFn(xformable);
TF_AXIOM(result);
Expand Down Expand Up @@ -657,8 +667,20 @@ Ufe::SetVector3dUndoableCommand::Ptr UsdTransform3dMayaXformStack::setVector3dCm
return UsdGeomXformOp();
}

// MAYA-111834 - Crash when transforming multiple objects with duplicate stage
// When sharing a stage if you have the same object selected from both proxy shapes
// and you perform a manip (such as translate) we end up with this OpFunc for both
// since the GetAttribute() above returns nothing. Now when we execute these OpFunc
// commands the first will succeed and add the translate op, but the second will
// error and return an empty UsdGeomXformOp (since a TranslateOp already exists).
// Therefore we detect (and allow) this case.
// TODO - What is the expected behavior in this case? Should the object move twice?
auto op = xformable.AddTranslateOp(OpPrecision<V>::precision, opSuffix);
TF_AXIOM(op);
if (!TF_VERIFY(
op,
"TranslateOp already exists for %s",
cmd.sceneItem()->path().string().c_str()))
return UsdGeomXformOp();
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

You'll see on lines 631-633 it checks for the existence of the attribute and then creates a OpFunc (from lambda) for either using existing attribute op, or creating one first (this second lamdba) and then using the newly created one. The AddTranslateOp will output an error message if one already exists and return an empty UsdGeomXformOp. The previous test at line 661 would then abort because of the empty op. I changed it to a TF_VERIFY which just prints a message and then returns empty op. So crash is avoided.

Copy link

Choose a reason for hiding this comment

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

We will fall into this situation only on the first manipulation when ops are missing. Every consecutive manipulation will perform double edit.

We can make the behavior consistent and re-query the op when the addition failed. Why did you choose to issue a warning instead?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Because I didn't know what the expected behavior is supposed to be. You are right about the consecutive manips will be double - I'm not sure this is expected either.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@ppt-adsk What do you think?

Copy link
Collaborator

Choose a reason for hiding this comment

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

@kxl-adsk I am not sure I understand your comment. The core issue is the fact that two UFE paths refer to the same underlying USD object. If you create n UFE scene items for a Maya command that have this property, then you will get n edits (2 in the case of a duplicated stage). It is difficult for this mayaUsd code to identify this situation --- we would be violating separation of concerns, IMO. That is for the double edit part of the discussion.

For the add / don't add a transform op part of the discussion, I'm not sure what consistent behavior is. On command creation we determined there was no transform op, and on command execution a transform op appeared. For unique objects, this would be a bug caused by invalid operation of the software. In the current case, with multiple UFE scene items pointing to the same USD prim, we can indeed deal with the problem by assuming all is well and querying the op --- and the result will be a double edit. I believe users will find this unexpected.

Copy link
Collaborator

Choose a reason for hiding this comment

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

After discussion with Krystian we will separate out the behavior from preventing the crash. To simplify debugging the behavior in the future, we'll make the create transform op failure code path simply try accessing the transform op, per Krystian's suggestion.

op.Set(v);
auto result = setXformOpOrderFn(xformable);
TF_AXIOM(result);
Expand Down Expand Up @@ -706,6 +728,11 @@ UsdTransform3dMayaXformStack::pivotCmd(const TfToken& pvtOpSuffix, double x, dou
TF_AXIOM(usdSceneItem);
UsdGeomXformable xformable(usdSceneItem->prim());
auto p = xformable.AddTranslateOp(UsdGeomXformOp::PrecisionFloat, pvtOpSuffix);
if (!TF_VERIFY(
p,
"TranslateOp already exists for %s",
usdSceneItem->path().string().c_str()))
return UsdGeomXformOp();

// At this point we already know that writing to attribute is going
// to succeed but we do not know if writing to "transform op order" succeed since
Expand All @@ -716,9 +743,14 @@ UsdTransform3dMayaXformStack::pivotCmd(const TfToken& pvtOpSuffix, double x, dou
return UsdGeomXformOp();
}

// See comment in setVector3dCmd().
auto pInv = xformable.AddTranslateOp(
UsdGeomXformOp::PrecisionFloat, pvtOpSuffix, /* isInverseOp */ true);
TF_AXIOM(p && pInv);
if (!TF_VERIFY(
pInv,
"TranslateOp (inverse) already exists for %s",
usdSceneItem->path().string().c_str()))
return UsdGeomXformOp();
p.Set(v);
auto result = setXformOpOrderFn(xformable);
TF_AXIOM(result);
Expand Down