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

MAYA-121408 Trigger the populate selection when a new instancer is added #2038

Merged

Conversation

williamkrick
Copy link
Contributor

or removed. This covers some of the workflows that change the instance indexing, but not all.

#ifdef ENABLE_RENDERTAG_VISIBILITY_WORKAROUND
_visibilityVersion = 0;
#endif
_changeVersions.reset();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I refactored how these "versions" of things from USD are tracked into a single struct to keep it simpler.

Choose a reason for hiding this comment

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

Much nicer, thx!

{
_renderTag = tracker.GetRenderTagVersion();
_visibility = tracker.GetVisibilityChangeCount();
_instanceIndex = tracker.GetInstancerIndexVersion();
Copy link
Contributor Author

Choose a reason for hiding this comment

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

GetInstanceIndexVersion doesn't capture all the workflows we care about, but it captures some of them. So at least some things work correctly with this updated version of the code.

@williamkrick williamkrick requested a review from kxl-adsk January 27, 2022 19:56
@williamkrick williamkrick added the vp2renderdelegate Related to VP2RenderDelegate label Jan 27, 2022
kxl-adsk
kxl-adsk previously approved these changes Jan 28, 2022
#ifdef ENABLE_RENDERTAG_VISIBILITY_WORKAROUND
_visibilityVersion = 0;
#endif
_changeVersions.reset();

Choose a reason for hiding this comment

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

Much nicer, thx!

@kxl-adsk kxl-adsk requested a review from vlasovi January 28, 2022 09:41
@williamkrick
Copy link
Contributor Author

The failing test is testVP2RenderDelegatePerInstanceInheritedData in Maya 2022 with USD 20.05. That is a test I would expect to potentially fail with these changes, but the differing image is inheritedDisplayColor_noPxrMtls_unselected.png which I would not expect to fail. I tried running the test locally in Maya 2022 and USD 20.05, and it passed for me. I'll try running preflight again.

@williamkrick
Copy link
Contributor Author

These changes help with the performance issue in #1991.

@vlasovi
Copy link
Collaborator

vlasovi commented Jan 28, 2022

@williamkrick, based on the autotest png files, it looks like your PR changes what objects are selected. Why is that? Since your first preflight failed, I just want to make sure we are not introducing here some random behaviour.

@williamkrick
Copy link
Contributor Author

@williamkrick, based on the autotest png files, it looks like your PR changes what objects are selected. Why is that? Since your first preflight failed, I just want to make sure we are not introducing here some random behaviour.

@vlasovi this change is intentionally changing the behavior of this test. What is happening is USD is telling the VP2RenderDelegate which instances of the instancer are selected. When that instancer is modified by adding or removing instances (by hiding them or causing them to not be drawn in some other way) USD doesn't update its internal information about which instances are selected. That causes the selection to "jump" from one instance to another.

We work around this issue of drawing the selection incorrectly by calling populateSelection again. Calling populateSelection gets the instance to update its internal state about which instance indices are selected. Calling populateSelection is slow, and we don't have complete dirty information telling us when we need to call it and when we don't need to call it.

Originally I had code in place always calling populate selection. Once we discovered it was too slow I created a PR to stop doing the work around (#2034). This change uses the incomplete dirty information to get the selection right some of the time, but not all of the time. We're still fast, and we're more correct than we were before. I also have a PR against USD to provide access to the additional dirty information we need to be correct all the time (PixarAnimationStudios/OpenUSD#1754).

@vlasovi
Copy link
Collaborator

vlasovi commented Feb 1, 2022

This change uses the incomplete dirty information to get the selection right some of the time, but not all of the time.

So probably that means that we may have some random behaviour here. But since we know about that and since we have a plan to fix that later, I think we can move forward with the change.

vlasovi
vlasovi previously approved these changes Feb 1, 2022
krickw added 2 commits February 1, 2022 11:26
…ed. This covers some of the workflows that change the instance indexing, but not all.
@williamkrick williamkrick dismissed stale reviews from vlasovi and kxl-adsk via bf29d65 February 1, 2022 16:43
@williamkrick williamkrick force-pushed the krickw/MAYA-121408/improve_temporary_populate_selection_code branch from bc4df50 to bf29d65 Compare February 1, 2022 16:43
@williamkrick williamkrick added the ready-for-merge Development process is finished, PR is ready for merge label Feb 1, 2022
@kxl-adsk kxl-adsk merged commit 66b77ee into dev Feb 1, 2022
@kxl-adsk kxl-adsk deleted the krickw/MAYA-121408/improve_temporary_populate_selection_code branch February 1, 2022 19:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
ready-for-merge Development process is finished, PR is ready for merge vp2renderdelegate Related to VP2RenderDelegate
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants