Skip to content

Conversation

pablode
Copy link
Contributor

@pablode pablode commented Jun 21, 2025

This PR exposes the 'normal', 'tangent' and 'bitangent' <normalmap> inputs, so that they can be connected to other nodes.

@jstone-lucasfilm jstone-lucasfilm requested a review from kwokcb June 21, 2025 18:36
@jstone-lucasfilm
Copy link
Member

This looks like a natural change to me, @pablode, and I'm CC'ing @kwokcb and @emackey for their perspectives.

@emackey
Copy link

emackey commented Jun 23, 2025

Interesting. The top-level ND_gltf_pbr_surfaceshader has its own tangent input, was this not hooked up to the orientation of the normalmap's TBN?

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 normal and tangent inputs from the top-level node (which are roughly analogous to similarly-named glTF vertex attributes) are used for both the base normalmap and clearcoat normalmap TBNs (as well as anisotropy, which is already hooked up).

@kwokcb
Copy link
Contributor

kwokcb commented Jun 23, 2025

The intent of these nodes is to make the MaterialX node an exact match glTF if possible.
I'd strongly suggest we do not add any deviations, and discuss whether to and how to add normal and tangent. Exposure does allow for an explicit specification (good / bad) but as you note Ed it seems worth thinking over how to best have 1 consistent basis for all normal maps.

( 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. )

@kwokcb
Copy link
Contributor

kwokcb commented Jun 23, 2025

A quick question is what is the best test for mapping (or is a new one required). eg. Would it be the NormalTangentTest ?

@emackey
Copy link

emackey commented Jun 23, 2025

@kwokcb Thanks for the reminder about tangent.w being the handedness. @jstone-lucasfilm or @pablode Does MaterialX have a concept of tangent "handedness"? Basically glTF does not directly supply the bitangents, and the tangent.w must be either 1.0 or -1.0 to indicate right-handed or left-handed bitangents (I'm typing this from memory, should check the spec to be certain of the details). I don't know if such an encoding exists in MaterialX, and this detail seems like it could easily be considered a geometric concern outside of the PBR model.

The NormalTangentTest is a good one, but it relies on screen-space derivatives and does not supply its own tangents.

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.

@pablode
Copy link
Contributor Author

pablode commented Jun 26, 2025

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 top-level ND_gltf_pbr_surfaceshader has its own tangent input, was this not hooked up to the orientation of the normalmap's TBN?

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.

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.

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 vector3 (alternatively, you can author a raw vector4 geomprop/primvar and decode it).

Therefore, I would argue that the best/idiomatic way to support handedness is to precompute the bitangents.

@emackey
Copy link

emackey commented Jun 26, 2025

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.

@emackey
Copy link

emackey commented Jul 2, 2025

@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.

@kwokcb
Copy link
Contributor

kwokcb commented Jul 3, 2025

Hi @emackey

  • I think in the baked case, I'd like to see at least 1 test example which shows the mapping to glTF and from glTF working (and where it does not).
  • For the non-baked case, I'd like to know the mapping of this to glTF texture procedurals.

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 normalmap node so nothing is disallowed at that level.

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.

4 participants