MeshStandardMaterial: Remove .vertexTangents property

[donmccurdy]

This change is, I hope, in line with @WestLangley's comments in #11438 (comment), "we may be able to fix this automatically by honoring the flipY flag in the shader". Effects:

• MeshStandardMaterial no longer requires vertexTangents; they're enabled automatically similarly to vertex alpha.
• GLTFLoader no longer needs normalScale.set(1, -1) (previously required because all textures use flipY=false)
• Users whose normal maps set flipY=false, but follow some other UV coordinate convention, may need to begin setting normalScale.set(1, -1). I'm not aware of any such use.

[donmccurdy]

Aside, I'm also interested in removing other material boolean flags that could be inferred from context:

• vertexColors
• flatShading
• morphTargets
• morphNormals

Are there any of these we'd prefer to keep as explicit, opt-in properties? /cc @elalish, related to mapping three.js materials to/from source glTF materials.

[donmccurdy]

Some useful test cases below. Results appear to be identical before/after in each.

• https://threejs.org/examples/?q=gltf#webgl_loader_gltf (normal map)
• https://threejs.org/examples/#webgl_animation_keyframes (normal map + vertex tangents)
• https://threejs.org/examples/?q=gltf#webgl_loader_gltf_extensions (normal map + clearcoat normal map)

before	after

[mrdoob]

Can we infer flatShading...?
Maybe when the geometry has no normals attribute but the material is lit?

[elalish]

Can we infer flatShading...?
Maybe when the geometry has no normals attribute but the material is lit?

Precisely, in fact this is how the glTF standard defines this as well. Removing duplicate info is a great way to simplify code and avoid bugs, so I'm all in favor. Thanks @donmccurdy!

[gkjohnson]

Aside, I'm also interested in removing other material boolean flags that could be inferred from context:

• vertexColors
• flatShading

I feel pretty strongly that these two properties shouldn't be removed from materials because they're used to stylistically control the rendering of the geometry and the ability to toggle them without messing with the geometry is useful (and more intuitive). For my work, for example, vertex colors are one option for rendering temperature visualizations and it's something you'd want to be able to turn off (and use the diffuse color only) when the user doesn't want to see it. I also toggle flat shading on geometry even if they have normals because flat shading helps provide more information on the structure of the geometry.

Having said that I'm not against changing the flags so they use a more reasonable default inferred from context. For example flatShading can default to null and if no normals are available then true can be assumed so rendering can "just work" in these cases.

[Mugen87]

I think we should not discuss the removal of other material flags in this PR but focus on its implementation. Otherwise this will be a hard to follow topic.

[WestLangley]

GLTFLoader no longer needs normalScale.set( 1, - 1 )

But it sets it. Can you please further explain what you are referring to?

flipNormalScaleY: ( ( material.normalMap && material.normalMap.flipY === false || material.clearcoatNormalMap && material.clearcoatNormalMap.flipY === false ) && ! ( object.geometry && object.geometry.attributes.tangent ) )

Can you please explain this line? What are the assumptions upon which this is based?

Maybe @elalish can help?

[donmccurdy]

@WestLangley this PR removes the negation of .normalScale.y from GLTFLoader...

// Before
materialParams.normalScale = new Vector2( materialDef.normalTexture.scale, - materialDef.normalTexture.scale );

// After
materialParams.normalScale = new Vector2( materialDef.normalTexture.scale, materialDef.normalTexture.scale );

...unless I've missed another example?

Explaining the line you mentioned:

// Invert normalScale.y if ...
flipNormalScaleY: (
  // ... material has flipY=false on normalMap OR clearcoatNormalMap ...
  ( 
    material.normalMap && material.normalMap.flipY === false 
    || material.clearcoatNormalMap && material.clearcoatNormalMap.flipY === false
  ) 
  // ... AND the geometry does not include vertex tangents.
  && ! ( object.geometry && object.geometry.attributes.tangent )
)

One edge case here would be setting both normalMap and clearcoatNormalMap, where only one of the two uses flipY=false. Because they're required to use the same UVs, this case seems implausible.

---

I think we should not discuss the removal of other material flags in this PR but focus on its implementation. Otherwise this will be a hard to follow topic.

Sounds good, I'll open a new issue.

[WestLangley]

nit. optional. your choice.

new Vector2().setScalar( materialDef.normalTexture.scale );

[WestLangley]

@donmccurdy Thanks for this PR, but TBH, I don't understand the rationale behind the proposed implementation, so I'll bow out of this thread.

I do not want to be a blocker -- especially if it seems OK to @mrdoob and others. :-)

Once I see how this PR is resolved, I will open a new issue with my thoughts.

[donmccurdy]

@WestLangley did you mean something different by "honoring the flipY flag in the shader", in the earlier comment? Or this implementation just doesn't fully make sense?

[WestLangley]

did you mean something different by "honoring the flipY flag in the shader"

TBH, I am not sure what I was thinking at the time...

//

mapN.xy *= normalScale;

#ifdef FLIP_NORMAL_SCALE_Y
	mapN.y *= -1.0;
#endif

To me, having normalScale.y and FLIP_NORMAL_SCALE_Y seems odd.

//

// Invert normalScale.y if ...
flipNormalScaleY: (
  // ... material has flipY=false on normalMap OR clearcoatNormalMap ...
  ( 
    material.normalMap && material.normalMap.flipY === false 
    || material.clearcoatNormalMap && material.clearcoatNormalMap.flipY === false
  ) 
  // ... AND the geometry does not include vertex tangents.
  && ! ( object.geometry && object.geometry.attributes.tangent )
)

I also do not understand why, if material.normalMap.flipY === false, you apply a flip in normal.y contrary to what the user set.

[donmccurdy]

I also do not understand why, if material.normalMap.flipY === false, you apply a flip in normal.y contrary to what the user set.

The user has no explicit way to tell us the tangent space convention used by the normal map, but I think it is reasonable to infer that from the flipY flag. Perhaps there's a more general name we could use for FLIP_NORMAL_SCALE_Y instead, such as USE_GL_TANGENT_SPACE? Basing that on comments by @emackey about DirectX vs OpenGL tangent space convention in #11438 (comment).

[WestLangley]

@donmccurdy I think it is more complex than that due to the flexibility of three.js. Perhaps the demo provided in #22165 will be helpful.

[donmccurdy]

Is the purpose of normalScale being a vec2 basically to support directx- and opengl-style normal map conventions? The oldest thread I could find was #7459. I'm wondering if we could convert normalScale to a scalar, with this PR identifying the normal map convention via normalMap.flipY instead. Too confusing maybe... I guess the core problem is that a gl-style normal map could very reasonably use flipY=true, as well?

Some alternatives, then...

• (A) Reduce .normalScale to a scalar, expanding .normalMapType to three options:
  • DirectXTangentSpaceNormalMap (default)
  • OpenGLTangentSpaceNormalMap
  • ObjectSpaceNormalMap
• (B) Remove .vertexTangents, but don't infer tangent space convention in the renderer, and continue setting .normalScale.y = -1 in GLTFLoader as before.

I suppose (B) is the simpler of the two, I'm happy to switch to that if you agree.

[donmccurdy]

I'm overthinking this. 😅 Thanks for steering this in the right direction @WestLangley. It sounds like we just need this flag to be:

vertexTangents: ( material.normalMap && object.geometry && object.geometry.attributes.tangent ),

... and avoid the whole flipNormalScaleY parameter?

---

@elalish it does not sound likely that 1:1 glTF↔three.js material mappings are going to be possible, for several reasons. I think I would advise planning around robust 1:many mappings instead, improving GLTFLoader for that purpose if needed.

[elalish]

@elalish it does not sound likely that 1:1 glTF↔three.js material mappings are going to be possible, for several reasons. I think I would advise planning around robust 1:many mappings instead, improving GLTFLoader for that purpose if needed.

Yeah, I pretty much expected that. We handle it now and hopefully it won't be too bug-prone.

[donmccurdy]

Started a separate issue for discussion of morphNormals and morphTargets: #22166

[mrdoob]

Thanks!

[donmccurdy]

Following up in #22182 to address feedback, I'd meant to revert a part of this change.