-
Notifications
You must be signed in to change notification settings - Fork 398
Add TBN inputs to gltf_normalmap #2457
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
base: main
Are you sure you want to change the base?
Add TBN inputs to gltf_normalmap #2457
Conversation
Interesting. The top-level For what it's worth, in the actual glTF file format, we've imposed a limitation: A single material can have only one "tangent basis" in glTF. We don't expect realtime renderers to calculate multiple TBN matrices per fragment. So for example, if the base normalmap and the clearcoat normalmap both exist in the same material, they must share the single TBN matrix even if they use different images. The tangents may be supplied by vertex attribute, or be calculated from screen-space derivatives of the base normalmap's selected texture coordinates, but there's only one source of tangents for the material. These normalmaps also share geometric normals as well, as there is only one source of those in the glTF vertex attributes. That said, I certainly see the wisdom of keeping the MaterialX implementation of glTF's PBR model separate and free of limitations imposed on the glTF file format, and I believe evidence of that is already present elsewhere in this graph. If it's more useful to have separate TBNs per normalmap here, we should talk through the reasoning and implications. But if the goal is better parity with glTF itself, then perhaps we should make sure the |
The intent of these nodes is to make the MaterialX node an exact match glTF if possible. ( BTW. Note that to we could get a closer match by allowing 4-channel tangent so allow handedness encoding in W instead of the current 3 channel input. ) |
A quick question is what is the best test for mapping (or is a new one required). eg. Would it be the NormalTangentTest ? |
@kwokcb Thanks for the reminder about The For a test model that supplies tangents, see the related model NormalTangentMirrorTest. The "mirrored" sections show the UV map mirrored on one axis, such that the UV winding order flips (clockwise) with respect to the XYZ winding order (counterclockwise), requiring the bitangent handedness to flip over. |
my 2c: I would propose to keep the current state of affairs, as it's cleaner and there are multiple benefits. For instance, it allows the artist to create procedural textures. If the logic were to be handled inside <gltf_pbr>, many inputs would have to be exposed.
The tangent input of <gltf_pbr> is used for anisotropy and does not affect normal mapping. The TBN inputs of <gltf_normalmap> are used to transform the normalmap value from tangent space to world/object space.
That's a good reminder and I wasn't aware of this limitation! However, in practice I would argue that there is only one T/B/N stream per mesh, which is either sampled using a <geompropvalue> or <tangent> node. Therefore, it is very unlikely that someone is going to author multiple tangent bases. Regarding tangent handedness: MaterialX is agnostic and lets the runtime provide "tangent" and "bitangent" geometry streams, which are both of type Therefore, I would argue that the best/idiomatic way to support handedness is to precompute the bitangents. |
OK, I think I agree with this logic. The ability to author procedural maps in MaterialX and have those flow naturally into the glTF PBR material is paramount here. An exporter that bakes these out to images for storage in actual glTF files will need to be aware of the rules and share the tangent basis for complex materials where more than one normalmap/anisotropy map is in use. But it sounds like it's not the right move to have the PBR node graph attempt to enforce that directly. @kwokcb Maybe have the exporter report an error if it's converting a graph that can't conform? If you're fine with this, then I think this PR is good to merge. |
@kwokcb Can we merge this? It's just a quick patch, and the stuff I outlined above would be a larger effort to rework existing parts of the node graph that aren't touched here. |
Hi @emackey
I'm just being finnicky on this, to avoid scenarios (we've already had to leave unresolved) like ORM mapping. Basically, I don't want to add more burden on the interop process to provide flexibility on a node definition which is intended to match glTF as much as possible. There is already a way to achieve the explicit N,T,B routing on a base level |
This PR exposes the 'normal', 'tangent' and 'bitangent' <normalmap> inputs, so that they can be connected to other nodes.