VP2RenderDelegate support for USD curves #228

huidong-chen · 2020-01-30T23:28:54Z

This PR implements the VP2RenderDelegate support for USD curves, as well as some derisking work on tessellation shader approach for curve refinements.

Refinement level 0 is fully implemented to get parity with USDView, which simply shows the curves as line list.
Refinement level 1 is partially implemented as part of the work to derisk tessellation shader approach for curve refinement. For now it only interpolates widths and normals. The work is currently disabled until the new MRenderItem::setPrimitive() API is shipped.
Refinement level 2 and 3 are not implemented, which I think is only about more complex algorithm than level 1 and I've already set up the code framework to host new algorithms.

The refactoring change to HdVP2Mesh and HdVP2DrawItem is necessary because the draw item should not be tied with mesh and should be Rprim type agnostic.

…mitive() is published.

lib/render/vp2RenderDelegate/mesh.cpp

cfmoore007 · 2020-02-03T21:20:07Z

Usability Tests ( conducted in Windows 10, latest maya beta with VS2017 for compiler, and this PR merged with latest dev branch commits ) :

The current functionality - does not look like it supports display color
Selection seems to work ( at the prim level )
Visually, it looks correct beyond the displayColor note

Christopher

lib/render/vp2RenderDelegate/basisCurves.cpp

lib/render/vp2RenderDelegate/draw_item.cpp

lib/render/vp2RenderDelegate/draw_item.h

lib/render/vp2RenderDelegate/render_delegate.cpp

huidong-chen · 2020-02-04T23:55:43Z

@mattyjams Will you have time to review this PR please? If not, I can remove you from reviewers because William and Krystian has reviewed it. Thanks.

cfmoore007 · 2020-02-05T01:06:08Z

The displayColor now seems to come through.
However, wanted to mention an issue with shaded mode selection highlighting of the curves with instanced objects. Wireframe seems fine.

Sorry, can't share the data set publically, but wanted to mention the issue.

Thanks!

Christopher

huidong-chen · 2020-02-05T01:49:24Z

@cfmoore007 Thanks for testing. Could you log the instanced curve issue please? I need to switch to some other task at the moment and in the meantime I will also collect all feedbacks about curve before further planning. Appreciated your help and test scenes.

huidong-chen · 2020-02-05T02:01:15Z

@kxl-adsk Ready to merge.

kxl-adsk · 2020-02-05T14:10:32Z

@cfmoore007 I wonder how to make it more visible that there was validation done by BlueSky. Maybe I can just set you as a reviewer and once you are happy with your testing you approve it?

cfmoore007 · 2020-02-05T17:28:54Z

@kxl-adsk - we tested on our own data set in house.

I just tested if it was visible in the viewport, looked 'roughly correct' ( we did not do direct before/after, point by point analysis ), and selection.

Still have to test manipulation ( will try that later this evening )

As for visual example - we'll work on a simple example for both this, and the instanced selection problem that we can share here.
Hopefully, we'll get it done soon ...

cfmoore007 · 2020-02-06T00:37:12Z

Visual Results :

BasisCurves Example :
basisCurves.zip

Tests conducted :

manipulation works ( given current known bugs )
framing
displayColor ( after fb60543 was committed )
wireframe / shaded mode / boundingBox

cfmoore007

Approving based on internal testing.

robthebloke · 2020-02-05T23:42:25Z

lib/render/vp2RenderDelegate/basisCurves.cpp

+        int curveIndex = 0;
+        TF_FOR_ALL(itCounts, vertexCounts) {
+            for(int i = 0; i < *itCounts; i+= 2) {
+                indices.push_back(GfVec2i(vertexIndex, vertexIndex + 1));


indices.emplace_back(vertexIndex, vertexIndex + 1);

However that is still a pretty terrible way to populate an array :(

The utility functions are copied from HdSt:

https://github.com/PixarAnimationStudios/USD/blob/dev/pxr/imaging/hdSt/basisCurvesComputations.cpp

I think the code can be kept the same for now and when we need to make necessary change to these functions we can then refine them.

robthebloke · 2020-02-05T23:43:39Z

lib/render/vp2RenderDelegate/basisCurves.cpp

+                v0 = v1;
+            }
+            if(wrap) {
+                indices.push_back(GfVec2i(v0, firstVert));


indices.emplace_back(v0, firstVert);

The utility functions are copied from HdSt:

https://github.com/PixarAnimationStudios/USD/blob/dev/pxr/imaging/hdSt/basisCurvesComputations.cpp

I think the code can be kept the same for now and when we need to make necessary change to these functions we can then refine them.

robthebloke · 2020-02-05T23:51:58Z

lib/render/vp2RenderDelegate/basisCurves.cpp

+                    indices.push_back(GfVec2i(v0, v1));
+                }
+                v0 = v1;
+            }


This for loop is pretty unpleasant. For a start, if you are testing skipFirstAndLastSegs on every single iteration, and that was set prior to the first loop, then please move the condition outside the loop, e.g.

if(skipFirstAndLastSegs) { // loop that skips first and last seg TF_FOR_ALL(itCounts, vertexCounts) {} } else { // loop that does the opposite. TF_FOR_ALL(itCounts, vertexCounts) {} }

and then tidy up the inner loop a little.

The utility functions are copied from HdSt:

https://github.com/PixarAnimationStudios/USD/blob/dev/pxr/imaging/hdSt/basisCurvesComputations.cpp

I think the code can be kept the same for now and when we need to make necessary change to these functions we can then refine them.

robthebloke · 2020-02-05T23:54:44Z

lib/render/vp2RenderDelegate/basisCurves.cpp

+                }
+                v0 = v1;
+            }
+            if(wrap) {


again, testing exactly the same boolean value each iteration within a loop. Send a bit of love to the branch predictor, and move the condition above the loop.

The utility functions are copied from HdSt:

https://github.com/PixarAnimationStudios/USD/blob/dev/pxr/imaging/hdSt/basisCurvesComputations.cpp

I think the code can be kept the same for now and when we need to make necessary change to these functions we can then refine them.

robthebloke · 2020-02-05T23:58:35Z

lib/render/vp2RenderDelegate/basisCurves.cpp

+            // Store first vert index incase we are wrapping
+            const int firstVert = v0;
+            ++ vertexIndex;
+            for(int i = 1;i < *itCounts; ++i) {


Please don't dereference an iterator as the terminating condition of a loop. The var exists outside the scope of this method, so now the optimiser will fail to apply any loop optimisations. Take a local copy of the value and use that instead.

The utility functions are copied from HdSt:

https://github.com/PixarAnimationStudios/USD/blob/dev/pxr/imaging/hdSt/basisCurvesComputations.cpp

I think the code can be kept the same for now and when we need to make necessary change to these functions we can then refine them.

robthebloke · 2020-02-06T02:49:56Z

lib/render/vp2RenderDelegate/basisCurves.cpp

+        MHWRender::MShaderInstance* _shader{ nullptr };
+
+        //! Is this object transparent
+        bool _isTransparent{ false };


Move this boolean to be last in the struct. Will avoid 7 bytes of pointless padding.

Fixed with f13a3e1.

robthebloke · 2020-02-06T03:16:41Z

lib/render/vp2RenderDelegate/basisCurves.cpp

+        const VtArray<int> vertexCounts = topology.GetCurveVertexCounts();
+        bool wrap = topology.GetCurveWrap() == HdTokens->periodic;
+        int vStep;
+        TfToken basis = topology.GetCurveBasis();


robthebloke · 2020-02-06T03:16:52Z

lib/render/vp2RenderDelegate/basisCurves.cpp

+        std::vector<GfVec4i> indices;
+
+        const VtArray<int> vertexCounts = topology.GetCurveVertexCounts();
+        bool wrap = topology.GetCurveWrap() == HdTokens->periodic;


robthebloke · 2020-02-06T03:38:40Z

lib/render/vp2RenderDelegate/basisCurves.cpp

+            for(int i = 1;i < *itCounts; ++i) {
+                v1 = vertexIndex;
+                ++ vertexIndex;
+                if (!skipFirstAndLastSegs || (i > 1 && i < (*itCounts)-1)) {


Why are you looping from 1 to *itCounts, and then checking to see if i is in a different range?

doThingWhen(i == 0); doThingWhen(i == 1); for(int i = 2, n = *itCounts - 1; i < n; ++i) { indices.emplace_back(v0, v1); } doThingWhen(i == *itCounts - 1);

It is from HdSt.

robthebloke · 2020-02-06T03:41:07Z

lib/render/vp2RenderDelegate/basisCurves.cpp

+                v1 = vertexIndex;
+                ++ vertexIndex;
+                if (!skipFirstAndLastSegs || (i > 1 && i < (*itCounts)-1)) {
+                    indices.push_back(GfVec2i(v0, v1));


indices.emplace_back(v0, v1);

Although if you can resize indices to the correct size up front, this code would be an order of magnitude faster than using emplace_back here.

* Reduce unnecessary byte alignment padding. * Reduce unnecessary allocation/deallocation. * More efficient matrix math.

huidong-chen · 2020-02-11T14:18:55Z

Summary on the good discussion with @robthebloke #228 (comment):

About avoiding code dependency or duplication with HdSt, I've discussed with George Elkoura in usd interest group and logged issue PixarAnimationStudios/OpenUSD#1112, which requires to factor out common geometry processing codes from HdSt to a common library and allows other render delegates to link to it. At this point, I don't think it is worthy for us to refine our duplicated code.

@kxl-adsk Could we merge this PR please?

VP2RenderDelegate now supports USD curves

79cb741

huidong-chen requested review from mattyjams and kxl-adsk January 30, 2020 23:29

Huidong Chen added 2 commits January 31, 2020 11:15

Curve refinement is not currently supported until MRenderItem::setPri…

e97f577

…mitive() is published.

Fix compilation error in strict build mode.

0e59271

kxl-adsk requested a review from williamkrick January 31, 2020 20:10

Huidong Chen added 2 commits January 31, 2020 16:22

Cleanup for shader fragment xml files.

a801943

USD curves can provide snapping points.

1e1994b

williamkrick approved these changes Feb 3, 2020

View reviewed changes

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

kxl-adsk approved these changes Feb 4, 2020

View reviewed changes

Fix shader errors when assigning material to USD curves

fb60543

Code review feedback

4dd8081

Fix compilation error on macOS

94967f1

kxl-adsk requested a review from cfmoore007 February 5, 2020 17:34

cfmoore007 approved these changes Feb 6, 2020

View reviewed changes

huidong-chen removed the request for review from mattyjams February 6, 2020 00:43

robthebloke reviewed Feb 6, 2020

View reviewed changes

Code review feedback.

f13a3e1

* Reduce unnecessary byte alignment padding. * Reduce unnecessary allocation/deallocation. * More efficient matrix math.

kxl-adsk merged commit 4a8f0a7 into dev Feb 11, 2020

huidong-chen deleted the chenh/MAYA-101845/usd-geom-curves branch February 11, 2020 15:54

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

VP2RenderDelegate support for USD curves #228

VP2RenderDelegate support for USD curves #228

huidong-chen commented Jan 30, 2020 •

edited

Loading

cfmoore007 commented Feb 3, 2020

huidong-chen commented Feb 4, 2020

cfmoore007 commented Feb 5, 2020

huidong-chen commented Feb 5, 2020

huidong-chen commented Feb 5, 2020

kxl-adsk commented Feb 5, 2020

cfmoore007 commented Feb 5, 2020 •

edited

Loading

cfmoore007 commented Feb 6, 2020 •

edited

Loading

cfmoore007 left a comment

robthebloke Feb 5, 2020

huidong-chen Feb 6, 2020

robthebloke Feb 5, 2020

huidong-chen Feb 6, 2020

robthebloke Feb 5, 2020

huidong-chen Feb 6, 2020

robthebloke Feb 5, 2020

huidong-chen Feb 6, 2020

robthebloke Feb 5, 2020

huidong-chen Feb 6, 2020

robthebloke Feb 6, 2020

huidong-chen Feb 7, 2020

robthebloke Feb 6, 2020

robthebloke Feb 6, 2020

robthebloke Feb 6, 2020

huidong-chen Feb 6, 2020

robthebloke Feb 6, 2020

huidong-chen commented Feb 11, 2020

VP2RenderDelegate support for USD curves #228

VP2RenderDelegate support for USD curves #228

Conversation

huidong-chen commented Jan 30, 2020 • edited Loading

cfmoore007 commented Feb 3, 2020

huidong-chen commented Feb 4, 2020

cfmoore007 commented Feb 5, 2020

huidong-chen commented Feb 5, 2020

huidong-chen commented Feb 5, 2020

kxl-adsk commented Feb 5, 2020

cfmoore007 commented Feb 5, 2020 • edited Loading

cfmoore007 commented Feb 6, 2020 • edited Loading

cfmoore007 left a 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

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

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

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

huidong-chen commented Feb 11, 2020

huidong-chen commented Jan 30, 2020 •

edited

Loading

cfmoore007 commented Feb 5, 2020 •

edited

Loading

cfmoore007 commented Feb 6, 2020 •

edited

Loading