-
Notifications
You must be signed in to change notification settings - Fork 203
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
Fix orphaned node discard edits with unloaded ancestor. #2638
Conversation
// layer stored as overs, the usdPrim will never be invalid: a prim that | ||
// exists only because of over opinions is valid, but is typeless. | ||
// Therefore, the conditional will always succeed, and | ||
// discardOrphanedEdits() is never called. PPT, 30-Sep-2022. | ||
auto ret = usdPrim ? discardPrimEdits(pulledPath) : discardOrphanedEdits(dagPath, pulledPath); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is very uncomfortable: before the fix, discardOrphanedEdits() was never called. However, the fix requires UFE support.
if(CMAKE_UFE_V3_FEATURES_AVAILABLE) | ||
set(TEST_DISCARD_EDITS testDiscardEdits.py) | ||
mayaUsd_get_unittest_target(target ${TEST_DISCARD_EDITS}) | ||
if(UFE_TRIE_NODE_HAS_CHILDREN_COMPONENTS_ACCESSOR) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Enable test for orphaned node discard edits on ancestor unload only if UFE support is present.
// layer stored as overs, the usdPrim will never be invalid: a prim that | ||
// exists only because of over opinions is valid, but is typeless. | ||
// Therefore, the conditional will always succeed, and | ||
// discardOrphanedEdits() is never called. PPT, 30-Sep-2022. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could we check if it is typeless? Would that help?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That feels weak... And yet another code path to support and test. My hope is that since the required UFE support in Maya is coming very shortly on all Maya versions where this matters (UFE v3 and up), that we simply remove this broken code path soon. Thanks for the thought, though.
} | ||
TF_VERIFY(trieNode->hasData()); | ||
// If the pull parent is visible, the pulled path is not orphaned. | ||
return !getVisibilityPlug(trieNode); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The only caveat I see with this approach, which we may not want to support, is: what if the user has set the node invisible by hand for whatever reason? Here we take for granted that invisible means orphaned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If you touch the pull parent, which by default is not visible in the Outliner, you're voiding your warranty... So yes, invisible pull parent means orphaned.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aside for a few interrogations, this looks fine.
As per MAYA-123378, discarding edits on an orphaned Maya node whose USD ancestor is unloaded causes an error.
Added a unit test for this case. Without the fix, the test fails, with the fix, the test succeeds.