Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Support object-space normal maps #14239

Merged
merged 1 commit into from
Jun 17, 2018

Conversation

WestLangley
Copy link
Collaborator

As proposed in #14139.

@WestLangley
Copy link
Collaborator Author

/ping @Julien-Arlandis
/ping @Ben-Mack

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
Copy link
Contributor

pailhead commented Jun 8, 2018

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
Copy link
Contributor

pailhead commented Jun 8, 2018

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
Copy link
Contributor

bhouston commented Jun 9, 2018

This is a great change @WestLangley. Thank you.

@Julien-Arlandis
Copy link

Very good work/ Thanks !

@Julien-Arlandis
Copy link

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
Copy link
Collaborator Author

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
Copy link
Contributor

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:

  1. extra code
  2. lots of files touched
  3. should it be on the material, or on the map?
var myNormalMap = new Texture({normal: THREE.ObjectSpaceNormal})
  1. 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
Copy link
Contributor

pailhead commented Jun 14, 2018

Related:

The three.js community has started looking into other engines and tools for integration and influence. I'm not sure if three will be merged with Sea3D, but Unity too was cited as influence for #7522.

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

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
Copy link
Contributor

Why is @pailhead capable of blocking perfectly good PRs?

@pailhead
Copy link
Contributor

pailhead commented Jun 14, 2018

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
Copy link
Contributor

@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
Copy link
Contributor

bhouston commented Jun 14, 2018

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.

Copy link
Contributor

@pailhead pailhead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

consider moving this property to Texture

@@ -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,
Copy link
Contributor

@pailhead pailhead Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

objectSpaceNormalMap: material.normalMap.objectSpace,

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

this.normalMap = source.normalMap;
this.normalMapType = source.normalMapType;
Copy link
Contributor

@pailhead pailhead Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

this.normalMap = null;
this.normalMapType = TangentSpaceNormalMap;
Copy link
Contributor

@pailhead pailhead Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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

this.normalMap = source.normalMap;
this.normalMapType = source.normalMapType;
Copy link
Contributor

@pailhead pailhead Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

If not, consider adding documentation for this class.

@@ -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;
Copy link
Contributor

@pailhead pailhead Jun 14, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@pailhead pailhead left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Consider completing or changing the documentation.

@@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@@ -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>
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@bhouston
Copy link
Contributor

@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
Copy link
Collaborator Author

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
Copy link
Contributor

pailhead commented Jun 15, 2018

@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
Copy link
Collaborator Author

@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
Copy link
Collaborator

looeee commented Jun 15, 2018

@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
Copy link
Owner

mrdoob commented Jun 17, 2018

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 mrdoob merged commit 6e89128 into mrdoob:dev Jun 17, 2018
@mrdoob
Copy link
Owner

mrdoob commented Jun 17, 2018

Thanks @WestLangley!

@mrdoob mrdoob added this to the r94 milestone Jun 17, 2018
@WestLangley WestLangley deleted the dev-object_space_normal_map branch June 17, 2018 01:38
@mrdoob
Copy link
Owner

mrdoob commented Jun 18, 2018

The model from #14139 looks great!

screen shot 2018-06-17 at 6 37 28 pm

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants