Support object-space normal maps #14239

WestLangley · 2018-06-08T04:38:46Z

As proposed in #14139.

WestLangley · 2018-06-08T04:45:27Z

Usage:

material.normalMapType = THREE.ObjectSpaceNormalMap; // default: THREE.TangentSpaceNormalMap

@bhouston nomenclature can be changed later.

An example can also be added later if desired. This PR is backwards-compatible.

pailhead · 2018-06-08T20:31:17Z

Would you consider not moving this into the core, as the object space normal map might be used disproportionately less compared to tangent space maps? This can be solved on top of the library.

The most recent material extension thread is this #14232.

What i'm suggesting:

99% percent of the users, for many years:

//no extra code, smaller library 
myMaterial.normalMap = myTangentNormalMap

1% of the users, recent request

var utils = require('three-material-utils') //user downloads extra code, bigger library

utils.objectNormalMap(myMaterial) //it's optional, not part of the core for 99% of the users

Related:
#14245
^why are you extending THREE.ShaderChunk if they are being deprecated. The pattern applied to other PRs would block this because of NodeMaterial?

#7522
^why are you writing GLSL if it's not future proof, this looks like a job for Node?

I would like to ask for this to be postponed and possibly done with onBeforeCompile which i would be more than happy to do tomorrow.

It adds too much code to the core, for an API that's being deprecated, further coupling built-in materials with the core.

pailhead · 2018-06-08T20:52:14Z

Code illustration for this problem:

my_normal_fragment_maps.glsl

normal = texture2D( normalMap, vUv ).xyz * 2.0 - 1.0; 

#ifdef FLIP_SIDED
  normal = - normal;
#endif

#ifdef DOUBLE_SIDED
  normal = normal * ( float( gl_FrontFacing ) * 2.0 - 1.0 );
#endif

normal = normalize( object_space_normal_normalMatrix * normal );

(removes the need for #ifdef branching, since you only inject this when you know you'll be using these type of normal maps)

var myMaterial = new Material()

myMaterial.onBeforeCompile = shader =>{ 
  shader.uniforms.object_space_normal_normalMatrix = { value: myObject.normalMatrix } 
  shader.fragmentShader = 'uniform mat3 object_space_normal_normalMatrix;\n' + shader.fragmentShader.replace('#include <normal_fragment_maps>', require('my_normal_fragment_maps.glsl') }

I read this as three lines of JS, and some data stored as a string. This PR touches 13 files, for what may end up being dead code for most people. If all the apps in existence got updated to the version of three.js that has this, this would be guaranteed dead code in all of those apps.

bhouston · 2018-06-09T14:35:25Z

This is a great change @WestLangley. Thank you.

Julien-Arlandis · 2018-06-12T12:17:47Z

Very good work/ Thanks !

Julien-Arlandis · 2018-06-13T07:37:22Z

3Dmap with the new normal map configuration : http://vm1.3dmap.fr:1357/mapRender/?ref=savoie
How to know when the commit will be passed on the official lib?

WestLangley · 2018-06-14T16:37:17Z

How to know when the commit will be passed on the official lib?

It may not have been merged because @pailhead has requested it not be.

pailhead · 2018-06-14T19:57:38Z

It may not have been merged because @pailhead has requested it not be.

Eh, when you put it like this.... 😄

I don't want to be the one blocking this, but i do want to start a discussion that may block this (in the current context).

Issues:

extra code
lots of files touched
should it be on the material, or on the map?

var myNormalMap = new Texture({normal: THREE.ObjectSpaceNormal})

is the best way to do this with defines, could it be different chunks for legibility? (don't overload normalmap_pars_fragment and normal_fragment_maps have the Renderer use the appropriate chunk based on the type of normal map)

On 1 and 2, i'm curious how is this request different from any other request that's asking for something to be done that's not in the examples. On 1 in particular, it's not that much code, but it is extra code. 2 complicates the core, which may not be the best thing to do given the NodeMaterial proposal (these i imagine would be two different normal mapping nodes, hence it ties into 4).

On 3 and 4 it also ties into this user request, just because someone requested it shouldn't mean that we should just scramble to land this ASAP, i think it needs to be reviewed.

pailhead · 2018-06-14T20:07:03Z

If we are looking at unity for guidance, this is what a texture object looks like in unity:

The material then just takes the texture.

I don't just putting all this stuff on the Material and coupling it with WebGLRenderer in so many places. I'd prefer something more generic going on, where the renderer can set this up properly in the most abstract way possible. In the future it could figure out which node to use or switch out, now, it could be a different Chunk.

I can't find an option for object/tangent space, but i'd assume it would belong on the texture.

var myTmap = new Texture()
var myOmap = new Texture()

var myMaterial = new Material()

myMaterial.normalMap = myTmap
myMaterial.normalMapType = THREE.TangentSpaceNormalMap // no bueno, why can't Material just know what kind of map i gave it

//now i want to change it
myMaterial.normalMap = myOmap // myOmap already knows its Object space

//but i have to inform my Material of this :((((((
//i dont want to do that, the computer can figure this out faster and safer
myMaterial.normalMapType = THREE.ObjectSpaceNormalMapType

bhouston · 2018-06-14T20:12:08Z

Why is @pailhead capable of blocking perfectly good PRs?

pailhead · 2018-06-14T20:19:48Z

If i know that the texture i'm loading is a normal map, and that it's in object space, i don't want to have to inform every material in the scene (and out of the scene) that it needs to treat that map as such.

var my_shared_OBJECT_SPACE_normal_map = loadTexture()

scene.traverse( o=>{
if(o.material.normalMap === my_shared_OBJECT_SPACE_normal_map){
  o.material.objectSpaceNormalMap = true
}
})

I'd rather have:

var my_shared_OBJECT_SPACE_normal_map = loadTexture()
my_shared_OBJECT_SPACE_normal_map.space = "object"

And the rest takes care of itself ^

@bhouston

Not a blocker, just a concern.

perfectly good PRs?

I think this is somewhat subjective, with all due respect i don't think that you should get to decide what's a perfectly good PR and what is not. It should be reviewed.

bhouston · 2018-06-14T20:26:54Z

@pailhead, you do realize that this is like @WestLangley's ~400th PR to Three.JS right? He is being polite to you, but I do not have that type of patience. Basically if the math/theory is correct, there is user demand, the feature itself is not weird by its nature, we should be merging this. Currently this is how Three.JS code is written. This is a straight forward PR.

If you want to refactor the code after the merge to something you like better, then you make a PR that builds atop of this, that maintains Three.JS's forward progress. And then we can judge such a PR on its own basis.

bhouston · 2018-06-14T20:28:50Z

For example, I didn't like how animations were done in Three.JS. Instead of blocking other people's contributions to ThreeJS I just rewrote the animation system into the form I thought it should be in: #6934

When I didn't like the fact that textures didn't support encodings, I didn't just complain about it, I wrote a solution in the form I thought was best: #8117

Be the change you want to see in the world. Otherwise you will not be happy and you will feel as if you are at the mercy of others.

pailhead

consider moving this property to Texture

pailhead · 2018-06-14T22:51:53Z

src/renderers/webgl/WebGLPrograms.js

@@ -148,6 +148,7 @@ function WebGLPrograms( renderer, extensions, capabilities ) {
 			emissiveMapEncoding: getTextureEncodingFromMap( material.emissiveMap, renderer.gammaInput ),
 			bumpMap: !! material.bumpMap,
 			normalMap: !! material.normalMap,
+			objectSpaceNormalMap: material.normalMapType === ObjectSpaceNormalMap,


Consider decoupling from Material, use it as a property of the normal map (Texture)

objectSpaceNormalMap: material.normalMap.objectSpace,

pailhead · 2018-06-14T22:52:28Z

src/materials/MeshPhongMaterial.js

@@ -136,6 +138,7 @@ MeshPhongMaterial.prototype.copy = function ( source ) {
 	this.bumpScale = source.bumpScale;

 	this.normalMap = source.normalMap;
+	this.normalMapType = source.normalMapType;


Consider nuking, and moving to Texture. Introduces coupling with Material and overloads this Material.

pailhead · 2018-06-14T22:52:51Z

src/materials/MeshPhongMaterial.js

@@ -79,6 +80,7 @@ function MeshPhongMaterial( parameters ) {
 	this.bumpScale = 1;

 	this.normalMap = null;
+	this.normalMapType = TangentSpaceNormalMap;


Consider nuking, and moving to Texture. Introduces coupling with Material and overloads this Material.

pailhead · 2018-06-14T22:52:59Z

src/materials/MeshNormalMaterial.js

@@ -70,6 +73,7 @@ MeshNormalMaterial.prototype.copy = function ( source ) {
 	this.bumpScale = source.bumpScale;

 	this.normalMap = source.normalMap;
+	this.normalMapType = source.normalMapType;


Consider nuking, and moving to Texture. Introduces coupling with Material and overloads this Material.

If not, consider adding documentation for this class.

pailhead · 2018-06-14T22:54:38Z

src/materials/Material.js

@@ -194,6 +194,7 @@ Material.prototype = Object.assign( Object.create( EventDispatcher.prototype ),
 		if ( this.normalMap && this.normalMap.isTexture ) {

 			data.normalMap = this.normalMap.toJSON( meta ).uuid;
+			if ( this.normalMapType !== TangentSpaceNormalMap ) data.normalMapType = this.normalMapType;


Consider nuking, would be serialized with the texture, there are possibly materials that extend Material that do not use normal maps, like the 13 other materials that all extend Material but this property is not added to.

Consider not making the super class aware of the sub classes.

pailhead

Consider completing or changing the documentation.

pailhead · 2018-06-14T23:03:56Z

docs/api/materials/MeshPhongMaterial.html

@@ -178,6 +178,13 @@ <h3>[property:Texture normalMap]</h3>
 			the way the color is lit. Normal maps do not change the actual shape of the surface, only the lighting.
 		</p>

+		<h3>[property:Integer normalMapType]</h3>
+		<p>


Consider consolidating multiple document entries by moving the property to Texture. If not, add the documentation for MeshNormalMaterial.

pailhead · 2018-06-14T23:04:14Z

docs/api/materials/MeshStandardMaterial.html

@@ -220,6 +220,13 @@ <h3>[property:Texture normalMap]</h3>
 			the way the color is lit. Normal maps do not change the actual shape of the surface, only the lighting.
 		</p>

+		<h3>[property:Integer normalMapType]</h3>


Consider consolidating multiple document entries by moving the property to Texture. If not, add the documentation for MeshNormalMaterial.

bhouston · 2018-06-14T23:27:51Z

@pailhead I find you ignored completely the main advice I offered: to allow this to be merged (don't act as a blocker) and then suggest your refactoring in a separate PR (be the chance you want to see.) I am not sure there is much more to talk about, I have other things to do.

WestLangley · 2018-06-15T01:37:30Z

add the missing documentation for MeshNormalMaterial because as is only MeshStandardMaterial and MeshPhongMaterial are documented.

The MeshNormalMaterial documentation is significantly deficient as-is, and since the deficiencies are unrelated to the feature added here, I decided that document can best be updated in a separate PR.

pailhead · 2018-06-15T05:51:23Z

@WestLangley

Would you consider not overloading materials, but rather using this as a property of Texture?

Please consider adding an example with appropriate assets.

WestLangley · 2018-06-15T06:27:53Z

@pailhead Coupling a modification of the Texture class with this PR is probably not advisable. If you want to get significant changes merged, it's wise to get buy-in from @mrdoob first -- before you invest a lot of effort in the implementation details.

If you feel you have a better design for the Texture class, outlining your proposal in a separate issue would be advisable. It would be high-risk, I expect, so be prepared; you may have more success working on something that does not impact significantly on the three.js design.

looeee · 2018-06-15T07:32:17Z

@pailhead if you want to do something really useful, the documentation always needs work. PRs for docs are much easier to get accepted as well.

mrdoob · 2018-06-17T01:19:35Z

Sorry for the delay merging this one guys...!

I was out last week on a conference and seems like things got a little bit out of control.

@pailhead I prefer having this as a property of Material rather than a property of Texture.

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

Thanks @WestLangley!

mrdoob · 2018-06-18T02:00:37Z

The model from #14139 looks great!

Support object-space normal maps

e5f809a

Julien-Arlandis approved these changes Jun 8, 2018

View reviewed changes

pailhead mentioned this pull request Jun 8, 2018

What are the plans regarding NodeMaterial? #14245

Closed

Mugen87 approved these changes Jun 9, 2018

View reviewed changes

pailhead mentioned this pull request Jun 9, 2018

Alternative to .onBeforeCompile for material extension #14232

Closed

pailhead reviewed Jun 14, 2018

View reviewed changes

pailhead mentioned this pull request Jun 15, 2018

Material super class may know too much about it's sub classes #14290

Closed

pailhead mentioned this pull request Jun 15, 2018

Consider placing object_space_normal_map property on Texture instead of Material #14291

Closed

Mugen87 mentioned this pull request Jun 16, 2018

[WIP] normalmap space example #14303

Closed

mrdoob merged commit 6e89128 into mrdoob:dev Jun 17, 2018

mrdoob added this to the r94 milestone Jun 17, 2018

WestLangley deleted the dev-object_space_normal_map branch June 17, 2018 01:38

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Support object-space normal maps #14239

Support object-space normal maps #14239

WestLangley commented Jun 8, 2018

WestLangley commented Jun 8, 2018

pailhead commented Jun 8, 2018 •

edited

pailhead commented Jun 8, 2018 •

edited

bhouston commented Jun 9, 2018

Julien-Arlandis commented Jun 12, 2018

Julien-Arlandis commented Jun 13, 2018

WestLangley commented Jun 14, 2018

pailhead commented Jun 14, 2018

pailhead commented Jun 14, 2018 •

edited

bhouston commented Jun 14, 2018

pailhead commented Jun 14, 2018 •

edited

bhouston commented Jun 14, 2018

bhouston commented Jun 14, 2018 •

edited

pailhead left a comment

pailhead Jun 14, 2018 •

edited

pailhead Jun 14, 2018 •

edited

pailhead Jun 14, 2018 •

edited

pailhead Jun 14, 2018 •

edited

pailhead Jun 14, 2018 •

edited

pailhead left a comment

pailhead Jun 14, 2018

pailhead Jun 14, 2018

bhouston commented Jun 14, 2018

WestLangley commented Jun 15, 2018

pailhead commented Jun 15, 2018 •

edited

WestLangley commented Jun 15, 2018

looeee commented Jun 15, 2018

mrdoob commented Jun 17, 2018 •

edited

mrdoob commented Jun 17, 2018

mrdoob commented Jun 18, 2018

Support object-space normal maps #14239

Support object-space normal maps #14239

Conversation

WestLangley commented Jun 8, 2018

WestLangley commented Jun 8, 2018

pailhead commented Jun 8, 2018 • edited

pailhead commented Jun 8, 2018 • edited

bhouston commented Jun 9, 2018

Julien-Arlandis commented Jun 12, 2018

Julien-Arlandis commented Jun 13, 2018

WestLangley commented Jun 14, 2018

pailhead commented Jun 14, 2018

pailhead commented Jun 14, 2018 • edited

bhouston commented Jun 14, 2018

pailhead commented Jun 14, 2018 • edited

bhouston commented Jun 14, 2018

bhouston commented Jun 14, 2018 • edited

pailhead left a comment

Choose a reason for hiding this comment

pailhead Jun 14, 2018 • edited

Choose a reason for hiding this comment

pailhead Jun 14, 2018 • edited

Choose a reason for hiding this comment

pailhead Jun 14, 2018 • edited

Choose a reason for hiding this comment

pailhead Jun 14, 2018 • edited

Choose a reason for hiding this comment

pailhead Jun 14, 2018 • edited

Choose a reason for hiding this comment

pailhead left a comment

Choose a reason for hiding this comment

pailhead Jun 14, 2018

Choose a reason for hiding this comment

pailhead Jun 14, 2018

Choose a reason for hiding this comment

bhouston commented Jun 14, 2018

WestLangley commented Jun 15, 2018

pailhead commented Jun 15, 2018 • edited

WestLangley commented Jun 15, 2018

looeee commented Jun 15, 2018

mrdoob commented Jun 17, 2018 • edited

mrdoob commented Jun 17, 2018

mrdoob commented Jun 18, 2018

pailhead commented Jun 8, 2018 •

edited

pailhead commented Jun 8, 2018 •

edited

pailhead commented Jun 14, 2018 •

edited

pailhead commented Jun 14, 2018 •

edited

bhouston commented Jun 14, 2018 •

edited

pailhead Jun 14, 2018 •

edited

pailhead Jun 14, 2018 •

edited

pailhead Jun 14, 2018 •

edited

pailhead Jun 14, 2018 •

edited

pailhead Jun 14, 2018 •

edited

pailhead commented Jun 15, 2018 •

edited

mrdoob commented Jun 17, 2018 •

edited