Support stored vertex 'tangent' attribute

[donmccurdy]

Fixes #15692, fixes #15428.

Changes:

geometry.addAttribute( 'tangent', attribute );
material.vertexTangents = true; // changed to boolean based on feedback below

smooth vertex normals	smooth vertex normals + flat normal map

Here's the effect on some example glTF files we've had normal-related issues with in the past:

case	before	after
flat normal map (normals)
flat normal map
❌ normalTangentTest (without tangents)
normalTangentMirrorTest (with tangents)
shoe

Row 2 (flat normal map) looks a bit less "flat shaded" than I'd like. I'm not sure why, but it's certainly closer to the goal.

Row 3 (normalTangentTest) shows a regression because the model doesn't have tangents. By removing normalScale.y *= -1 from GLTFLoader, I've broken that case. But row 4 (similar model, with tangents) appears correctly, and keeping normalScale.y *= -1 would break it. I've never understood that y *= -1 hack, and so I'm inclined to remove it, but we could also apply it only to models that don't have tangents.

[WestLangley]

@mrdoob This is excellent, and it can be merged, IMO.

[WestLangley]

@donmccurdy @mrdoob

To be addressed in a later PR: I don't like this terminology, though:

vertexTangents = THREE.AutoTangents;
// or
vertexTangents = THREE.VertexTangents;

I have not heard of the term "auto tangents" used in this regard.

I never liked this, either, for that matter:

vertexColors = THREE.VertexColors; // ugh

If it wasn't for Legacy Geometry (and a flag was required) I would prefer

vertexTangents = true; // default false
vertexColors = true; // default false

Or, an alternate property name:

attributeTangents = true; // default false

---

That being said, we could take another approach and use vertex tangents automatically if they are present as a buffer geometry attribute. Likewise for vertex colors.

I do not see a use case for "the attribute is present but do not use it", so there is really no need for a flag, IMO. Users can remove the attribute if they don't want it used.

I think I would prefer this final suggestion. EDIT: never mind

[donmccurdy]

That being said, we could take another approach and use vertex tangents automatically if they are present as a buffer geometry attribute. Likewise for vertex colors.

The downside here is that a single material can be reused where the renderer will have to compile two materials. There is a nice simplicity in the explicit 1:1 mapping.

If we’re planning to drop support for rendering Geometry (#15514) then we may as well remove the Material property for FaceColors, and just have:

material.vertexColors = true;
material.vertexTangents = true;

[donmccurdy]

@stephenhurleyjpl do you have other examples that we should test here, or would you be able to test them? :)

[Mugen87]

Just for clarity: The change only affects MeshStandardMaterial, MeshPhysicalMaterial and MeshNormalMaterial. How about MeshPhongMaterial and MeshLambertMaterial?

[mrdoob]

If it wasn't for Legacy Geometry (and a flag was required) I would prefer

vertexTangents = true; // default false
vertexColors = true; // default false

Agreed. Lets set vertexTangents as boolean here (default false). And change vertexColors later.

[donmccurdy]

Updated so that vertexTangents is a boolean. ✅

Just for clarity: The change only affects MeshStandardMaterial, MeshPhysicalMaterial and MeshNormalMaterial. How about MeshPhongMaterial and MeshLambertMaterial?

That's correct. MeshLambertMaterial isn't supported because it computes lighting per-vertex. MeshPhongMaterial could be supported, but (unless we still have code somewhere to compute tangents on the CPU?) I'm not sure how it much would be used, since GLTFLoader does not create Phong materials. I'm also not sure whether we want more advanced options like this on the Phong model. Any preference?

[WestLangley]

Just for clarity: The change only affects MeshStandardMaterial, MeshPhysicalMaterial and MeshNormalMaterial. How about MeshPhongMaterial?

Is that statement true? Phong is sharing the same shader chunks...

unless we still have code somewhere to compute tangents on the CPU?

BufferGeometryUtils.computeTangents()

We should not be computing tangents for the user, I would think. Are you saying we have no way of loading tangents with any loader other than glTF?

I'm also not sure whether we want more advanced options like this on the Phong model.

I don't think it is necessary.

//

What about three.js tangent support for grayscale bump maps? Or is there such a thing? glTF does not support bump maps, of course.

[mrdoob]

We should not be computing tangents for the user, I would think. Are you saying we have no way of loading tangents with any loader other than glTF?

Maybe the FBXLoader? And we should probably implement in ObjectLoader too.

[mrdoob]

Thanks!

[donmccurdy]

