-
-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
Conversation
/ping @Julien-Arlandis 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. |
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:
1% of the users, recent request
Related: #7522 I would like to ask for this to be postponed and possibly done with It adds too much code to the core, for an API that's being deprecated, further coupling built-in materials with the core. |
Code illustration for this problem:
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 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. |
This is a great change @WestLangley. Thank you. |
Very good work/ Thanks ! |
3Dmap with the new normal map configuration : http://vm1.3dmap.fr:1357/mapRender/?ref=savoie |
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:
var myNormalMap = new Texture({normal: THREE.ObjectSpaceNormal})
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 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. |
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: The material then just takes the texture. I don't just putting all this stuff on the 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 |
Why is @pailhead capable of blocking perfectly good PRs? |
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 ^ Not a blocker, just a concern.
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. |
@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. |
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. |
There was a problem hiding this 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, |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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; |
There was a problem hiding this comment.
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.
There was a problem hiding this 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> |
There was a problem hiding this comment.
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> |
There was a problem hiding this comment.
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
.
@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. |
The |
Would you consider not overloading materials, but rather using this as a property of Please consider adding an example with appropriate assets. |
@pailhead Coupling a modification of the If you feel you have a better design for the |
@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. |
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 |
Thanks @WestLangley! |
The model from #14139 looks great! |
As proposed in #14139.