Consider placing object_space_normal_map property on Texture instead of Material

[pailhead]

I think this makes slightly more sense on Texture instead of Material. Needs to be tested, i'd also propose landing something like this with an example.

Related:
#14239

In case of utmost urgency, this functionality can be achieved today by doing:

myMesh.material.onBeforeCompile = shader => {

  shader.u_my_matrix = {value: myMesh.normalMatrix}
 
  shader.fragmentShader = `
  uniform mat3 u_my_matrix;
  ${ 
    shader.fragmentShader.replace('#include <normal_fragment_maps>', `
#ifdef USE_NORMALMAP
    normal = texture2D( normalMap, vUv ).xyz * 2.0 - 1.0; // overrides both flatShading and attribute normals

		#ifdef FLIP_SIDED

			normal = - normal;

		#endif

		#ifdef DOUBLE_SIDED

			normal = normal * ( float( gl_FrontFacing ) * 2.0 - 1.0 );

		#endif

		normal = normalize( normalMatrix * normal );
#endif
    `) 
  }

}

[looeee]

In case of utmost urgency, this functionality can be achieved today by doing

It could also be achieved today if @WestLangley's PR #14239 was merged.

[looeee]

Can you provide some more justification as to why you think this makes more sense on the Texture rather than the Material?

[pailhead]

It could also be achieved today if @WestLangley's PR #14239 was merged.

Apologies, i meant to write:

In case of utmost urgency, this functionality can be achieved at this very moment, without merging, by using shader patching.

---

Can you provide some more justification as to why you think this makes more sense on the Texture rather than the Material?

• base Material class doesn't have to know about an arbitrary property of a sub class. There are 16 materials now, but imagine if there were 116, Material.js would just keep growing. Material also has to know about a property that belongs to only 3 of the 16 materials.
• normal - map - space the construct itself refers to a map, which in three.js is commonly associated with Texture (every ${something}Map is a Texture object). This alone could justify having the "space" attribute on the Texture
• space in particular, refers to a property of the Texture - it's data. Whatever pixels are in the texture could belong to an albedo map and not be related to normal maps, but if the are, they can be in one of the two spaces. This is one level detached from Material.
• reduces duplicated entries in docs, some poor soul maintaining the docs will find it easier to maintain them
• reduces amount of code added to the library
• reduces amount of files modified

[Mugen87]

I'm not voting for this approach since properties like normalScale, bumpScale, displacementScale or displacementBias are also material properties. @WestLangley's PR is way more consistent in that regard. I also think that changing this strategy at the current maturity level of the library is not meaningful.

[pailhead]

normalScale, bumpScale, displacementScale or displacementBias

None of these actually refer to the image data found in the maps though? Those are all actually some values, this here is just a constant.

normalScale makes sense to be applied to an arbitrary normal map that's either in tangent or object space. There really isn't a reason for that property to be on Texture. I believe the same applies to the others but i haven't used them.

Please consider comparing this property to something like wrapS or magFilter or best .format since this is most similar to "encoding".

Ie. the material doesn't need to be told specifically to look at the encoding of some image in a certain way. What should happen if you provide a tangent space normal map but tick the material to display object space normal maps. It seems like a state that no user should ever be able to find themselves in. Such a safety measure shouldn't be limited by the fact that a material can scale some vector that it obtains by looking up some texture. This property happens while that vector is being looked up, its a different stage.

[looeee]

There are 16 materials now but imagine if there were 116, Material.js would just keep growing

This is not going to happen so it's irrelevant. It's in the nature of class extension that sometimes things will be easier if the super class has properties that only some subclasses use. This is to be minimised, of course, but it's often the case that it can't be entirely avoided without making the code more complex.

normal - map - space the construct itself refers to a map

space in particular, refers to a property of the Texture - it's data

These are just nomenclature issues / semantics, not reasons why the property would be better on Texture. As @Mugen87 points out, doing it this way goes against all the other texture properties on materials. That means that it's inconsistent, which is by far the worst way of doing things in a codebase.

Those are all actually some values, this here is just a constant.

A constant is just a value from a restricted set that's been given a special name to make it memorable.

some poor soul maintaining the docs

Over the last year or two, a lot of that has been done by me. Every so often I suggest that other people help, but for some reason they only want to get involved with "important" stuff like nit-picking and blocking other people's PRs 🙄

[pailhead]

"important" stuff like nit-picking and blocking other people's PRs

I didn't mean it like that, i just was referring to the same sentence used twice. Imagine there was a typo, you'd have to fix it in two places.

These are just nomenclature issues / semantics, not reasons why the property would be better on Texture. As @Mugen87 points out, doing it this way goes against all the other texture properties on materials. That means that it's inconsistent, which is by far the worst way of doing things in a codebase.

I disagree with this. Changing this property triggers the whole shader to be recompiled into a different shader. Changing the value of normalScale:Vector2 works with any provided texture without recompiling the shader.

https://threejs.org/docs/#api/textures/Texture

List of possible properties it's in alignment with and not going against, up for debate:

mapping
wrapS
wrapT
magFilter
minFilter
anisotropy
format
type
encoding
//material ever uses only one of these:
offset
repeat
rotation
center

offset,repeat,rotation and center actually don't belong on the Texture since three's Material will only ever use one of them based on a priority list. If that somehow made it's way onto Texture i'd urge you to consider loosening the restrictions around this a little bit.

Just consider this code:

myMaterial.normalScale(2,2) //want to enhance texture
myMaterial.normalMap.repeat.set(2,2) //want to make the texture more dense
myMaterial.alphaMap.repeat(2,2) //i actually have to call this because the previous line did absolutely nothing

If i want to scale normals in UV i have to go through Texture if i want to scale them in T Material, the former also doesn't work if there are other textures present, hence i have to repeat some other map and not the one i actually want to repeat. normalMap has a dedicated slot for the scale param, but it arbitrarily uses the repeats from other maps or self.

var o = new Texture({ space: 'o' })
var t = new Texture()

var myMaterial = new Material()

myMaterial.normalMap = t  

myMaterial.normalMap = o //doesn't work
myMaterial.normalMapType = 'object' //needed to work

myMaterial.normalMap = t // breaks again
myMaterial.normalMapType = 'tangent' // above call ALWAYS has to be accompanied by this call

[mrdoob]

Opted for merging #14239 instead.

[mrdoob]

Thanks though!