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.

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

Reduced dirty notifications for isolate select. #239

Merged
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -174,6 +174,7 @@ void ViewportInformationAndSceneIndicesPerViewportDataManager::DisableIsolateSel
)
{
_isolateSelection[viewportId] = nullptr;
_isolateSelectSceneIndex->SetViewport(viewportId, nullptr);
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

}

SelectionPtr
Expand Down
63 changes: 55 additions & 8 deletions lib/flowViewport/sceneIndex/fvpIsolateSelectSceneIndex.cpp
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -108,6 +108,18 @@ bool isGeomSubset(const HdSceneIndexPrim& prim) {
#endif
}

HdContainerDataSourceHandle setInstancerMaskDataSource(
const HdSceneIndexPrim& inputPrim,
const VtArray<bool>& instanceMask
)
{
auto maskDs = HdRetainedTypedSampledDataSource<VtArray<bool>>::New(instanceMask);

return HdContainerDataSourceEditor(inputPrim.dataSource)
.Set(instancerMaskLocator, maskDs)
.Finish();
}

}

namespace FVP_NS_DEF {
Expand Down Expand Up @@ -159,11 +171,8 @@ HdSceneIndexPrim IsolateSelectSceneIndex::GetPrim(const SdfPath& primPath) const

auto instancerMask = _instancerMasks.find(primPath);
if (instancerMask != _instancerMasks.end()) {
auto maskDs = HdRetainedTypedSampledDataSource<VtArray<bool>>::New(instancerMask->second);

inputPrim.dataSource = HdContainerDataSourceEditor(inputPrim.dataSource)
.Set(instancerMaskLocator, maskDs)
.Finish();
inputPrim.dataSource =
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Factored out to unnamed namespace.

setInstancerMaskDataSource(inputPrim, instancerMask->second);

return inputPrim;
}
Expand All @@ -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
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

// an instance mask that is entirely false.
if (inputPrim.primType == HdPrimTypeTokens->instancer) {
auto instancerTopologySchema = HdInstancerTopologySchema::GetFromParent(inputPrim.dataSource);
const auto nbInstances = getNbInstances(instancerTopologySchema);
VtArray<bool> instanceMask(nbInstances, false);

inputPrim.dataSource = setInstancerMaskDataSource(inputPrim, instanceMask);
}

// Unclear whether instancers need their visibility off. As of OpenUSD
// 0.24.11, Hydra Storm seems to ignore visibility for instancers, but
// there seems to be no harm in setting it false.

inputPrim.dataSource = HdContainerDataSourceEditor(inputPrim.dataSource)
.Set(HdVisibilitySchema::GetDefaultLocator(), visOff)
.Finish();
Expand Down Expand Up @@ -437,21 +460,45 @@ void IsolateSelectSceneIndex::_DirtyVisibilityRecursive(
HdSceneIndexObserver::DirtiedPrimEntries* dirtiedEntries
) const
{
TF_DEBUG(FVP_ISOLATE_SELECT_SCENE_INDEX)
.Msg(" %s: marking %s visibility locator dirty.\n", _viewportId.c_str(), primPath.GetText());

// If the prim is a material, early out: visibility is not
// relevant for materials
auto prim = GetInputSceneIndex()->GetPrim(primPath);
if (prim.primType == HdPrimTypeTokens->material) {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't set materials as invisible, and don't recurse.

return;
}

// GeomSubset visibility must not be set (see GetPrim()), so no need to
// dirty it.
if (isGeomSubset(prim)) {
return;
}

TF_DEBUG(FVP_ISOLATE_SELECT_SCENE_INDEX)
.Msg(" %s: marking %s visibility locator dirty.\n", _viewportId.c_str(), primPath.GetText());

// Unclear whether instancers need their visibility off. As of OpenUSD
// 0.24.11, Hydra Storm seems to ignore visibility for instancers, but
// there seems to be no harm in setting it false.

dirtiedEntries->emplace_back(
primPath, HdVisibilitySchema::GetDefaultLocator());

// If the prim is an instancer, set its mask dirty, but don't recurse, as
// the hierarchy beneath an instancer is its prototypes, whose visibility
// is managed by the instancer mask data source.
if (prim.primType == HdPrimTypeTokens->instancer) {
_AddDirtyInstancerMaskEntry(primPath, dirtiedEntries);
return;
}

for (const auto& childPath : GetChildPrimPaths(primPath)) {
#if PXR_VERSION >= 2403
// Recursing down to set visibility on a geomSubset child is wasteful.
auto childPrim = GetInputSceneIndex()->GetPrim(childPath);
if (childPrim.primType == HdPrimTypeTokens->geomSubset) {
continue;
}
#endif
_DirtyVisibilityRecursive(childPath, dirtiedEntries);
}
}
Expand Down
35 changes: 35 additions & 0 deletions lib/mayaHydra/mayaPlugin/renderOverride.cpp
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

Original file line number Diff line number Diff line change
Expand Up @@ -1831,15 +1831,50 @@ void MtohRenderOverride::_ViewSelectedChangedCb(
return;
}

auto found = instance->_isolateSelectState.find(viewName.asChar());
if (found == instance->_isolateSelectState.end()) {
found = instance->_isolateSelectState.insert(found, VpIsolateSelectStates::value_type(viewName.asChar(), IsolateSelectState::IsolateSelectOff));
}

// The M3dView returns the list of view selected objects as strings.
// If isolate select is turned off, we want to disable isolate selection.
// Otherwise, replace with what is in the M3dView.
auto& vpDataMgr = Fvp::ViewportDataMgr::Get();
if (!view.viewSelected()) {
vpDataMgr.DisableIsolateSelection(viewName.asChar());
found->second = IsolateSelectState::IsolateSelectOff;
return;
}

// Deal with IsolateSelectState changes. The messages we receive are
// viewSelectedObjectsChanged true or false.
//
// State transitions:
//
// off: message false --> go to pendingObjects, do not notify.
// off: message true --> illegal, warn, do not notify.
//
// pendingObjects: message false --> illegal, warn, do not notify.
// pendingObjects: message true --> notify, go to on.
//
// on: message false --> go to off, notify.
Copy link
Collaborator

@debloip-adsk debloip-adsk Jan 20, 2025

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

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.

// on: message true --> stay on, notify.

if (found->second == IsolateSelectState::IsolateSelectOff) {
if (TF_VERIFY(!viewSelectedObjectsChanged)) {
found->second = IsolateSelectState::IsolateSelectPendingObjects;
}
return;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Early out and don't notify, wait for the second notification.

}
else if (found->second == IsolateSelectState::IsolateSelectPendingObjects) {
if (!TF_VERIFY(viewSelectedObjectsChanged)) {
return;
}
found->second = IsolateSelectState::IsolateSelectOn;
}

TF_VERIFY(found->second == IsolateSelectState::IsolateSelectOn);

// The M3dView returns the list of view selected objects as strings.
// Loop over the view selected objects and try to create UFE paths from
// them. If there is more than one element to the MStringArray, this
Expand Down
16 changes: 16 additions & 0 deletions lib/mayaHydra/mayaPlugin/renderOverride.h
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@
#include <memory>
#include <mutex>
#include <vector>
#include <map>

#include <ufe/ufe.h>
UFE_NS_DEF {
Expand Down Expand Up @@ -338,6 +339,21 @@ class MtohRenderOverride : public MHWRender::MRenderOverride,
#ifdef MAYA_HAS_VIEW_SELECTED_OBJECT_API
long int _nbViewSelectedChangedCalls{0};
#endif

// Maya has an awkward notification mechanism for isolate select,
// with a view selected objects changed boolean that indicates
// whether the state has changed (false), or the isolate selected
// objects have changed (true). When changing a viewport from
// isolate select off to on, two notifications are therefore sent,
// first false (state change), then true (objects set). To avoid
// double dirtying in Hydra, we track the following isolate select
// states per viewport:
//
enum class IsolateSelectState {IsolateSelectOff, IsolateSelectPendingObjects,
IsolateSelectOn};

using VpIsolateSelectStates = std::map<std::string, IsolateSelectState>;
VpIsolateSelectStates _isolateSelectState;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Per-viewport isolate select notification state tracking.

};

PXR_NAMESPACE_CLOSE_SCOPE
Expand Down