-
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
MAYA-103701 - UFE: Maya crashes after switching variant #726
MAYA-103701 - UFE: Maya crashes after switching variant #726
Conversation
…tching variant * Removed special case for add/remove reference and instead created special case for add or delete operations. In these cases we send the UFE add/delete notifs. Otherwise we fallback and send the UFE subtree invalidate notif. * In the tests that add new prims and test delete, added a UFE observer to make sure we are getting the correct notifs.
//! \brief Helper class to scope when we are in an add or remove reference operation. | ||
class InAddOrRemoveReference | ||
//! \brief Helper class to scope when we are in an add or delete operation. | ||
class InAddOrDeleteOperation |
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.
As we discussed, removed the special case for add/remove reference and instead made a special case for add/delete of prims.
if (prim.IsActive()) | ||
// Special case when we know the operation came from either | ||
// the add or delete of our UFE/USD implementation. | ||
if (InAddOrDeleteOperation::inAddOrDeleteOperation()) |
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.
As we discussed, if we are in an add/delete prim operation, we'll send the appropriate add/delete UFE notif.
#if UFE_PREVIEW_VERSION_NUM >= 2014 | ||
// According to USD docs for GetResyncedPaths(): | ||
// - Resyncs imply entire subtree invalidation of all descendant prims and properties. | ||
// So we send the UFE subtree invalidate notif. | ||
auto notification = Ufe::SubtreeInvalidate(sceneItem); | ||
Ufe::Scene::notifySubtreeInvalidate(notification); | ||
#endif |
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.
Otherwise send the UFE subtree invalidate. Question: do we need anything for UFE < 0.2.14?
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.
No. We only support the current Maya preview release, and it has 0.2.20. We are supposed to go back and clean out these conditionals, but we rarely have.
@@ -135,15 +133,13 @@ class AddReferenceUndoableCommand : public Ufe::UndoableCommand | |||
|
|||
void undo() override { | |||
if (_prim.IsValid()) { | |||
MayaUsd::ufe::InAddOrRemoveReference ar; |
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.
Removed special case for add/remove reference. This will still send the UFE subtree invalidate, since it will not fall into the add/delete prim case.
|
||
|
||
# This test requires no additional setup. | ||
if self._testMethodName == 'testAddNewPrim': |
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 add new prim test only needs a proxy shape (not the top layer scene).
import mayaUsd_createStageWithNewLayer | ||
mayaUsd_createStageWithNewLayer.createStageWithNewLayer() |
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.
Move here from the setup()
ufeObs = TestAddPrimObserver() | ||
ufe.Scene.addObjectDeleteObserver(ufeObs) | ||
ufe.Scene.addObjectAddObserver(ufeObs) |
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.
Use a UFE observer helper class to make sure when we add/remove prims we get the proper UFE add/delete notifs.
# Create our UFE notification observer | ||
ufeObs = TestObserver() | ||
ufe.Scene.addObjectDeleteObserver(ufeObs) | ||
ufe.Scene.addObjectAddObserver(ufeObs) |
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.
Same thing for the delete operation (which does the prim deactivate/activate)
@@ -209,10 +263,14 @@ def testDeleteArgs(self): | |||
self.assertNotIn(sphereShapeName, sphereChildrenNames) | |||
self.assertNotIn(ball35Name, propsChildrenNames) | |||
self.assertNotIn(ball34Name, propsChildrenNames) | |||
self.assertFalse(cmds.ls("|pSphere1|pSphereShape1")) | |||
self.assertFalse(cmds.objExists("|pSphere1|pSphereShape1")) |
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 ls command is actually undoable. So this test wasn't really testing the undo of the cmds.delete from line 250. My new test case caught this since the notifs weren't right.
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.
Funny thing is that objExists says its also undoable (in docs), but this case works.
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.
Nice catch! Uh, objExists is undoable? What does that mean? It's a pure query... Maybe if you undo objExists and it returned true, the object will become an anti-matter version of itself? I'm struggling with this, I admit.
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.
Great tests and a nice simplification. As discussed now we have guaranteed correctness regardless of whether you're using the UFE or the USD API, with finer-grained UFE semantics where possible.
#if UFE_PREVIEW_VERSION_NUM >= 2014 | ||
// According to USD docs for GetResyncedPaths(): | ||
// - Resyncs imply entire subtree invalidation of all descendant prims and properties. | ||
// So we send the UFE subtree invalidate notif. | ||
auto notification = Ufe::SubtreeInvalidate(sceneItem); | ||
Ufe::Scene::notifySubtreeInvalidate(notification); | ||
#endif |
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.
No. We only support the current Maya preview release, and it has 0.2.20. We are supposed to go back and clean out these conditionals, but we rarely have.
} | ||
else | ||
{ | ||
#if UFE_PREVIEW_VERSION_NUM >= 2014 |
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.
You don't need this any more, we're at 2020.
@@ -209,10 +263,14 @@ def testDeleteArgs(self): | |||
self.assertNotIn(sphereShapeName, sphereChildrenNames) | |||
self.assertNotIn(ball35Name, propsChildrenNames) | |||
self.assertNotIn(ball34Name, propsChildrenNames) | |||
self.assertFalse(cmds.ls("|pSphere1|pSphereShape1")) | |||
self.assertFalse(cmds.objExists("|pSphere1|pSphereShape1")) |
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.
Nice catch! Uh, objExists is undoable? What does that mean? It's a pure query... Maybe if you undo objExists and it returned true, the object will become an anti-matter version of itself? I'm struggling with this, I admit.
MAYA-103701 - UFE: Maya crashes when selecting a new object after switching variant