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

Pr/use usd preview surface for cards draw mode #2680

Merged

Conversation

dj-mcg
Copy link
Collaborator

@dj-mcg dj-mcg commented Oct 25, 2022

No description provided.

@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Oct 25, 2022

In order to get them to render in RenderMan, cards are now drawn using UsdPreviewSurface. Since UsdImaging is now responsible for drawing cards, this allows us to remove some work being done in render delegates, but it looks like there may be an issue with backface culling in the Vp2RenderDelegate that is causing imaging issues using this new approach. We've installed version guards to preserve this old behavior until the underlying work in the next Usd release, but this should be addressed to avoid breakage once this change rolls out

@tgvarik
Copy link

tgvarik commented Oct 26, 2022

Some additional details:

Prior to these changes, cards were drawn with a custom shader, and the vp2 render delegate reimplemented that shader with an additional input parameter to which a mayaIsBackFacing node got connected. The reimplemented shader then performed a discard based on that input.

After these changes, cards are drawn with UsdPreviewSurface, and there may be as many as six such materials for a given cards representation. The old approach to culling the backsides of the cards doesn't really fit anymore.

Hydra communicates backface culling preference on the HdSceneDelegate interface via GetCullStyle(), and this is how we request culling of the backfaces of cards. Once these changes land in USD we should look into getting the vp2 render delegate to apply backface culling generically based on this published info.

@seando-adsk seando-adsk added pxr Related to Pixar plugin vp2renderdelegate Related to VP2RenderDelegate labels Oct 26, 2022
@seando-adsk
Copy link
Collaborator

@dj-mcg There were some build errors you'll need to fix.

@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Oct 26, 2022

Thanks @seando-adsk - I see them but I don't have a Windows build environment handy. I had hoped to be able to identify the problem just by eyeballing the changes but I'm not seeing it offhand - if you see the issue can you let me know, otherwise I'll keep poking

@seando-adsk
Copy link
Collaborator

@dj-mcg You have access to the build logs by clicking "Details" next to the failing preflight check. Then click "Summary" on left hand side and scroll down the bottom of the page. There is a "build_logs" link. Open that .zip file and then look at "xxx_result.txt" which tells you all the jobs that failed. Then you can open each of those logs (from that zip) and see the exact build error(s).

Let me know if you are able to find the logs and see the errors.

Sean

@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Oct 26, 2022

I found the errors but I was a bit confused by them. It looks like we can't have the version guard directive inside the token definitions on Windows. I'll push an attempted fix to see if I'm right

Windows doesn't seem to like the version guard directives within the
private token definition
@seando-adsk
Copy link
Collaborator

Yes that is true "TF_DEFINE_PRIVATE_TOKENS" is a #define and you cannot have a #if inside of that. What we usually do is a use a #if/#else

Copy link
Collaborator

@JGamache-autodesk JGamache-autodesk left a comment

Choose a reason for hiding this comment

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

Changes look good. I am a bit surprised that none of the reference images in test\lib\mayaUsd\render\vp2RenderDelegate\VP2RenderDelegateDrawModesTest\baseline\ got updated. Can you confirm this test produces identical rendering with USD >= 21.11?

@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Oct 27, 2022

Hi @JGamache-autodesk - please check out the comment by @tgvarik above:
#2680 (comment)

With the latest Usd changes (which may still be internal to Pixar), cards draw mode for Vp2RenderDelegate actually breaks due to how backface culling is currently handled. So, as you assumed, the test results did change and currently appear broken. We'll need to update Vp2Rd to comply with the new approach to backface culling since custom shaders are no longer needed/used, but wanted to work with you to determine the best approach before the next Usd release lands

@JGamache-autodesk
Copy link
Collaborator

Ok, I'll guide you with the update if you have some time to try.

We usually do backface culling by feeding the MayaIsBackfacing fragment into a passthrough function that discards early.
The Nw input would probably be the best one to use.
Connections would be: MayaIsBackfacing (out) -> (isBackfacing) BackfaceNormalCuller (out) -> (Nw) UsdPreviewSurface
And the code of the backface culler frqagment would be:

vec3 backfaceNormalCuller(vec3 Nw, bool isBackfacing) {
    if (isBackfacing) discard;
    return Nw;
}

Would be nice if these only got connected in card mode, otherwise regular planes will also get the backface treatment. This would require a way to know that the UsdPreviewSurface currently being processed is for drawing cards.

You can see a similar structure in the pre-22.11 code where we add a MayaIsBackface that will allow the UsdDrawModeCards fragment to know when to discard.

@tgvarik
Copy link

tgvarik commented Oct 28, 2022

Is there any reason culling cannot be applied generically, based on the return value of sceneDelegate->GetCullStyle()? This is how Hydra communicates culling and is available on everything with the HdSceneDelegate interface.

@JGamache-autodesk
Copy link
Collaborator

That would be an interesting thing to add. I don't see any reason why it could not be used. Are the cards supposed to respect that setting or do they override it?

@tgvarik
Copy link

tgvarik commented Oct 28, 2022

For the cards adapter, sceneDelegate->GetCullStyle() will always return HdCullStyleBack, indicating backfaces should be culled. For most other prims, it returns HdCullSytleDontCare, which just means Hydra has no opinion on culling for that prim, and the renderer should follow its own default. But there are a few places where it's something different. HdCullStyle can be any of:

  • HdCullStyleDontCare
  • HdCullStyleNothing
  • HdCullStyleBack
  • HdCullStyleFront
  • HdCullStyleBackUnlessDoubleSided
  • HdCullStyleFrontUnlessDoubleSided

Note that cull style isn't currently expressible in USD. It's something Hydra uses to help achieve consistent behavior across diverse renderers.

@JGamache-autodesk
Copy link
Collaborator

Slightly more involved, but still possible. Might be better to always install the culling node, and add an integer input for cullStyle. The OpenGL code in the cull node will then be able to discard or not based on combinations of cullStyle and isBackfacing.

Setting the shader values will be done in HdVP2Material::CompiledNetwork::_UpdateShaderInstance() where access to the sceneDelegate is provided. There will most probably be a need to have an IsCullingNode() function to call to properly set the value of cullMode for the sceneDelegate. We have a similar workflow to set texture parameters. The value of isBackfacing is computed automatically and will not appear in the list of shader parameters.

@seando-adsk
Copy link
Collaborator

@dj-mcg Is this PR ready to be merged or would there be new changes based on the discussions above?

@dj-mcg
Copy link
Collaborator Author

dj-mcg commented Oct 31, 2022

I'm ok with merging this now since it's version guarded. However, the culling changes will need to land with or prior to the next Usd release (likely 23.02) or cards won't draw correctly in vp2 render delegate.

@seando-adsk seando-adsk added the ready-for-merge Development process is finished, PR is ready for merge label Oct 31, 2022
@seando-adsk seando-adsk merged commit ba969ad into Autodesk:dev Oct 31, 2022
@dj-mcg dj-mcg deleted the pr/Use_UsdPreviewSurface_For_Cards_DrawMode branch October 31, 2022 19:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pxr Related to Pixar plugin 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.

5 participants