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

MikkTSpaceTangents: Generating tangents for "NormalTangentTest" glTF sample model breaks normals #25263

Closed
gkjohnson opened this issue Jan 9, 2023 · 5 comments
Labels

Comments

@gkjohnson
Copy link
Collaborator

Description

When loading the glTF sample NormalTangentTest model and manually generating tangents using the BufferGeometryUtils.computeMikkTSpaceTangents function the computed normals change and the model renders incorrectly.

However according to the spec - MikkTSpace tangents should produce the correct result:

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.

It looks like the computeMikkTSpaceTangents function inverts the tangent direction by default here specifically noting alignment for glTF files. However disabling this behavior by passing "false" into the function fixes the issue. Is the tangent inversion correct?

// Texture coordinate convention of glTF differs from the apparent
// default of the MikkTSpace library; .w component must be flipped.
if ( negateSign ) {
for ( let i = 3; i < tangents.length; i += 4 ) {
tangents[ i ] *= - 1;
}
}

Related to gkjohnson/three-gpu-pathtracer#210 (comment)
cc @donmccurdy

Reproduction steps

  1. Load the sample model
  2. Compute tangents using BufferGeometryUtils.computeMikkTSpaceTangents
  3. Model renders incorrectly

Code

See jsfiddle

Live example

https://jsfiddle.net/kpj76ob0/2/

Screenshots

computeMikkTSpaceTangents( geometry, MikkTSpace )

image

computeMikkTSpaceTangents( geometry, MikkTSpace, false ) or no tangent generation

image

Version

r147

Device

Desktop

Browser

Chrome

OS

Windows, MacOS

@gkjohnson gkjohnson added this to the r149 milestone Jan 9, 2023
@gkjohnson gkjohnson added the Bug label Jan 9, 2023
@donmccurdy
Copy link
Collaborator

donmccurdy commented Jan 9, 2023

@gkjohnson it looks like the correct way to update the mesh for MikkTSpace tangents would be:

const geometries = new Set();
const materials = new Set();

gltf.scene.traverse( ( object ) => {

	if ( object.isMesh ) {

		geometries.add( object.geometry );
		materials.add( object.material );

	}


} );

for ( const geometry of geometries ) {
  
	BufferGeometryUtils.computeMikkTSpaceTangents( geometry, MikkTSpace ); 

}

for ( const material of materials ) {

	if ( material.normalScale ) material.normalScale.y *= - 1;
	if ( material.clearcoatNormalScale ) material.clearcoatNormalScale.y *= - 1;

}

The tangent inversion is correct, but does require changes to the material properties. That would also explain why my generating tangents with glTF Transform did not cause the same issue: GLTFLoader sees the tangents in that case, and applies the necessary material changes automatically.

Related:

@donmccurdy
Copy link
Collaborator

Perhaps it would also be fine to disable tangent.w negation, and leave the material properties as-is. But that would break a round-trip export, if (for example) you want to bake maps in three-gpu-pathtracer.

@gkjohnson
Copy link
Collaborator Author

gkjohnson commented Jan 9, 2023

Okay I see - interesting. Thanks!

for ( const material of materials ) {

  if ( material.normalScale ) material.normalScale.y *= - 1;
  if ( material.clearcoatNormalScale ) material.clearcoatNormalScale.y *= - 1;

}

So if I clear the tangents and then generate them again how can I make sure they line up with the appropriate material properties appropriately? If I clear the tangents would I need to multiple the normal scale - 1 again?

#18112 may answer this question but just to be sure.

@donmccurdy
Copy link
Collaborator

donmccurdy commented Jan 9, 2023

If we're talking about arbitrary three.js scenes, it might be hard to know whether a material has been configured with vertex tangents in mind, or what UV convention has been used (which would have related effects).

If we're talking about glTF source models, then it's helpful to know any normalScale specified by the model will be uniform: glTF does not allow different x/y parameters to normalScale. As a result I think you can safely assume:

  • Math.sign(x) === Math.sign(y) material configured for derivative-based tangents
  • Math.sign(x) !== Math.sign(y) material configured for vertex tangents

@gkjohnson
Copy link
Collaborator Author

Great, thanks!

@gkjohnson gkjohnson added Question and removed Bug labels Jan 9, 2023
@gkjohnson gkjohnson removed this from the r149 milestone Jan 9, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants