BufferGeometryUtils: Fixes for MikkTSpace tangents

[donmccurdy]

Related issues:

• Fixes BufferGeometryUtils.computeTangents( geometry ) fails with non-indexed geometry #23795
• Fixes Build error with 0.139.0 #23807
• Related BufferGeometryUtils: Add MikkTSpace version of .computeTangents() #23716

Description

• Avoid top-level await.
• Fix error on non-indexed geometry.

/cc @gkjohnson do you mind checking that this works with your Parcel setup?

[marcofugaro]

I think this import can stay now that there is no top-level await anymore? The wasm variable will contain the instance once the request has been fulfilled.

[donmccurdy]

It technically could stay, it's a question of the preferred API / usage:

import * as MikkTSpace from 'three/examples/jsm/libs/mikktspace.module.js';
import { computeTangents } from 'three/examples/jsm/utils/BufferGeometryUtils.js';

await MikkTSpace.ready;

// (A)
geometry = computeTangents( geometry, MikkTSpace );

// (B)
geometry = computeTangents( geometry );

I was thinking (A) might be better because it is more explicit – you have to know about this MikkTSpace module, because you're passing it into the function, so there's perhaps less chance of forgetting to check .ready and having a subtle race condition. This also allows users to provide the mikktspace npm package instead, which is more optimized for modern bundlers (it doesn't embed the WASM as a base64 string as required here).

I'm open to either (A) or (B), though.

[marcofugaro]

Right, if it makes users able to use the wasm version, it makes sense.

[gkjohnson]

Works great! I recommend having a code blurb in the documentation showing how to import the MikkTSpace object since it wasn't immediately clear to me:

import * as MikkTSpace from './lib/mikktspace.module.js';
import { computeTangents } from './lib/BufferGeometryUtils.js';

// ...

await MikktSpace.ready;
computeTangents( geometry, MikktSpace );

Here's a demo of a model that had to have tangents generated to use the normal map correctly:

[gkjohnson]

Also not sure if this is a requirement of MikkTSpace computation or not but it would be nice if vertices were not implicitly unmerged while computing tangents. But I guess it's not a huge deal since you can remerge after.

[donmccurdy]

MikkTSpace does require non-indexed input, although re-indexing after computing tangents is fine. Would it be better to throw an error for indexed input rather than rebuilding the input geometry?

[WestLangley]

I find it confusing to have two methods called .computeTangents(). Maybe .computeMikkTSpaceTangents()?

[gkjohnson]

MikkTSpace does require non-indexed input, although re-indexing after computing tangents is fine. Would it be better to throw an error for indexed input rather than rebuilding the input geometry?

No I think as-is is fine. Just thought I'd mention it in case there was a better way to handle it. I think requiring the user to merge after makes sense. I'd just make sure it's well-noted in the docs.

[WestLangley]

Here's a demo of a model that had to have tangents generated to use the normal map correctly

@gkjohnson Since the docs are not clear as to when the existing method may not work, can you please use VertexTangentsHelper and show us where the differences lie? Is it the mirrored UVs... or what?

[gkjohnson]

can you please use VertexTangentsHelper and show us where the differences lie? Is it the mirrored UVs... or what?

The model above just does not have tangents at all. For my use case (path traced ao map baking) I need tangents in order to correctly use the normal map and the existing view-space generation doesn't work. I want to generate tangents in-shader for this use case, as well, (which I think is doable) but that's a longer term effort.

[WestLangley]

@gkjohnson But you compute tangents, and the existing method is inadequate in your example. Right?

I'd like to know where the existing method fails.

[gkjohnson]

Oh tbh I didn't realize that BufferGeometry.computeTangents had been implemented. I do see that GLTF recommends MikkTSpace normals if no tangents are provided in a model, though.

When tangents are not specified, client implementations SHOULD calculate tangents using default MikkTSpace algorithms with the specified vertex positions, normals, and texture coordinates associated with the normal texture.

But here's an image with the different compute tangent methods. Notice on the BufferGeometry.computeTangents that the normal map seems inverted -- this is likely due to the negateSign argument that is included in the MikkT function in this PR. Perhaps a negate argument could be added to the BufferGeometry version? Perhaps the built in version would be good enough in most cases, then. I'll have to look at the tangents helper another time, though.

MikkT	computeTangents

[gkjohnson]

It might also be worth throwing a more human-readable error if there are no normals / uvs present on the geometry since they're required for generating tangents.

[marcofugaro]

@mrdoob we might want to publish a patch with this.. Some bundlers are affected even if not importing directly from BufferGeometryUtils.

[mrdoob]

Thanks!

[mrdoob]

@marcofugaro Done. Cherry-picked this PR into master and published 0.139.1.

[donmccurdy]

I find it confusing to have two methods called .computeTangents(). Maybe .computeMikkTSpaceTangents()?

It might also be worth throwing a more human-readable error if there are no normals / uvs present on the geometry since they're required for generating tangents.

Made both changes in #23815, for r140.

[Methuselah96]

@mrdoob I'm not sure that this PR was successfully included in 0.139.1. When I look at the code in the published package it appears to match the code before this PR and my build is still failing due to the top-level await issue. Maybe someone else can verify as well?

[gkjohnson]

I'm not sure that this PR was successfully included in 0.139.1.

Looks like the old version, still: https://unpkg.com/browse/[email protected]/examples/jsm/utils/BufferGeometryUtils.js

[mrdoob]

Eek... I cherry-picked the wrong PR...

[mrdoob]

Alright, 0.139.2 is out 😅