Reduced dirty notifications for isolate select. #239
Conversation
| @@ -174,6 +174,7 @@ void ViewportInformationAndSceneIndicesPerViewportDataManager::DisableIsolateSel | |||
| ) | |||
| { | |||
| _isolateSelection[viewportId] = nullptr; | |||
| _isolateSelectSceneIndex->SetViewport(viewportId, nullptr); | |||
There was a problem hiding this comment.
Fixed latent bug that was being compensated for in renderOverride.cpp. The Flow Viewport Toolkit should not rely on clients to manage the isolate select scene index properly when it can do so itself.
There was a problem hiding this comment.
Core file in the review. Instancers that were completely excluded were being excluded by virtue of their prototype visibility being set false, which is wasteful. Now, we simply use the instancer mask to make all instances invisible.
| inputPrim.dataSource = HdContainerDataSourceEditor(inputPrim.dataSource) | ||
| .Set(instancerMaskLocator, maskDs) | ||
| .Finish(); | ||
| inputPrim.dataSource = |
There was a problem hiding this comment.
Factored out to unnamed namespace.
| @@ -179,6 +188,20 @@ HdSceneIndexPrim IsolateSelectSceneIndex::GetPrim(const SdfPath& primPath) const | |||
| // HYDRA-1242: setting visibility on GeomSubset prim causes hang in Hydra | |||
| // Storm. | |||
| if (!included && !isGeomSubset(inputPrim)) { | |||
| // If an instancer is not included, none of its instances are, so set | |||
There was a problem hiding this comment.
Mark all instances as invisible by using the instancer mask. We still set the instancer itself as invisible, even though for Hydra Storm this does not seem to have any effect.
| auto prim = GetInputSceneIndex()->GetPrim(primPath); | ||
| if (prim.primType == HdPrimTypeTokens->material) { |
There was a problem hiding this comment.
Don't set materials as invisible, and don't recurse.
There was a problem hiding this comment.
Deal with awkward Maya notification system that uses a boolean to distinguish between isolate select state changes and isolate select object changes, sending two notifications on first transition from on to off. We account for this and only dirty once.
| if (TF_VERIFY(!viewSelectedObjectsChanged)) { | ||
| found->second = IsolateSelectState::IsolateSelectPendingObjects; | ||
| } | ||
| return; |
There was a problem hiding this comment.
Early out and don't notify, wait for the second notification.
| IsolateSelectOn}; | ||
|
|
||
| using VpIsolateSelectStates = std::map<std::string, IsolateSelectState>; | ||
| VpIsolateSelectStates _isolateSelectState; |
There was a problem hiding this comment.
Per-viewport isolate select notification state tracking.
| // pendingObjects: message false --> illegal, warn, do not notify. | ||
| // pendingObjects: message true --> notify, go to on. | ||
| // | ||
| // on: message false --> go to off, notify. |
There was a problem hiding this comment.
Just to be sure, is line 1845 handling this case? Asking since "message" in this comment corresponds to viewSelectedObjectsChanged, but when IsolateSelectState is already on, I don't see any code that sets it to off based on the value of viewSelectedObjectsChanged, only based on view.viewSelected(). Maybe one implies the other but just want to make sure.
There was a problem hiding this comment.
Correct, I am assuming that view.viewSelected() false is consistent with a transition from isolate select on to a message with viewSelectedObjectsChanged false. Empirical validation says this is the case, so line 1845 is handling it.
ppt-adsk
added
the
ready-for-merge
lilike-adsk
deleted the
tremblp/HYDRA-1341/isolate_select_fewer_dirty_notifications
branch
Makes isolate select notifications easier to understand, and potentially improves performance by avoiding pulling on data when not necessary. Performance has not been measured, only correctness.