BufferGeometry: Allow computeTangents to be called without an Index attribute

[gkjohnson]

Related issue: --

Description

Currently when calling computeTangents it fails if the geometry does not have an index buffer. This PR adjusts the function so it can be called on geometry with or without an index buffer.

Tested by changing the webgl_helpers demo to convert the geometry to non indexed before computing tangents. You can see in the image below that different tangents are generated for the same vertices when they are no longer merged because different triangle edges are being used to derive the tangents and they are not being averaged:

Indexed Geometry	Non Indexed Geometry

cc @WestLangley @donmccurdy

[donmccurdy]

Looks good to me! For what it's worth, the standard way of computing tangents (MikkTSpace) requires that the input geometry be de-indexed: https://github.com/donmccurdy/mikktspace-wasm#other-considerations.

[donmccurdy]

different tangents are generated for the same vertices when they are no longer merged because different triangle edges are being used to derive the tangents and they are not being averaged...

Are you using these tangents with a normal map, or for some other purpose? This kind of sounds like it would introduce visible seams.

[gkjohnson]

Are you using these tangents with a normal map, or for some other purpose? This kind of sounds like it would introduce visible seams.

I would think this would depend on the normal map, no?

TBH my use doesn't rely explicitly on generating tangents for non indexed geometry but it's something I noticed while working with the function that seemed like it could be cleaned up. Right now I'm working on merging geometry from multiple meshes with different materials that may be structured differently which requires consistent vertex attributes. If no normals are present on a geometry I generated them for consistency. Likewise for tangents. Ultimately I'm path tracing against the model so if no normal map is present then the generated tangents are inconsequential.

The computeTangents function also does not work with interleavedBuffedAttributes which I'll plan to address in another PR after this one.

[WestLangley]

The tangents that you use have to be (at least reasonably) consistent with the tangents that were used when creating the normal map.

[WestLangley]

This kind of sounds like it would introduce visible seams

It does.

@gkjohnson Did you test this on a variety of geometries with a normal map and proper lighting?

[gkjohnson]

@gkjohnson Did you test this on a variety of geometries with a normal map and proper lighting?

I haven't but I expect it would cause seams if the normal map was not designed for the generated tangents. And just like merged vertices (with index buffer) are required for generating "smooth" normals, merged vertices will be required for generating "smooth" tangents (or perfectly consistent UV derivatives). I don't think the current result should be considered "incorrect", would it?

[WestLangley]

A non-indexed SphereGeometry with a normal map will render with lighting discontinuties all over the place.

Sorry, I can't support this PR.

[donmccurdy]

I haven't but I expect it would cause seams if the normal map was not designed for the generated tangents

I think it's safe to assume that no normal map is ever designed for the tangents generated by three.js – the only way to really have that consistency is to generate tangents with MikkT.

merged vertices will be required for generating "smooth" tangents (or perfectly consistent UV derivatives)

MikkT does not run on merged vertices, but you can merge vertices after running MikkT (without averaging tangents).

---

My feeling is that tangents generated with computeTangents are not — before or after this change — the right way to ensure a normal map renders as it should... For that you need MikkTSpace. Where computeTangents can be useful is if your custom shader simply requires vertex tangents for some other reason.

Would we want to just pull a MikkTSpace WASM bundle in here? It's 14kb minzipped, but it does the right thing. This would mean that computeTangents needs to de-index input geometry though.

[gkjohnson]

MikkT does not run on merged vertices, but you can merge vertices after running MikkT (without averaging tangents).

Reading about MikkT tangents it sounds like it's specifically designed to address the types of issues that we're seeing here so yeah I understand and agree.

My feeling is that tangents generated with computeTangents are not — before or after this change — the right way to ensure a normal map renders as it should...

This is my understanding after reading into it more. To that end I wonder what the purpose of the current implementation is? Is it to generate tangents that are "good enough" in most cases despite not necessarily being exactly correct?

Anyway if people don't support this change that's fine -- I don't have enough of an invested use case to figure out what works for everyone. I just needed to generate tangents for geometry that didn't have them so I could merge geometries. I suppose the workaround is to merge the vertices before computing tangents which is all I was trying to avoid.

[mrdoob]

@donmccurdy

Would we want to just pull a MikkTSpace WASM bundle in here? It's 14kb minzipped, but it does the right thing. This would mean that computeTangents needs to de-index input geometry though.

Maybe that's something that could sit in the examples folder?

[Mugen87]

When I remember correctly we initially wanted to use tangent generation to fix issues like #20997. However, the change was reverted (#21186) and it seems at the end of the discussion it was suggested to avoid per-vertex processing and generating tangents offline. At least in context of glTF.

If we now want to add the MikkTSpace WASM bundle in the repository, I have the following questions:

• Would it be an option to embed a JS version of MikkTSpace in the core and use it for computeTangents()? We also have an Earcut implementation in the core (although such an algorithm is more compact).
• If we put the WASM bundle in the examples, would it make sense to use it in GLTFLoader? Or is the problem mentioned in GLTFLoader: Revert usage of computeTangents(). #21186 (comment) still a thing?

[donmccurdy]

It'd be a good addition to BufferGeometryUtils, particularly because users can tree-shake utils:

import { computeTangents } from 'examples/jsm/utils/BufferGeometryUtils.js';

I'm not keen to attempt a plain-JS port of MikkTSpace, so I expect that import would include the 14kb WASM binary, either inlined in base64 (+33% size) or leaning on ES Modules and bundler support.

I still wouldn't compute tangents by default in GLTFLoader (or other loaders), for the same reasons as before — it imposes a performance cost on everyone, in order to solve a problem most models (even with normal maps) don't have. Better to make users aware that if their normal map doesn't look the way they expect, computing tangents (online or offline) might be the solution.