MAYA-104542 Handle rprim purpose changing to a hidden purpose correctly #558

williamkrick · 2020-06-03T13:14:36Z

No description provided.

williamkrick · 2020-06-03T13:16:41Z

lib/mayaUsd/render/vp2RenderDelegate/proxyRenderDelegate.cpp

+        // Set the render tags to be all the tags. Vp2RenderDelegate implements
+        // render tags as a per-render item setting, so we always need to visit
+        // all the rprims to handle cases when an rprim changes from a displayed
+        // tag to a hidden tag.


The performance downside of this change is for animated rprims which have a purpose which is not displayed. In that case we'll do all the updates for those rprims and then not draw the item, which is unfortunate.

We might be able to work around it by having a special case for visibility, where we update the visibility of an item first. Then, for hidden items don't bother updating anything else, just leave it all dirty.

With this change, it sounds like purpose will only allow us to save on drawing, but not on the cost to sync changes? What's the implication for memory usage of big static scenes, time to the first frame, etc?

I think you are right we will create all the guide objects and then not draw them. Basically we have to poll every rprim every frame to see if it goes its visibility dirtied because it's purpose changed.

I checked again how HdSt handles the render tag on an rprim changing to a hidden tag:

HdChangeTracker::MarkRprimDirty() is called with DirtyRenderTag as one of the dirty bits, which increments rprimIndexVersion.

In HdSt_RenderPass::_PrepareDrawItems() they detect the change in rprimIndexVersion and rebuild the list of draw items to draw (filtered by render tags).

This is what we are missing. We don't have an equivalent search over all the draw items to only update their "visibility". Items get hidden by being left out of the _drawItems list, and therefore excluded from their draw batches in _PrepareCommandBuffers.
I'm trying use our existing pass over all the rprims in execute to turn items on and off as necessary, to do that I explicitly need to consider each MRenderItem and turn on or off its visibility.

I can improve it with a combination of two changes:

Have an early exit in Sync for rprims that are hidden by render tags, leaving the dirty bits intact so they update correctly when made visible. That prevents creating Maya stuff for guide items which are not drawn.

Only force the pass over all render tags when that HdChangeTracker::rprimIndexVersion has changed. That prevents visiting everything and checking its visibility every refresh (I'm assuming we'd have lots of dirty + hidden stuff because we skip the update for hidden things). We'd only visit the stuff that passes the render tags test.

lib/mayaUsd/render/vp2RenderDelegate/proxyRenderDelegate.cpp

lib/mayaUsd/render/vp2RenderDelegate/mesh.cpp

This reverts commit 0a07070.

williamkrick · 2020-06-03T19:06:23Z

MayaUsd - Build External GitHub Branch run 142

cfmoore007 · 2020-06-04T13:59:18Z

Here are my results ( note this post is updated from earlier ) :

Results are basically the same.

Christopher

…ep the set of render tags on our dummy render task small.

williamkrick · 2020-06-04T20:08:38Z

There is a clear improvement in shaded guide display before/after.

Christopher

@cfmoore007 Your performance results surprised me, I wasn't expecting anything to go faster.

I'm haven't been able to reproduce your slow result with 38c3ea in Guide Purpose mode. My numbers before and after the change are similar to your 38c3ea+PR558 numbers. Am I correct in assuming that when you did this test only Guide Puropose and not Proxy Purpose? Anything else to reproduce this beyond create the proxy shape and set the file name?

williamkrick · 2020-06-04T20:14:48Z

lib/mayaUsd/render/vp2RenderDelegate/mesh.cpp

+        // marking visibility dirty instead of render tag. See UsdImagingGprimAdapter::ProcessPropertyChange
+        // for where this happens.
+        *dirtyBits &= ~( HdChangeTracker::DirtyRenderTag | HdChangeTracker::DirtyVisibility );
+        return;


This is the risky bit of the change. We don't have much experience or tests leaving items dirty & hidden. I tested a scene with animation where I change the purpose of the items so they are hidden, change time, and change the purpose so they are drawn, and that simple workflow is correct.

This pattern will have to be repeated in the other HdVP2 shapes because they all have the same problem. Ideally we can find a way for the different shapes to share code.

williamkrick · 2020-06-04T20:15:46Z

lib/mayaUsd/render/vp2RenderDelegate/mesh.cpp

