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

T bailp/maya 123815/reparent on deactivated #2419

Merged
merged 3 commits into from
Jun 19, 2022

Conversation

pierrebai-adsk
Copy link
Collaborator

No description provided.

When the USD prim is deactivated, many operations will crash if called on that prim, as the USD prim API assumes that many of its functions are illegal to call on an invalid prim abd deactivated prim are invalid.

(Internally, the UsdPrim contains a pointer which is null when invalid, and most function don't check the pointer, instead relying on the caller to not call them when invalid.)

So check the prim validity in parent() and when creating sibling iterators.
@seando-adsk seando-adsk requested a review from ppt-adsk June 14, 2022 21:36
seando-adsk
seando-adsk previously approved these changes Jun 14, 2022
Copy link
Collaborator

@seando-adsk seando-adsk left a comment

Choose a reason for hiding this comment

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

The change looks good to me. I've added PierreT as another review just to ensure I didn't miss anything.

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.

I'd move some of the checks to the command creation code. Can you confirm that ProxyShapeHierarchy does not need any changes, because it corresponds to the pseudo-root, which cannot be inactivated?

Comment on lines +255 to +256
else
return Hierarchy::createItem(fItem->path().pop());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Not sure why this is correct. If we have an invalid prim, shouldn't we return a nullptr as its parent?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I hit this case while investigating. I did not write down the exact circumstances (I think you could use the repro steps of the Maya JIRA ticket with a breakpoint in this function without my code changes.) The path parent is valid, but not the path. It was breaking checking if a node had children, IIRC.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I can see that happening with the child of an inactive prim. Attempting to use that prim would be invalid, its parent would not. I suspect we are hiding a bug with this code... We are not checking that the parent path is valid. I would return a nullptr and see what breaks.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The call was correct. It was trying to get the parent of the deactivated prim, IIRC. The deactivated prim has a parent, so we should not fail the call.

Copy link
Collaborator

Choose a reason for hiding this comment

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

But the deactivated prim is still valid, so we should never get into that branch of the conditional.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Indeed, and yet I got into that situation, otherwise I'd not change the code :). IDK how I got there during my investigations, honestly. (and unfortunately, blame my bad memory)

Made prim validity tests less redundants.
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.

We can merge as-is, and you can investigate the code path where a parent is returned for an invalid prim whose parent path is valid later.

@pierrebai-adsk
Copy link
Collaborator Author

Only failure is the known Linux 2022 interactive test.

@pierrebai-adsk pierrebai-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Jun 17, 2022
@seando-adsk seando-adsk merged commit 1f35bdb into dev Jun 19, 2022
@seando-adsk seando-adsk deleted the t_bailp/MAYA-123815/reparent-on-deactivated branch June 19, 2022 15:34
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