MAYA-111834 - Crash when transforming multiple objects with duplicate stage #1554
Conversation
… stage
* Replaced TF_AXIOM (which abort when fail) with TF_VERIFY
to allow Add{Scale/Translate/Rotate} calls that already exist
to return without crashing.
| 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(); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@ppt-adsk What do you think?
There was a problem hiding this comment.
@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.
There was a problem hiding this comment.
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.
| // 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(); |
There was a problem hiding this comment.
See my comment down below. Same change made for Scale/Rotate/Translate/Pivot.
seando-adsk
requested review from
HamedSabri-adsk,
kxl-adsk and
ppt-adsk
and removed request for
HamedSabri-adsk
… stage * Reworked all transform op commands to: ** Check for no edit allowed first and return null command. ** Combine two lambda OpFunc into one which will use attribute if it exists, otherwise will create one.
… stage * Factor out error checking into helper method.
| bool UsdTransform3dMayaXformStack::isAttributeEditAllowed(const PXR_NS::TfToken attrName, std::string& errMsg) const | ||
| { | ||
| UsdAttribute attr; | ||
| if (!attrName.IsEmpty()) | ||
| attr = prim().GetAttribute(attrName); | ||
| if (attr && !MayaUsd::ufe::isAttributeEditAllowed(attr, &errMsg)) { | ||
| return false; | ||
| } else if (!attr) { | ||
| UsdGeomXformable xformable(prim()); | ||
| if (!MayaUsd::ufe::isAttributeEditAllowed(xformable.GetXformOpOrderAttr(), &errMsg)) { | ||
| return false; | ||
| } | ||
| } | ||
| return true; | ||
| } |
There was a problem hiding this comment.
Helper to factor out some common error checking code.
| auto f = OpFunc( | ||
| [attrName, opSuffix = getTRSOpSuffix(), setXformOpOrderFn = getXformOpOrderFn(), v]( | ||
| const BaseUndoableCommand& cmd) { | ||
| auto usdSceneItem = std::dynamic_pointer_cast<UsdSceneItem>(cmd.sceneItem()); | ||
| TF_AXIOM(usdSceneItem); | ||
| auto attr = usdSceneItem->prim().GetAttribute(attrName); | ||
| if (attr) { | ||
| return UsdGeomXformOp(attr); | ||
| } else { | ||
| // Use notification guard, otherwise will generate one notification | ||
| // for the xform op add, and another for the reorder. | ||
| InTransform3dChange guard(cmd.path()); | ||
| auto usdSceneItem = std::dynamic_pointer_cast<UsdSceneItem>(cmd.sceneItem()); | ||
| TF_AXIOM(usdSceneItem); | ||
| UsdGeomXformable xformable(usdSceneItem->prim()); | ||
|
|
||
| auto r = xformable.AddRotateXYZOp(UsdGeomXformOp::PrecisionFloat, opSuffix); | ||
| TF_AXIOM(r); | ||
| r.Set(v); | ||
| auto result = setXformOpOrderFn(xformable); | ||
| TF_AXIOM(result); | ||
|
|
||
| return r; | ||
| } | ||
| }); |
There was a problem hiding this comment.
Combined two lambda OpFunc into one. First check if the prim already has the attribute. If yes, then use it to create UsdGeomXformOp. Otherwise first add the attribute and then use the newly added one. Same changes made in all the command methods.
seando-adsk
added
bug
… stage * clang-format
seando-adsk
added
the
ready-for-merge
kxl-adsk
deleted the
donnels/MAYA-111834/crash_transforming_with_shared_stage
branch
… stage