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

BufferGeometryUtils: Add MikkTSpace version of .computeTangents() #23716

Merged
merged 3 commits into from
Mar 21, 2022

Conversation

donmccurdy
Copy link
Collaborator

@donmccurdy donmccurdy commented Mar 13, 2022

Summary: Adds a computeTangents( geometry ) function to BufferGeometryUtils, implementing the MikkTSpace standard for vertex tangents used with tangent space normal maps.

Background: Vertex tangents are used when baking normal maps, but often those original tangents are not included in the model itself. While a different method of computing tangents will usually look "close" to the right result, in some cases (particularly with mirrored UVs) you may see seams or other visual problems. The only way to completely avoid those problems is to use exactly the same vertex tangents as the normal map baker. For that purpose, MikkTSpace is the de facto standard — it is open source, widely used, and is designed to return the same tangents even when certain changes (like reordering vertices) have been made to the underlying geometry.

Currently this PR does not remove the existing geometry.computeTangents() method. The new function is added to BufferGeometryUtils instead, because the size of the MikkTSpace WASM module (from mikktspace-wasm) is about 21 KB gzipped.

Note that calculating vertex tangents is somewhat costly — on my machine it takes 75ms for the DamagedHelmet.glb sample. Any existing geometry index must be removed, although the index can be recomputed after generating tangents. For those reasons, most users probably should not blindly compute vertex tangents in the hope it will make their normal maps look better — where possible, compute tangents offline with tools like gltf-transform tangents or Blender, and omit tangents when there's no benefit.

This PR also borrows a denormalize() function from #22874.

Usage:

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

geometry = computeTangents( geometry );

Related:

@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Mar 14, 2022

One other note – the build script will skip mikktspace.module.js, leaving examples/js/utils/BufferGeometryUtils.js depending on an undefined global generateTangents function when used from the examples/js/* directory. So users not importing ES Modules would need to define that with a modified build of the mikktspace npm package. The mikktspace package only publishes CJS (for Node.js) and ES Module builds currently.

@gkjohnson
Copy link
Collaborator

This would be nice to get merged. I'd like to use it for my path tracing work for meshes with normal maps that don't already have tangents generated. I'm looking at other ways to generate them as-needed in the shader but this would be a good start.

Would like to see it in the next release 😁 🙏

@mrdoob mrdoob added this to the r139 milestone Mar 21, 2022
@mrdoob mrdoob merged commit 74e6f4c into mrdoob:dev Mar 21, 2022
@mrdoob
Copy link
Owner

mrdoob commented Mar 21, 2022

Thanks!

@donmccurdy donmccurdy deleted the feat-mikktspace-tangents branch March 21, 2022 20:13
@gkjohnson
Copy link
Collaborator

Looks like this doesn't work with bundlers that don't support top level await at the moment... which Parcel doesn't, at least (argh!)

parcel-bundler/parcel#4028

@gkjohnson
Copy link
Collaborator

Oh... which actually means that BufferGeometryUtils can't be imported into a project at all without breaking it in one of these bundlers.

@joshuaellis joshuaellis mentioned this pull request Mar 26, 2022
10 tasks
@donmccurdy
Copy link
Collaborator Author

donmccurdy commented Mar 26, 2022

Oh... which actually means that BufferGeometryUtils can't be imported into a project at all without breaking it in one of these bundlers...

Hm, BufferGeometryUtils is all individual exports now — is access to methods other than computeTangents broken in Parcel with v139 as well?

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

Looks like this doesn't work with bundlers that don't support top level await at the moment...

Ugh, I thought top-level await was just part of the ES Modules spec. 🤦‍♂️ I don't suppose dynamic imports or .wasm imports are better supported? Would rather not do the Base64 decode and WASM instantiation synchronously on the main thread... I guess our other option would be to make the MikkTSpace dependency explicit:

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

async function someUserCode() {

  await MikkTSpace.ready;
 
  // ...

  geometry = computeTangents( geometry, MikkTSpace );

}

Also note — there's currently a bug with non-indexed geometry, #23795.

@LeviPesin
Copy link
Contributor

LeviPesin commented Mar 26, 2022

Ugh, I thought top-level await was just part of the ES Modules spec. 🤦‍♂️ I don't suppose dynamic imports or .wasm imports are better supported?

I think that dynamic imports (if you are talking about import()) are supported pretty much everywhere.

@donmccurdy
Copy link
Collaborator Author

In browsers, yes — but like top-level await it will also depend on your bundler to rewrite URLs correctly. I guess in this case it doesn't matter, we probably want computeTangents itself to be synchronous, so we couldn't put in a dynamic import. Unless/until importing .wasm files directly is widely supported, let's do the userland await MikkTSpace.ready; option?

@gkjohnson
Copy link
Collaborator

gkjohnson commented Mar 27, 2022

I'm okay with MikkTSpace.ready.

is access to methods other than computeTangents broken in Parcel with v139 as well?

Yup -- just importing the file causes the issue.

Ugh, I thought top-level await was just part of the ES Modules spec.

Heh, we can dream 😁 Some day new enabling JS features will just work.

abernier pushed a commit to abernier/three.js that referenced this pull request Sep 16, 2022
…doob#23716)

* BufferGeometryUtils: Add MikkTSpace version of .computeTangents()

* Lint

* BufferGeometryUtils: Fix .denormalize() usage.
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.

5 participants