Removal of Tangents code?

[mrdoob]

Now that bump/normalmaps are using derivatives I wonder if we need the tangents code (Geometry.computeTangents and BufferGeometry.computeTangents) any longer.

These are the only examples that rely on it:

webgl_materials_normaldisplacementmap
Uses NormalDisplacementShader.js, which I don't know if we should still maintain

webgl_materials_bumpmap_skin
I think it's not needed on this one, but it's currently broken

webgl_terrain_dynamic
easy to port to derivatives I think

Does anyone rely on this code? And if so, what's the use case?

[bhouston]

Clara.io uses the tangents, although I had to modify them a bit. I use then to do anisotropic (non-circular) highlights:

https://clara.io/view/4faca36d-d9aa-4b1b-b9a2-dfa8d6bc978b

Details: https://en.wikipedia.org/wiki/Specular_highlight#Ward_anisotropic_distribution

I did try to implement anisotropic reflection without tangents but I could never get great results. Now I am not 100% sure I can not get the same results with just UV derivatives, but I couldn't manage it and I tried a fair bit.

The main issue I believe I ran into is that the UV derivatives are constant within a face and change abrutly when you switch to a difference face -- because they are derived from the UV interpolation across each face. Tangents are interpolated from the vertices, thus they vary smoothly across faces, thus you can get continuous results across faces.

You do not notice the abrupt change of UV derivates with bump and normal map because you are not looking at the tangent/binormal directly as you are with an anisotropic highlight, rather just the normal.

/ping @WestLangley: Your math expertise may be useful here.

[mrdoob]

I assume you're using BufferGeometry and custom shaders for that, right? I guess you could then move the tangents generation code to the app level instead?

I'm just trying to simplify/reduce the code we have to maintain.

[bhouston]

I am fine removing it from core.

Actually we did end up are generating tangents explicitly within Clara.io and storing them in our mesh data files, very similar to how explicit UV maps are handled. So we are already doing a fully out of ThreeJS core solution for tangents, although it is based on the tangent code in ThreeJS -- so thank you!

[mrdoob]

Ended up moving BufferGeometry.computeTangents() to BufferGeometryUtils.computeTangents() in examples/js.

[mrdoob]

Aaaand... we're back at 99kb gzipped 😁

[WestLangley]

Yikes!

I agree that tangent computation should be at the application/modeling level. Moving computeTangents() to the examples was the right thing to do.

It was also OK to remove from the library and examples the shaders that rely on attribute tangents.

But you also removed all tangent support from the library. I would not do that.

I would continue to support users who have attribute tangents in their geometry.

[mrdoob]

Oh, I left one. webgl_terrain_dynamic still uses tangents (uses THREE.BufferGeometryUtils.computeTangents()). I'm not sure what to do with webgl_materials_skin though...

[mrdoob]

In the IRC someone is saying that they were relying on NormalDisplacementShader.js. I'll try to bring it back use in a BufferGeometry based example.

[WestLangley]

As I said

But you also removed all tangent support from the library. I would not do that.

I would continue to support users who have attribute tangents in their geometry.

I would suggest we add vertex texture and displacement map support to MeshPhongMaterial instead of restoring the NormalDisplacementShader.

Do any vertex texture examples remain?

[mrdoob]

I would suggest we add vertex texture and displacement map support to MeshPhongMaterial instead of restoring the NormalDisplacementShader.

That sounds good to me!

Do any vertex texture examples remain?

Hmm... webgl_kinect?

[mrdoob]

As per things to keep... Maybe we can keep VertexTangentsHelper, but only for BufferGeometry.

[WestLangley]

It would seem to me that any loader that supports tangents should continue to do so.

Also, the pipeline from Geometry -> DirectGeometry -> BufferGeometry should support tangents.

IMO, those two are important.

You can move the VertexTangentsHelper to the examples if you want.

[bhouston]

I'd prefer to still be able to specify tangents and have them communicated to shaders. But everything but basic support can be moved to examples.

Radical idea - if we want to be space efficient why don't we demo Geometry.js to examples/js? It is both slow, and memory inefficient. Using it is never best practice. It is useful when creating custom geometry for sure but in most playback situations and gaming situation that isn't needed. I think there are other types of inefficient "creation" oriented stuff that can also be demoted. A core ThreeJS could be completely oriented around playback of loaded geometries/textures/materials.

[mrdoob]

@bhouston that's the long term goal 😀 I suspect that someone that knows what tangents are is unlikely to be using THREE.Geometry...

[mrdoob]

