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.

Sign up for GitHub

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

Fixed crash caused by out of date stage map #1116

Merged
merged 2 commits into from
Jan 27, 2021
Merged

Fixed crash caused by out of date stage map #1116

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
87dc1e5
Fixed crash caused by out of date stage map
Jan 26, 2021
ea81f44
Adjust conditional compilation based on review feedback
Jan 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 20 additions & 5 deletions lib/mayaUsd/ufe/StagesSubject.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -120,7 +120,11 @@ StagesSubject::Ptr StagesSubject::create() { return TfCreateWeakPtr(new StagesSu

bool StagesSubject::beforeNewCallback() const { return fBeforeNewCallback; }

void StagesSubject::beforeNewCallback(bool b) { fBeforeNewCallback = b; }
void StagesSubject::beforeNewCallback(bool b)
{
fBeforeNewCallback = b;
fInvalidStages.clear();
}

/*static*/
void StagesSubject::beforeNewCallback(void* clientData)
Expand Down Expand Up @@ -380,6 +384,18 @@ void StagesSubject::onStageSet(const MayaUsdProxyStageSetNotice& notice)
fStageListeners[stage] = noticeKeys;
}

#ifdef UFE_V2_FEATURES_AVAILABLE
// Now we can send the notifications about stage change.
for (auto& path : fInvalidStages) {
Ufe::SceneItem::Ptr sceneItem = Ufe::Hierarchy::createItem(path);
if (sceneItem) {
Ufe::Scene::instance().notify(Ufe::SubtreeInvalidate(sceneItem));
}
}

fInvalidStages.clear();
#endif

stageSetGuardCount = false;
}
}
Expand All @@ -391,10 +407,9 @@ void StagesSubject::onStageInvalidate(const MayaUsdProxyStageInvalidateNotice& n
#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);
}
Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

Addressed in ea81f44

#endif
}
Expand Down
12 changes: 12 additions & 0 deletions lib/mayaUsd/ufe/StagesSubject.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -25,8 +25,11 @@
#include <pxr/usd/usd/stage.h>

#include <maya/MCallbackIdArray.h>
#include <ufe/path.h>
#include <ufe/ufe.h> // For UFE_V2_FEATURES_AVAILABLE

#include <unordered_set>

PXR_NAMESPACE_USING_DIRECTIVE

namespace MAYAUSD_NS_DEF {
Expand Down Expand Up @@ -98,6 +101,15 @@ class MAYAUSD_CORE_PUBLIC StagesSubject : public TfWeakBase
typedef TfHashMap<UsdStageWeakPtr, NoticeKeys, TfHash> StageListenerMap;
StageListenerMap fStageListeners;

/*! \brief Store invalidated ufe paths during dirty propagation.

We need to delay notification till stage changes, but at that time it could be too costly to
discover what changed in the stage map. Instead, we store all gateway notes that changed
during dirty propagation and send invalidation from compute, when the new stage is set. This
cache is only useful between onStageInvalidate and onStageSet notifications.
*/
std::unordered_set<Ufe::Path> fInvalidStages;

bool fBeforeNewCallback = false;

MCallbackIdArray fCbIds;
Expand Down