+            continue;
+
+        _delegate->GetVP2ResourceRegistry().EnqueueCommit([renderItem]() {
+            renderItem->enable(false);


Setting enable can change which world the renderItem in, we need those changes to be serialized so use the commit system to do it from the main thread.

Soon we will need ThreadRun to free main-thread and get processing of commits started as soon as possible.

williamkrick · 2020-06-04T20:19:45Z

lib/mayaUsd/render/vp2RenderDelegate/proxyRenderDelegate.cpp

-        // any newly visible rprims will update and mark themselves visible.
+    // When the render tag on an rprim changes we do a pass over all rprims to update
+    // their visibility. The frame after we do the pass over all the tags, set the tags back to
+    // the minimum set of tags.


In the new version we call Execute once with all the tags (to hide those visible->hidden rprims) and the next update we go back to calling Execute with the minimum set of render tags.

cfmoore007 · 2020-06-04T21:00:38Z

There is a clear improvement in shaded guide display before/after.
Christopher

@cfmoore007 Your performance results surprised me, I wasn't expecting anything to go faster.

I'm haven't been able to reproduce your slow result with 38c3ea in Guide Purpose mode. My numbers before and after the change are similar to your 38c3ea+PR558 numbers. Am I correct in assuming that when you did this test only Guide Puropose and not Proxy Purpose? Anything else to reproduce this beyond create the proxy shape and set the file name?

I've updated my previous post. Not sure what happened, but I can confirm the roughly match

I did my tests in both guide and proxy. And, yes all I did was create the proxyShape and hit 'F' to frame the viewport.
Viewport size was the same in all tests.

kxl-adsk · 2020-06-08T10:44:24Z

lib/mayaUsd/render/vp2RenderDelegate/mesh.cpp

+            continue;
+
+        _delegate->GetVP2ResourceRegistry().EnqueueCommit([renderItem]() {
+            renderItem->enable(false);


Soon we will need ThreadRun to free main-thread and get processing of commits started as soon as possible.

kxl-adsk · 2020-06-08T10:46:12Z

lib/mayaUsd/render/vp2RenderDelegate/mesh.h

@@ -102,6 +102,8 @@ class HdVP2Mesh final : public HdMesh {
        const HdMeshReprDesc& desc,
        bool requireSmoothNormals, bool requireFlatNormals);

+    void _HideAllDrawItems(const TfToken& reprToken);


We haven't adopted _ as our naming convention for methods. No need to change it now, but we should stop using it.

Handle rprim purpose changing to a hidden purpose correctly.

4346939

williamkrick requested review from huidong-chen and kxl-adsk June 3, 2020 13:14

williamkrick commented Jun 3, 2020

View reviewed changes

lib/mayaUsd/render/vp2RenderDelegate/proxyRenderDelegate.cpp Show resolved Hide resolved

williamkrick commented Jun 3, 2020

View reviewed changes

lib/mayaUsd/render/vp2RenderDelegate/proxyRenderDelegate.cpp Show resolved Hide resolved

kxl-adsk added the vp2renderdelegate Related to VP2RenderDelegate label Jun 3, 2020

kxl-adsk assigned ecp-maya-devops-adsk Jun 3, 2020

Selection hi-lite items respect purpose.

0a07070

huidong-chen reviewed Jun 3, 2020

View reviewed changes

lib/mayaUsd/render/vp2RenderDelegate/mesh.cpp Outdated Show resolved Hide resolved

kxl-adsk unassigned ecp-maya-devops-adsk Jun 3, 2020

huidong-chen mentioned this pull request Jun 3, 2020

MAYA-104981 Disable all degenerated render items. #559

Merged

Revert "Selection hi-lite items respect purpose."

51e6b1b

This reverts commit 0a07070.

williamkrick requested a review from huidong-chen June 3, 2020 17:26

huidong-chen approved these changes Jun 3, 2020

View reviewed changes

kxl-adsk assigned ecp-maya-devops-adsk and unassigned ecp-maya-devops-adsk Jun 4, 2020

Don't update mesh rprims that are hidden by render tags (purpose). Ke…

7665260

…ep the set of render tags on our dummy render task small.

kxl-adsk added the do-not-merge-yet Development is not finished, PR not ready for merge label Jun 4, 2020

williamkrick commented Jun 4, 2020

View reviewed changes

williamkrick requested a review from huidong-chen June 6, 2020 19:44

huidong-chen approved these changes Jun 8, 2020

View reviewed changes

kxl-adsk approved these changes Jun 8, 2020

View reviewed changes

kxl-adsk removed the do-not-merge-yet Development is not finished, PR not ready for merge label Jun 8, 2020

kxl-adsk merged commit 4ea2f25 into dev Jun 8, 2020

kxl-adsk deleted the krickw/MAYA-104542/handle_rprim_purpose_changed branch June 8, 2020 12:25

kxl-adsk mentioned this pull request Jun 8, 2020

[MAYA-104542] proxyShape does not draw nodes with assigned purpose #285

Closed

williamkrick mentioned this pull request Jun 12, 2020

Changing purpose doesn't mark the render tags on a prim dirty PixarAnimationStudios/OpenUSD#1243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MAYA-104542 Handle rprim purpose changing to a hidden purpose correctly #558

MAYA-104542 Handle rprim purpose changing to a hidden purpose correctly #558

williamkrick commented Jun 3, 2020

williamkrick Jun 3, 2020

kxl-adsk Jun 3, 2020

williamkrick Jun 4, 2020

williamkrick commented Jun 3, 2020

cfmoore007 commented Jun 4, 2020 •

edited

Loading

williamkrick commented Jun 4, 2020

williamkrick Jun 4, 2020

williamkrick Jun 4, 2020

williamkrick Jun 4, 2020

kxl-adsk Jun 8, 2020

williamkrick Jun 4, 2020

cfmoore007 commented Jun 4, 2020

kxl-adsk Jun 8, 2020

kxl-adsk Jun 8, 2020

MAYA-104542 Handle rprim purpose changing to a hidden purpose correctly #558

MAYA-104542 Handle rprim purpose changing to a hidden purpose correctly #558

Conversation

williamkrick commented Jun 3, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

williamkrick commented Jun 3, 2020

cfmoore007 commented Jun 4, 2020 • edited Loading

williamkrick commented Jun 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfmoore007 commented Jun 4, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cfmoore007 commented Jun 4, 2020 •

edited

Loading