Ok, implemented displacementMap. I think this is the first time I hack the shader chunk stuff. What a mess... 😅

I'm not updating the normals though, I think that would require sampling the neighbour pixels in the texture to find the slope. And for that I would also need the dimensions of the texture...

[WestLangley]

Ok, implemented displacementMap.

So did I in #7107. Hmmm...

[donmccurdy]

I'd prefer to still be able to specify tangents and have them communicated to shaders.

Is this still something we'd want in core three.js? For example:

geometry.setAttribute( 'tangent', tangentAttribute );

material.vertexTangents = true;

var mesh = new THREE.Mesh( geometry, material );

[WestLangley]

@donmccurdy To the best of my knowledge, there has not been any demand for tangent support in three.js in years. three.js does not require them, and internal support for tangents has been removed. So, in spite of what I wrote in 2015, I would not worry about it at this point.

[mrdoob]

With the current code, is there a way to invert normalScale when rendering the flipped normalMap?

Maybe something like...

mapN.xy = mapN.xy * normalScale * normalize( vUV.st );

https://threejs.org/examples/webgl_loader_gltf.html

Notice how the right side of the helmet has the normalMap inverted due to the texture being mirrored.

[WestLangley]

@mrdoob This is definitely a normal map issue. I would think that mirroredRepeatWrapping would be appropriate, but it is not used in this model -- nor do I expect it would work correctly for normal maps.

Also see this comment -- and in fact, the entire thread.

[mrdoob]

Maybe something like...

mapN.xy = mapN.xy * normalScale * normalize( vUV.st );

Actually, that code is stupid. Sorry about that 😅

[donmccurdy]

Just confirmed that re-introducing tangent support fixes the DamagedHelmet model:

without tangents	with tangents

Longer discussion in KhronosGroup/glTF-Sample-Models#149.

(also requires updating to a version of the model that uses tangents)

[bhouston]

@donmccurdy This is a pretty minor issue and does not require tangents to be calculated, you are going about it in the wrong way. I'll post it on the glTF issue as well.

This issue can be quickly fixed in the calculation of the normal map tangent space because it is possible to detected mirrored faces by comparing the specified normal with the tangent space normal.

Just update normalmap_pars_fragments.glsl with this code and it probably will be fixed:

vec3 perturbNormal2Arb( vec3 eye_pos, vec3 surf_norm ) {

		#if defined( TEXTURE_SLOTS )
			vec2 normalUv = normalMapUV();
		#else
			vec2 normalUv = vUv;
		#endif

		vec3 q0 = dFdx( eye_pos.xyz );
		vec3 q1 = dFdy( eye_pos.xyz );
		vec2 st0 = dFdx( normalUv.st );
		vec2 st1 = dFdy( normalUv.st );

		vec3 S = normalize( q0 * st1.t - q1 * st0.t );
		vec3 T = normalize( -q0 * st1.s + q1 * st0.s );		
		vec3 N = normalize( surf_norm );

		// invert S, T when the UV direction is backwards (from mirrored faces),
		// otherwise it will do the normal mapping backwards.
		vec3 NfromST = cross( S, T );
		if( dot( NfromST, N ) < 0.0 ) {
			S *= -1.0;
			T *= -1.0;
		}

		vec3 mapN = texture2D( normalMap, normalUv ).xyz * 2.0 - 1.0;
		mapN.xy = normalScale * mapN.xy;
		mat3 tsn = mat3( S, T, N );
		return normalize( tsn * mapN );

	}

[donmccurdy]

Thank you @bhouston! Yes, that fixes the issue and is a much easier solution than I attempted. 😅

[WestLangley]

@donmccurdy The DamagedHelmet model is mirrored. This means that the winding order of the uvs are clockwise on one half and counterclockwise on the other half.

This means that the problem can not be fixed be setting normalScale.x = - 1.

I'll file a PR to fix this.

@bhouston Our exiting code is based on this paper, which states.

Well, if we are going to normalize anyway, we don't really need a constant scaling term at all. Nuke it!

That statement is not true. If you scale a vector and then normalize it, the direction of the vector changes if the scale is negative.

Consequently, our code is buggy.

There are better solutions to this problem. I'll pick one and file a PR.

[bhouston]

That statement is not true. If you scale a vector and then normalize it, the direction of the vector changes if the scale is negative.

Huh? Normalizing a vector does not change its direction. ?

[bhouston]

@WestLangley please try my solution it is actually very robust and correct.

[bhouston]

@WestLangley I do not have a reference for my solution, I just derived it but I am pretty confident in it.

[WestLangley]