Is that statement true? Phong is sharing the same shader chunks...

There are more places I'd need to patch the Phong shader, but it would be a small addition if we decide we want it.

Are you saying we have no way of loading tangents with any loader other than glTF?

An FBX file can contain tangents, but FBXLoader doesn't use them. Note that FBXLoader always creates Phong or Lambert materials. I don't know how many other formats support tangents, but would be surprised if our other loaders were doing anything with them.

What about three.js tangent support for grayscale bump maps? Or is there such a thing? glTF does not support bump maps, of course.

I don't think there is such a thing... a 'tangent space bump map' wouldn't make sense to me, but I'm open to suggestions. 😅

[looeee]

Note that FBX always creates Phong or Lambert materials

In practice it always creates Phong materials. Lambert should be supported by FBX but I haven't found or been able to create any models with Lambert materials.

At least the Maya and 3DS Max exporters support computing tangents and binormals at export time. However, Maya and Max use different bases for the tangent space and apparently export their own basis directly to FBX so supporting that would add an extra layer of complexity.

[WestLangley]

So previously, three.js did not honor attribute tangents at all. Now it does in some cases.

@donmccurdy and @looeee If you could provide a clear statement as to when and where attribute tangents are now supported, I think that would be beneficial. I think it would also be useful to state where they are not supported.

Obviously, if loaders do not support tangents, that makes their use difficult.

BufferGeometryUtils.computeTangents() is a heuristic, and may have issues. I'd consider removing it -- that is a task for modeling software.

BTW, we used to have VertexTangentsHelper #3511 :-)

[donmccurdy]

glTF advises that MikkTSpace should be used to compute tangents when they are not provided in the model. I don’t think there is any requirement that stored tangents be calculated that way. The problem would be if the model did not have tangents, and we didn’t know how the normal map was created.

If it doesn’t already, one option would be to ensure BufferGeometryUtils.computeTangents uses MikkTSpace — That is the standard these days.

[donmccurdy]

Also - the bitangent is always the cross product of the normal and tangent. Whatever Maya and Max are doing, that should remain true. At most the loader might have to flip the .w component?

[WestLangley]

The problem would be if the model did not have tangents, and we didn’t know how the normal map was created.

Right. And for that reason, I do not think three.js should be in the tangent-creation business. I would prefer to remove computeTangents(), for that reason. Also, a loader should not be computing attributes.

At most the loader might have to flip the .w component?

I don't think a loader should do that -- an exporter, may have to, though.

[looeee]

@looeee If you could provide a clear statement as to when and where attribute tangents are now supported, I think that would be beneficial. I think it would also be useful to state where they are not supported.

OK. Regarding the FBXLoader, tangents are currently ignored, but it should be possible to add support.

However, given the complexity, unless we have a clear use case I'm not sure how useful it is to do so.

the bitangent is always the cross product of the normal and tangent. Whatever Maya and Max are doing, that should remain true. At most the loader might have to flip the .w component?

Yes. I mentioned binormals/bitangents just because it seems that there's no way to export tangents without bitangents being exported too. Converting between bases should be trivial, it's knowing when to do the conversion that may be problematic.

Also, from this article on the Blender wiki it seems like it might not be as simple as that, as tangent space normal maps are tied into the basis being used, so converting the basis will result in the tangent space normal map being read incorrectly. From the article:

A common misunderstanding about tangent space normal maps is that this representation is somehow asset independent. However, normals sampled/captured from a high resolution surface and then transformed into tangent space is more like an encoding. Thus to reverse the original captured field of normals the transformation used to decode would have to be the exact inverse of that which was used to encode.

That might make it simpler though, as we would not have to do any conversion of basis and just load the tangents as is from the file.

@donmccurdy are tangents in glTF models always in MikkTSpace? Or are you just assuming that the tangents loaded match the tangent space normal map that comes with the file?

[ondys]

@donmccurdy are tangents in glTF models always in MikkTSpace? Or are you just assuming that the tangents loaded match the tangent space normal map that comes with the file?

glTF specification states that if the tangents are not defined but needed, then they should be generated using the MikkTSpace algorithm. That is, as long as the gltf primitive contains tangents, the tangents can be generated with an arbitrary algorithm, but if the tangents are not explicitly defined but they are needed (e.g. because of an existing normal-map) than they need to be generated by the MikkTSpace algorithm.

I would argue that the loader should do that.

[Mugen87]

@donmccurdy I have tested today the dev version of GLTFLoader with a model from a user of the forum. When loading the following glb, it is rendered fine but with the following warning:

THREE.WebGLProgram: gl.getProgramInfoLog() WARNING: Output of vertex shader 'vTangent' not read by fragment shader

office.glb.zip

[WestLangley]

I would argue that the loader should do that.

The loader should load the model, not modify it. I disagree with the glTF spec if it implies otherwise.

[WestLangley]

@donmccurdy More test ideas...

Did you test both mesh.geometry.scale( - 1, 1, 1 ) and mesh.scale.set( - 1, 1, 1 ) when tangents are used?

[donmccurdy]

Output of vertex shader 'vTangent' not read by fragment shader...

@Mugen87 that example contains tangents but no normal map. That seems pretty pointless, but I suppose the loader should leave the tangents intact, e.g. in case the user adds normals later. We'll want another ifdef gate around the creation of the varying.

The loader should load the model, not modify it. I disagree with the glTF spec it implies otherwise.

A normal map is meaningless without a tangent space, and it has to be created – that we can do so in the fragment shader is just a nice convenience. The glTF spec must give some recommendation for the sake of consistency, and recommends that if the model is missing tangents, MikkTSpace is the preferred way to generate them. MikkTSpace is supported by UE4, Unity, Substance Painter, Xnormal, Blender, Toolbag, Modo, etc. — it's a robust choice, and since some of those tools do not write tangents, using MikkTSpace (or having a method for users to generate MikkTSpace tangents themselves) is necessary for correct results.

However, for now it's a moot point. I'm not aware that any JS implementation of MikkTSpace exists, and I don't think it can be done in the shader. If someone wanted to try a JS or WASM port of the native code, there would be uses for that. But even then, the amount of added code is significant (~2000 LOC)... it is probably better left to the user to do this.

Also, according to this thread I think Maya's FBX tangents should be supported by this change. 3DS Max maybe not, and I don't know how we tell the difference from a given FBX file. You can see why glTF wanted to recommend MikkTSpace. 😅

Did you test both mesh.geometry.scale( - 1, 1, 1 ) and mesh.scale.set( - 1, 1, 1 ) when tangents are used?

Hm, yeah that's no good. My changes to BufferGeometry.applyMatrix were incorrect – omitting those, geometry.scale looks better. mesh.scale is altering the tangents in a way it shouldn't be.

[WestLangley]

@donmccurdy The shader must use the same tangent space that was used to generate the normal map. If tangents are missing in the model, then the model is incomplete. It is not the job of the loader to fix it. That's my opinion, anyway.

That being said, I know I am fighting a losing battle here. Sooo, I'll drop it... :-)

[Mugen87]

Sidenote: The mentioned warning also pops up in one of the examples:

https://raw.githack.com/mrdoob/three.js/dev/examples/webgl_postprocessing_unreal_bloom.html

[EliasHasle]

Quote @WestLangley :

That being said, we could take another approach and use vertex tangents automatically if they are present as a buffer geometry attribute. Likewise for vertex colors.

I think rendering Points and Lines or Points and Meshes from the same geometry is a nice feature. Or Meshes with vertex colors and Meshes with other shading.

How about material.ignoreAttributes = array, defaulting to [] or null? This would replace material.vertexColors and material.vertexTangents and be more automagic for inexperienced users and more general for advanced users.

(Ideally, geometries should in my opinion have independent properties for each supported type of index. Not something which would be used often, perhaps, but it could for instance be a way to render a line-based wireframe and a shaded mesh from the same geometry.)

[donmccurdy]

@EliasHasle I don't think I understand how ignoreAttributes would be used, but want to open a new issue for this suggestion?

[EliasHasle]

@donmccurdy I just thought the presence of attributes would by default enable the usage of them in the material, but that the ignoreAttributes array could filter them out. It would obviously only work for attributes that can be omitted for the particular material, such as color for MeshPhongMaterial or tangents for some materials. I am thinking the attributes would be listed by string name, for maximal generality. Otherwise, I have too little knowledge about the technical details of attribute handling in three.js to give advice on that. And that is also a reason for me not to open an issue right away, in case the suggestion is just stupid (for technical reasons)...

[donmccurdy]

There's a conflict between automatically enabling features that affect shader compilation, and not automatically creating more shader programs than the user expects. Currently there's a 1:1 relationship here – 1 Material compiles to 1 shader program — and this makes performance optimization easier.

Supporting material.ignoreAttributes=[] would mean that applying the same material to three meshes could generate three different shader programs, if the meshes have different attributes. I don't know how difficult that would be to implement, technically, but the API and its effects might become confusing.