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

Support explicitly authored bitangents in GLSL backend #1156

Merged

Conversation

pablode
Copy link
Contributor

@pablode pablode commented Nov 25, 2022

The <bitangent> node implementation of the GLSL backend computes bitangents on-demand, and ignores existing, explicitly authored bitangents of an asset's geometry.

This PR changes the behaviour of the <bitangent> node to solely return an (optionally transformed) bitangent that is provided by the runtime.

It is now the runtime's responsibility to provide an input bitangent for this purpose, which is in line with how it's done in the MDL and OSL backends. The runtime is not necessarily forced to provide a geometry's authored bitangent; it could also compute the "B=NxT" fallback value like the <bitangent> node formerly did.
For example, the HdStorm renderer calculates the tangent from the normal, but it could as well read it from the geometry. The same should be done for the bitangent by renderers implementing the GLSL backend. If the bitangent ends up not being used, the calculation is optimized away.

Unfortunately, MaterialXView lacks the capability to check for the existence of geometric properties at shader compilation time. Hence, in order to support explicit bitangents, it is assumed that geometry bitangents always exist, with fallback bitangents being generated up-front on the CPU-side, just like we already do for tangents.

I've verified the results with the document proposed in PR #1155. Here's a comparison using the NormalTangentMirrorTest glTF model, which has explicitly authored tangents.

Result from glTF Viewer:

Before this PR:

After this PR:

Note that this is a breaking change, meaning that every runtime using the GLSL backend is now required to provide a bitangent vector in order to support the <bitangent> node, and in the future, the <normalmap> node.

@jstone-lucasfilm
Copy link
Member

This looks like a good proposal to me, @pablode, and I'd agree that support for authored bitangents is a necessary step forward. I'm CC'ing @klucknav and @meshula for their thoughts on the implications of this proposed change for Hydra delegates, and I'm additionally including @niklasharrysson and @kwokcb for their thoughts and recommendations.

@kwokcb
Copy link
Contributor

kwokcb commented Dec 6, 2022

This looks like a good proposal to me, @pablode, and I'd agree that support for authored bitangents is a necessary step forward. I'm CC'ing @klucknav and @meshula for their thoughts on the implications of this proposed change for Hydra delegates, and I'm additionally including @niklasharrysson and @kwokcb for their thoughts and recommendations.

MDL and OSL wrappers AFAIK are using intrinsic inputs, which I assume would be computed when required at some ray-intersection point. The change here appears to require that all meshes now must load in a bitangent to allow an explicit input so I don't think can work consistently for MDL and OSL without also changing those implementations.

I agree that there is no per-geometry check for the existence of explicit streams such as bitangent but the flip-side to this is that if this is done (which it could) then the shader in question would become specific to the geometry. One alternative I see is to add an "explicit" vs "implicit" flag to bitangent (and even tangent input). Then the same shader can be used but a input argument set. Without a connection the current behaviour is set. With a connection a custom user computation or a precompueted stream could used. The former can allow for all the "fun" variants folks have for what to encode in tangents such as encoding w with "sign".

Third, is that there can be shaders without the need for a bitangent / tangent or normal for that matter and having the requirement to always require these can be large overhead especially for a complex model / scene. The front end asking for additional streams when not required is more of integration thing I believe and could be fixed in MaterialXView which binds without checking for stream requirements. Anyways some work can be done here to make it "smarter".

@jstone-lucasfilm
Copy link
Member

@kwokcb I believe this proposal is restricted to GLSL generation, and should not impact MDL or OSL rendering.

@kwokcb
Copy link
Contributor

kwokcb commented Dec 6, 2022

I just mentioned MDL and OSL since I'm not really sure if this makes things more consistent or not. Bitangents are computed in MDL and OSL (I assume) and exposed as a predefined intrinsic which seems to be what the GLSL implementation "mimics". This change makes it so that it instead comes from a user defined geometric stream which can use a set index. However, as far as I see multiple stream sets are not allowed in MDL or OSL so this is an existing inconsistency and to support the explicit stream option being added I assume the MDL and OSL implementations would also need to allow this ?

@jstone-lucasfilm
Copy link
Member

@pablode We had a chance to discuss this proposal in today's MaterialX TSC, and here's a quick summary of the takeaways:

  • In order to make this a non-breaking change, could it instead be implemented as a shader generation option, allowing each renderer to choose between authored and auto-generated bitangents?
  • MaterialXView could then select authored bitangents, loading them from geometry when present, and generating them in C++ as a fallback.
  • HdStorm and other external clients could then continue to use auto-generated bitangents, giving them a straightforward upgrade path.

As a reference point, here is a shader generation option that similarly supports two different code paths for different real-time rendering environments:
https://github.com/AcademySoftwareFoundation/MaterialX/blob/main/source/MaterialXGenShader/GenOptions.h#L170

@pablode
Copy link
Contributor Author

pablode commented Dec 13, 2022

@jstone-lucasfilm Thank you for discussing this proposal, and I've implemented the shader generation option above!

Copy link
Member

@jstone-lucasfilm jstone-lucasfilm left a comment

Choose a reason for hiding this comment

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

This looks like a great proposal to me, @pablode, and I'll just CC @niklasharrysson and @kwokcb for their thoughts, since this is closely related to their contributions to the project.

@jstone-lucasfilm jstone-lucasfilm merged commit 29cac85 into AcademySoftwareFoundation:main Dec 22, 2022
Michaelredaa pushed a commit to Michaelredaa/MaterialX that referenced this pull request Oct 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants