Fixed crash caused by out of date stage map #1116
Conversation
|
Linking with #1103 |
lib/mayaUsd/ufe/StagesSubject.cpp
Outdated
| #ifdef UFE_V2_FEATURES_AVAILABLE | ||
| auto p = notice.GetProxyShape().ufePath(); | ||
| if (!p.empty()) { | ||
| Ufe::SceneItem::Ptr sceneItem = Ufe::Hierarchy::createItem(p); | ||
| if (sceneItem) { | ||
| Ufe::Scene::instance().notify(Ufe::SubtreeInvalidate(sceneItem)); | ||
| } | ||
| // We can't send notification to clients from dirty propagation. | ||
| // Delay it till the new stage is actually set during compute. | ||
| fInvalidStages.insert(p); | ||
| } |
There was a problem hiding this comment.
I see you didn't put fInvalidStages inside the ifdef V2 block. So I think that either all the code that uses this var should be inside an ifdef or remove the ifdef from this block. The block above should be ifdef protected because subtreeinvalidate is V2 only. But then line 396 should be moved outside the block.
There was a problem hiding this comment.
It was mostly to leave the header file cleaner since code that is not guarded has no real performance impact (very low-frequency operation on empty set). It also builds with all flavors of the build, because the entire ufe folder is not included when building without UFE.
Let me know if you feel strongly about it and I can add the guards.
There was a problem hiding this comment.
My one concern is that the population of fInvalidStages is done inside an ifdef V2 block. So if someone later adds code to use this fInvalidStages var and that code is not inside a V2 block, the new code will not work since the set will never have anything in it.
When using the
onStageInvalidatenotification, extra care has to be taken to NOT trigger any notification that would result in pulling on stage map. This notification is sent during dirty propagation, and we can't trigger compute from it (number one DG rule).We didn't fully follow this rule and started to send
Ufe::SubtreeInvalidate. One of the listeners, i.e. Outliner, was trying to refresh the tree by pulling on the list of stages. This caused a lack of refresh and what's more important, the potential use of released memory in future calls.The fix is to delay this notification till later when we know the stage map has been already populated.