Consider placing object_space_normal_map property on Texture instead of Material #14291

pailhead · 2018-06-15T07:58:16Z

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
    `) 
  }

}

Nuke MeshPhongMaterial documentation Nuke MeshStandardMaterial documentation Add Texture documentation Nuke Material references to new props Nuke new props from MeshNormalMaterial Nuke new props from MeshPhongMaterial Nuke new props from MeshStandardMaterial

looeee · 2018-06-15T08:45:27Z

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 · 2018-06-15T08:48:55Z

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

pailhead · 2018-06-15T09:09:58Z

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 · 2018-06-15T09:22:18Z

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 · 2018-06-15T09:35:00Z

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 · 2018-06-15T09:40:12Z

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 · 2018-06-15T09:50:50Z

"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 · 2018-06-17T01:20:44Z

Opted for merging #14239 instead.

mrdoob · 2018-06-17T01:20:51Z

Thanks though!

WestLangley and others added 2 commits June 8, 2018 00:34

Support object-space normal maps

e5f809a

pailhead mentioned this pull request Jun 15, 2018

Support object-space normal maps #14239

Merged

pailhead changed the title ~~Consider putting object_space_normal_map property on Texture instead of Material~~ Consider placing object_space_normal_map property on Texture instead of Material Jun 15, 2018

pailhead added 2 commits June 15, 2018 01:32

triage code order to remove one extra line

7c5ab7a

removed too much

0a3587e

mrdoob closed this Jun 17, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consider placing object_space_normal_map property on Texture instead of Material #14291

Consider placing object_space_normal_map property on Texture instead of Material #14291

pailhead commented Jun 15, 2018 •

edited

Loading

looeee commented Jun 15, 2018

looeee commented Jun 15, 2018

pailhead commented Jun 15, 2018 •

edited

Loading

Mugen87 commented Jun 15, 2018

pailhead commented Jun 15, 2018 •

edited

Loading

looeee commented Jun 15, 2018 •

edited

Loading

pailhead commented Jun 15, 2018 •

edited

Loading

mrdoob commented Jun 17, 2018

mrdoob commented Jun 17, 2018

Consider placing object_space_normal_map property on Texture instead of Material #14291

Consider placing object_space_normal_map property on Texture instead of Material #14291

Conversation

pailhead commented Jun 15, 2018 • edited Loading

looeee commented Jun 15, 2018

looeee commented Jun 15, 2018

pailhead commented Jun 15, 2018 • edited Loading

Mugen87 commented Jun 15, 2018

pailhead commented Jun 15, 2018 • edited Loading

looeee commented Jun 15, 2018 • edited Loading

pailhead commented Jun 15, 2018 • edited Loading

mrdoob commented Jun 17, 2018

mrdoob commented Jun 17, 2018

pailhead commented Jun 15, 2018 •

edited

Loading

pailhead commented Jun 15, 2018 •

edited

Loading

pailhead commented Jun 15, 2018 •

edited

Loading

looeee commented Jun 15, 2018 •

edited

Loading

pailhead commented Jun 15, 2018 •

edited

Loading