@bhouston These vectors are different.

new THREE.Vector3( 10, 0, 0 ).multiplyScalar( - 1 ).normalize();
new THREE.Vector3( 10, 0, 0 ).normalize();

You can't drop the scale factor.

A modification to our existing code that I believe works is simply honoring (at least the sign of) the scale factor:

vec3 perturbNormal2Arb( vec3 eye_pos, vec3 surf_norm ) {

	// Workaround for Adreno 3XX dFd*( vec3 ) bug. See #9988

	vec3 q0 = vec3( dFdx( eye_pos.x ), dFdx( eye_pos.y ), dFdx( eye_pos.z ) );
	vec3 q1 = vec3( dFdy( eye_pos.x ), dFdy( eye_pos.y ), dFdy( eye_pos.z ) );
	vec2 st0 = dFdx( vUv.st );
	vec2 st1 = dFdy( vUv.st );

	float denom = sign( st1.t * st0.s - st0.t * st1.s ); // we do not care about the magnitude
	vec3 S = normalize( ( q0 * st1.t - q1 * st0.t ) / denom );
	vec3 T = normalize( ( -q0 * st1.s + q1 * st0.s ) / denom );
	vec3 N = normalize( surf_norm );

	vec3 mapN = texture2D( normalMap, vUv ).xyz * 2.0 - 1.0;
	mapN.xy = normalScale * mapN.xy;
	mat3 tsn = mat3( S, T, N );
	return normalize( tsn * mapN );

}

There is another approach described here.

[bhouston]

@WestLangley I do not understand your argument. I am pretty sure in the end if your solution works correct it is mathematically equivalent to what I am proposing. I am fine if you want to use your solution instead of mine, I'm not picky, let's just fix it.

[bhouston]

@WestLangley I am confused that you are not comparing the st1/st0 result in denom to the surf_norm/N. That was sort of key to my approach. I think your approach is not equivalent.

[WestLangley]

@bhouston I do not care whose solution we use. I prefer to pick the solution that is most performant. You probably have a better feel for that than I do.

I am simply implementing the code in the paper, but I am not ignoring the scale term because it is incorrect to ignore it.

[bhouston]

@WestLangley, in the normalmap code, which scale factor am I ignoring in my proposed changes:

https://github.com/mrdoob/three.js/blob/dev/src/renderers/shaders/ShaderChunk/normalmap_pars_fragment.glsl

All I am proposing is to add this code that flips the U and V direction vectors when I detect its implied normal doesn't match up with the explicitly given normal. This situation happens in the case of mirrored faces, the UVs are backwards but someone has got the normal pointing forwards:

// invert S, T when the UV direction is backwards (from mirrored faces),
// otherwise it will do the normal mapping backwards.
vec3 NfromST = cross( S, T );
if( dot( NfromST, N ) < 0.0 ) {
   S *= -1.0;
   T *= -1.0;
}

It is a pretty simple fix. This fix is a cross + dot + if test. It is pretty minor.

[WestLangley]

@bhouston YOU are not ignoring a scale term. The reference from which our code is based is ignoring a scale term.

All I did was reintroduce the scale term. It is very simple.

I have not passed judgement on your proposal at all. I just presented the solution I discovered last week.

[bhouston]

All I did was reintroduce the scale term. It is very simple.

Does it work equivalently? Does it fix the mirror case? I could imagine that the signs may represent whether it is facing the camera. Thus it may work.

If you want it performance, just multiply the sign rather than dividing it. I think that divide is slow.

[donmccurdy]

Both proposals do fix the mirror case above.

[bhouston]

Let's go with @WestLangley solution but with muls instead of divides, it is mathematically equivalent.

[WestLangley]

There is another approach that uses cotangent frames that we could try if you want. I linked to that above.

There is a fourth approach used by khronos here.

I was in the process of trying to determine the advantages/disadvantages of the various approaches last week, but we can just pick one.

I was mostly just curious... :)

[WestLangley]

OK. Going with one solution now, rather than pursuing this further.

For the record, our approach does not guarantee an orthonormal TBN matrix. Other approaches, do. Apparently it doesn't matter that much...

Thank you @bhouston for your solution, and @donmccurdy for your input.

[bhouston]

For the record, our approach does not guarantee an orthonormal TBN matrix.

Guaranteeing an orthonormal matrix can introduce issues because it is basically a per pixel orthonormal matrix correction and that correction may not always be applied in a way that leads to continuity of the orthonormal matrix across adjacent pixels.

[WestLangley]

@bhouston Interesting... thanks!