Remove material.ambient, no need for a secondary surface color just for ambient light

[bhouston]

Per discussion here: #5741 (comment)

Now it should be mentioned that sometimes material.ambient is considered to be a per-material incoming non-directional light intensity (sometimes called material.selfIllumination, but not to be confused with material.emissive) rather than as a surface color specifically for ambient light as it is treated now If we wanted to go that route, we would modify the shading equation and leave material.ambient around, and it would be additive with ambientLightColor, both of which would then be modulated by the material.color.

I am okay with either route.

[mrdoob]

I've had a look and everything seems fine.
@WestLangley Looks good to you too?

[WestLangley]

@mrdoob Remember that not all users are interested in physically-based shading. Removing the ambient material property removes a feature that some users may want. An alternate solution is to leave the code as-is, and set material.ambient = material.color by default, unless it is specifically set to something else. I do agree, however, that removing the property makes things simpler. I am amenable to either approach.

@bhouston material.ambient is the ambient reflectance of the material. It has no other meaning in three.js. It is not a light intensity.

Here are some errors I have noticed so far:

In the Phong shader, you cannot add the terms in the following equation because they have different units.

gl_FragColor.xyz = gl_FragColor.xyz * ( emissive + totalDiffuse + ambientLightColor ) + totalSpecular;

Also in ShaderSkin.js, I am pretty sure your units do not match - the original code is a bit difficult to follow...

gl_FragColor.xyz = gl_FragColor.xyz * ( totalLight + ambientLightColor ) + specularTotal;

[bhouston]

@WestLangley wrote:

In the Phong shader, you cannot add the terms in the following equation because they have different units.

I think that before it was ambientLightColor * ambient * gl_FragColor.xyz, which seemed slightly wrong because ambientLightColor was double modulated by ambient and then by gl_FragColor.xyz (which is diffuse map) in this version.

I do fix up this particular line of the phong shader fairly well in this separate PR:

https://github.com/mrdoob/three.js/pull/5805/files#diff-3d33ad8ea54d86cc060ca7b09c39d896R236

(Although I can clean up the totalSpecularLight term in the above other PR as it is actually already pre-multiplied by specular, which can make it seem like there is a unit issue when there isn't.)

[bhouston]

Have a great Christmas all!, especially @mrdoob and the ever vigilant @WestLangley! I am out for the evening, and the rest of the week. Will be back for sure Monday.

[WestLangley]

@bhouston Please check again. It should be a simple change to substitute diffuse for ambient. If you find an error in the original code that you need to fix, then please point it out and explain the change you made.

[bhouston]

@WestLangley Please note that in this line here, ambientLightColor was double modulated, it was a bug. I removed the modulation by ambient: https://github.com/mrdoob/three.js/pull/5809/files#diff-3d33ad8ea54d86cc060ca7b09c39d896R276

I fix the majority of the issues with diffuse and gl_FragColor, and there are quite a few in the other PR. Do you want me to back port all those fixes into this PR? i can do that, but it will make this PR unweildy.

[bhouston]

@WestLangley I will add back the double modulation issue so it will be more of a replace of ambient with diffuse in this PR. I think that will get this past review - while the larger fixes to issues with gl_FragColor will remain in the other PR.

[WestLangley]

Please note that in this line here, ambientLightColor was double modulated, it was a bug. I removed the modulation by ambient.

I do not believe it was a bug. I believe you have introduced a bug.

Forking this PR, if I set material.diffuse to 0x000000, material.specular to 0x000000, and the ambient light to 0xffffff, I should see a black material, because the diffuse reflectance is zero. But with this PR, I don't. I see a white material, because the ambient light is not modulated by anything.

Here is the change you made:

Before

gl_FragColor.xyz = gl_FragColor.xyz * ( emissive + totalDiffuse + ambientLightColor * ambient ) + totalSpecular;

after

gl_FragColor.xyz = gl_FragColor.xyz * ( emissive + totalDiffuse + ambientLightColor ) + totalSpecular;

To try to prevent a lot of back-and forth, gl_FragColor.xyz in the above code is vec3(1) if there is no material map, and it is equal to the texel color if there is a map.

Also, in three.js, the diffuse reflectance of the material is defined to be (pseudo code):

diffuseReflectance = material.diffuse * material.map

Similarly (prior to this PR) for the ambient reflectance:

ambientReflectance = material.ambient * material.map // previously

After this PR, the ambient reflectance should be the same as the diffuse reflectance:

ambientReflectance = material.diffuse * material.map // after this PR

So the radiance due to ambient light is

radiance = ambientReflectance * ambientLightColor

Can you please explain how or where in the existing code, "ambientLightColor was double modulated".

---

P.S. Yes, emissive is not handled correctly in the current code. And yes, your idea of refactoring the code to fix how gl_FragColor is handled is a good one.

[bhouston]

@WestLangley I've made the change you requested. :)

My reasoning is that if there is an "ambient" surface color, it shouldn't be modulated by the diffuse map. But you believe it should. That is okay.

It was the same issue I saw with the emissive being modulated by the diffuse map -- it seemed strange/incorrect.

But I think when one has non-physical models (such as a separate surface modulation color for incoming ambient light), it is easy to argue about the correct way to implement it.

[WestLangley]

@bhouston I have found several more bugs introduced by this PR.

1. webgl_materials_bumpmap_skin.html
   in THREE.ShaderSkin[ "skinSimple" ];
   ambientLightColor is unmodulated
2. webgl_materials_skin.html
   in THREE.ShaderSkin[ "skin" ];
   ambientLightColor is unmodulated
3. webgl_terrain_dynamic.html
   in THREE.ShaderTerrain[ "terrain" ];
   ambientLightColor is unmodulated
4. webgl_materials_normaldisplacementmap.html
   in THREE.NormalDisplacementShader
   ambientLightColor is unmodulated, and
   ambient is no longer a property of material - remove it
5. webgl_shading_physical.html
   ambient is no longer a property of material - remove it
6. webgl_shadowmap.html
   ambient is no longer a property of material - remove it

You can see these bugs by setting diffuse and specular to 0x000000. The rendered material should be black, but it is not.

What you have effectively done is set the ambient reflectance to 0xffffff.

It should be the same as diffuse.

[bhouston]

Nice catches @WestLangley. I'm made those requested changes.

[WestLangley]

@bhouston Would you mind tracking down any remaining issues in this PR yourself?

Code reviewing and testing this PR is taking me a lot of time, and with all due respect, you should be demonstrating that the PR is correct. Instead, I must demonstrate that it is not.

Please see the Wiki article How to Contribute to three.js.

I think you have a lot of good ideas, and I, too, want to see your good ideas implemented in a timely and efficient manner. : - )

[bhouston]

@WestLangley Thanks for your time! I do appreciate it. I do think this PR is ready to go now.

[bhouston]

@WestLangley I am trying to make everything change as a separate PR (that is why I've made 7), and to make them as atomic as possible. I am trying as best to respond to your feedback on how to proceed. Maybe we could try to split the PR review across multiple different people so the load isn't all on you? You can say which ones interest you, if any, and I can try to pull in others to review the others. In a few days, I'll try to get the screenshot tool working, but its coverage will necessarily be less broad than all examples, although it will have deeper coverage on the main shaders.

[WestLangley]

@bhouston That sounds good. I will gladly let others more knowledgeable than myself provide feedback on code design and refactoring.

[mrdoob]

@repsac do you have any thoughts on this?

[mrdoob]

Well. I like both the code and mental simplification. So lets do it!
Many thanks @bhouston! ^^

[bhouston]

Wohoo! Thanks @mrdoob!