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

Reflectivity through specular color #7419

Merged
merged 3 commits into from
Oct 29, 2015

Conversation

bhouston
Copy link
Contributor

This PR is built upon the simplified lighting PR here: #7324

According to the Disney PBR paper, reflectivity should control the amount of light reflected for non-metalness. Thus it should modulate the specularColor. This PR completes that change - thus only specularColor is modulated by reflectivity and reflectivity is not used elsewhere (e.g. not in the envMap incoming light calculation.)

@mrdoob
Copy link
Owner

mrdoob commented Oct 26, 2015

@WestLangley thoughts?

@WestLangley
Copy link
Collaborator

Yes, what @bhouston says sounds reasonable, but I want to discuss this after the dev branch is working again. I am not sure what is changed here. The diff is too big.

@bhouston Can you provide a referece the related equations?

@mrdoob
Copy link
Owner

mrdoob commented Oct 26, 2015

Yes, what @bhouston says sounds reasonable, but I want to discuss this after the dev branch is working again.

Did #7455 fixed it for you?

@WestLangley
Copy link
Collaborator

Not when trying to use MeshPhysicalMaterial.

@mrdoob
Copy link
Owner

mrdoob commented Oct 26, 2015

/ping @tschw

@tschw
Copy link
Contributor

tschw commented Oct 26, 2015

Although combining several physical models, Burley's model as a whole is an empirical model. So MeshPhysicalMaterial could have a better name - e.g. MeshBurleyMaterial.

Also that metalness parameter (in that model - I didn't look at this implementation, but I have once implemented it myself) is somewhat weird (and I stripped it out). Why not pass the spectral reflectivity a.k.a. specular "color" - using the right constants yields in fact realistic metals.

/ping @tschw

But you didn't ping me for a review, right? How can I help?

@mrdoob
Copy link
Owner

mrdoob commented Oct 26, 2015

But you didn't ping me for a review, right? How can I help?

The review was a bonus. Thanks for that! 😊

I was pinging you because sounds like @WestLangley is still seeing the brokenness #7455 was meant to fix in examples using MeshPhisicalMaterial (webgl_loader_ctm_materials).

@tschw
Copy link
Contributor

tschw commented Oct 26, 2015

webgl_loader_ctm_materials

That one shows a car with matte paint and it looks really nice 👍
I have to run it on Chrome for the LOD extension to be found and the paint to show up matte, though.

The ground plane is black.

Is it supposed to look different?

@tschw
Copy link
Contributor

tschw commented Oct 26, 2015

@WestLangley is still seeing the brokenness #7455 was meant to fix in examples using MeshPhisicalMaterial

That material shouldn't be affected by #7455 in any way, which fixed flickering Lamberts. I guess the example looks fine for me.

Flicker again? Any diagnostics about uninitialized variables?

Try different browsers - I only got the warning with Firefox.

Switching on privileged extensions in about:config and uncommenting the debug line in WebGLShader allowed me to map webgl_crypticcryptic back to an entity in the code.

@WestLangley
Copy link
Collaborator

Also that metalness parameter (in that model - I didn't look at this implementation, but I have once implemented it myself) is somewhat weird (and I stripped it out).

Somewhat weird? Well, you are entitled to your opinion.

Why not pass the spectral reflectivity a.k.a. specular "color" - using the right constants yields in fact realistic metals.

There is a lot of history that went into the initial PR #7381.

The metal model for MeshPhongMaterial using combine is not appropriate for physical materials.

Most users do not understand what the diffuse reflectance of the material, and the spectral reflectance of the material are. Consequently, you will see -- even in the three.js examples -- something like material.color 0xffffff and material.specular 0xffffff simultaneously.

The decision to use the metalness workflow was a decision I made #7381. Now, there is just color and metalness; specular has been removed. The renderer sets a reasonable value for the specular reflectance automatically: 0.04 for plastics, and color for pure metals. In its current implementation, it is approximately energy conserving.

Prior to the recent PRs, MeshPhysicalMaterial was working quite nicely. It is not working for me now, as the shader is throwing errors. So I do not know if what you are seeing is what you should be seeing.

@tschw
Copy link
Contributor

tschw commented Oct 26, 2015

metalness

Somewhat weird? Well, you are entitled to your opinion.

OK, I can see why you want that parameter. With "weird" I was referring to the color normalization code to derive the reflectivity (doing a very rough approximation of luminance and using it to scale the color, IIRC).

Prior to the recent PRs, MeshPhysicalMaterial was working quite nicely. It is not working for me now, as the shader is throwing errors.

For me it looks as if it's still working quite nicely.

So I do not know if what you are seeing is what you should be seeing.

Maybe it's simpler when I say what I am not seeing: There are no errors on the log :-). Something driver-specific? What's it complaining about?

@mrdoob
Copy link
Owner

mrdoob commented Oct 26, 2015

Prior to the recent PRs, MeshPhysicalMaterial was working quite nicely. It is not working for me now, as the shader is throwing errors.

It's working here too. What errors are you getting?

The floor shouldn't be so dark though...

@WestLangley
Copy link
Collaborator

There are no errors on the log :-)

Yay! : - )

Not for me though...

THREE.WebGLShader: gl.getShaderInfoLog() fragment ERROR: 0:493: 'reflectivity' : undeclared identifier 

material.specularRoughness = roughnessFactor * 0.5 + 0.5; 
material.specularColor = mix( vec3( 0.04 ) * reflectivity, diffuseColor.rgb, metalnessFactor );

The addition of reflectivity into this formula was a modification by @bhouston.

@bhouston
Copy link
Contributor Author

I had pulled it out separately in the defines. I can fix it Wednesday when
I am back in town. I am travelling on persobal matters today and
tomorrrowm.

Best regards,
Ben Houston
http://Clara.io Online 3d modeling and rendering
On Oct 26, 2015 4:17 PM, "WestLangley" notifications@github.com wrote:

There are no errors on the log :-)

Yay! : - )

Not for me though...

THREE.WebGLShader: gl.getShaderInfoLog() fragment ERROR: 0:493: 'reflectivity' : undeclared identifier

material.specularRoughness = roughnessFactor * 0.5 + 0.5;
material.specularColor = mix( vec3( 0.04 ) * reflectivity, diffuseColor.rgb, metalnessFactor );

The addition of reflectivity into this formula was a modification by
@bhouston https://github.com/bhouston.


Reply to this email directly or view it on GitHub
#7419 (comment).

@tschw
Copy link
Contributor

tschw commented Oct 27, 2015

The addition of reflectivity into this formula was a modification by @bhouston.

Then I guess I meant the "spectral reflectance" (note the "ance") a.k.a. "L0_lambdawhatevar" or specular "color" earlier :-). Both terms even have distinct definitions - crazy physicists...

I see you also use reflectivity to weight the incidence from the environment map. Shouldn't it also consider Fresnel (for the reflection of the ray, not the one for the specular)?

@WestLangley
Copy link
Collaborator

@tschw Your input on the models is definitely welcome.

At this point, I am not sure which version you are referring to. I am currently dead-in-the-water, as the dev-build shaders are now invalid for me, and I have not had time to track down the problem.

Regarding the reflectivity parameter, yes, in the original MeshPhysicalMaterial it is not quite right, but I left it in the original proposal so I could have an extra free parameter to see what effect it would have. In the metalness model, the user does not get to control the spectral reflectance directly.

I have no problem changing the model. Well, as long as there is a compelling argument for doing so. :-)

@tschw
Copy link
Contributor

tschw commented Oct 27, 2015

I used mix( fresnel, 1.0, specularity ), where "specularity" is a parameter - similar to what Ben called "reflectivity". The compelling argument is that you can make pretty much any material reflect the environment when the angle of reflection approaches 90°.

I also multiplied it with the Smith geometry term (again, based on the reflection angle, not the one from the specular) to account for roughness. Read it in some paper and it gave a visual improvement, but I also had to apply some clamping to avoid artifacts, so I'm not sure whether I have gotten that part correctly correct.

@mrdoob
Copy link
Owner

mrdoob commented Oct 27, 2015

There are no errors on the log :-)

Not for me though...

THREE.WebGLShader: gl.getShaderInfoLog() fragment ERROR: 0:493: 'reflectivity' : undeclared identifier 

material.specularColor = mix( vec3( 0.04 ) * reflectivity, diffuseColor.rgb, metalnessFactor );

Reverted the formula. You shouldn't see errors now?

@WestLangley
Copy link
Collaborator

I see no errors. Thanks!

Note to self: I think the pesky warning

THREE.WebGLProgram: gl.getProgramInfoLog() WARNING: Output of vertex shader 'vWorldPosition' not read by fragment shader

can be removed by grepping for

#if MAX_SPOT_LIGHTS > 0 || defined( USE_ENVMAP )

and replacing with

#if defined( USE_ENVMAP )

@mrdoob
Copy link
Owner

mrdoob commented Oct 27, 2015

@WestLangley confirmed and fixed!

@WestLangley
Copy link
Collaborator

@mrdoob Sweet. Thanks!

@bhouston
Copy link
Contributor Author

BTW both specularColor and reflectivity are both what one can call F0, the fresnel reflection when the surface normal is aligned with the viewing normal. It is actually a alternative representation for a material's IOR -- you can convert between specularColor/reflectivity and IOR easily -- I've linked to such code in the past.

Reflectivity, being a single float, is appropriate for dielectric Fresnel (plastics, glass, etc), because dielectrics only do untinted reflections. For conductive Fresnel (matels) you want to be able to tint the reflected color and the MeshPhysicalMaterial does this by tinting reflecitivy by color as you increase the metalness.

It doesn't make sense to have both specularColor and reflectivity because we are defining the same thing twice. I choose SpecularColor for my implementation of Physical Materials in Clara.io. But in the Disney PBR model, they choose Reflectivity.

I am okay with either, but not both -- because then it just gets confusing.

Reflecitivity/SpecularColor should be the driver of cubemap reflections as well -- the separation of SpecularColor from reflectivity (only affects CubeMap reflections) in MeshPhongMaterial is not really physically correct.

BTW in the Disney PBR model, if you do set metalness to 0, reflecitivy/specularColor should be scaled down to 0.08 or something similar before going into the shading equations, because dielectric Fresnel materials have low reflecitivity in reality. It is only as it becomes metal (conductive Fresnel) that you can have reflecivity/specularColor near 1.0.

@tschw
Copy link
Contributor

tschw commented Oct 28, 2015

@bhouston

Thanks for taking the time to explain the rationale.

So for realistic metals we have to set diffuse color to the L0 constants for the RGB wavelengths and set metalness = 1, correct?

The reflectivity parameter has no influence on the specular color in this case, correct? Because metals always reflect?

Reflecitivity/SpecularColor should be the driver of cubemap reflections as well -- the separation of SpecularColor from reflectivity (only affects CubeMap reflections) in MeshPhongMaterial is not really physically correct.

I see there should be envmap_physical_... but the chunk file appears to missing. I guess I've been looking at the wrong code.

@WestLangley
Copy link
Collaborator

@tschw

@bhouston refactored, and moved the envmap_physical into lights_tempate. envmap_physical is gone, and needs to be removed from shaderLib.

@bhouston
Copy link
Contributor Author

@tschw wrote:

So for realistic metals we have to set diffuse color to the L0 constants for the RGB wavelengths and set metalness = 1, correct?

What is L0 constants? I am not used to that terminology.

The reflectivity parameter has no influence on the specular color in this case, correct? Because metals always reflect?

SpecularColor and reflectivity are the same thing, it is just a terminology difference -- this is why we shouldn't have both if we want our system to make physical sense. Reflectivity often has RGB channels but in the Disney PBR model it is monochromatic, I guess because it is aimed at materials that have dielectric Fresnel.

Generally, the large majority of materials reflect. For example this rant: http://filmicgames.com/archives/557 But metals have higher reflection ratios in general -- usually above 0.8, even at F0. Metals also differ in that they have no diffuse -- because diffuse models very minute subsurface absorption and re-emission (which is why it is unidirectional) -- metals do not do this. Rather metals have absorption at the interface without visible re-emission (I believe it is re-emitted at heat?), e.g conductor Fresnel, rather than pure reflection (dielectric Fresnel.) This is where specularColor makes physical sense -- because it is an attempt at modeling conductor Fresnel absorption at the interface.

But modeling metals (conductor fresnel) via specularColors is not accurate. Here is a test I did a few weeks ago comparing a true Conductor Fresnel with Dielectric Fresnel using the SpecularColor "hack": https://plus.google.com/+BenHouston3D/posts/aRUrteKg4WV

@bhouston
Copy link
Contributor Author

PS. I wrote a ThreeJS MeshMetalMaterial the other day that is a true conductive Fresnel material. But it is built on all my PRs so it isn't possible to contribute it back yet.

@tschw
Copy link
Contributor

tschw commented Oct 28, 2015

@bhouston

What is L0 constants?

L0_lambda_red, L0_lambda_green, L0_lambda_blue for real world metals are physical constants that describe the optical properties of a specific material.

The reflectivity parameter has no influence on the specular color in this case, correct? Because metals always reflect?

SpecularColor and reflectivity are the same thing, it is just a terminology difference -- this is why we shouldn't have both if we want our system to make physical sense

Yes, I get that part. I was talking code: The specular color is a function of reflectivitiy, diffuse and metalness and the influence of reflectivity gets cancelled out by metalness:

material.specularColor =
        mix( vec3( 0.04 ) * reflectivity, diffuseColor.rgb, metalnessFactor );

So reflectivity only / mostly applies to dielectrics? It's also used to weight the input from the environment map in getSpecularLightProbeIndirectLightColor, before it goes into the custom BRDF (found that one by now). So we can have a metal that does not reflect the envmap but otherwise has the typical specular response to lighting, right?

It contradicts your statement that the the environment is reflected based on the specular color alone and I am having difficulties figuring out how it can make sense.

@bhouston
Copy link
Contributor Author

UE4 uses 0.5 as the default, and they give a list of suggested specular values of a number of surfaces (all of which are in terms of the Disney PBR parameterization of [0, 0.08] for realistic dielectric Fresnel at F0):

https://docs.unrealengine.com/latest/INT/Engine/Rendering/Materials/PhysicallyBased/index.html

@WestLangley
Copy link
Collaborator

@bhouston

Aha! So we can just default reflectivity to 0.5.

From your link, I see now where the specularity term comes from and that, indeed, only non-metals are affected.

The Specular input should not be connected and left as its default value of 0.5 for most cases.

Maybe if we do decide to have a simple model and an advanced model, the reflectivity parameter can be removed from the simple model.

@tschw
Copy link
Contributor

tschw commented Oct 30, 2015

@WestLangley

Aha! So we can just default reflectivity to 0.5.

Note I kept that property, suggesting to combine reflectivity with metalness:

float metalness = smoothstep( 0.75, 1.0, specularity );
specular = mix( vec3( 0.08 * specularity ), diffuse, metalness );

It would sacrifice the upper end, figuring that a slight "metalness" may achieve a similar effect. All the values listed in the UE4 documentation can be represented.

@bhouston
Was reflectivity intended to compensate for the lack of clear coat?

@bhouston
Copy link
Contributor Author

@tschw wrote:

Note I kept that property, suggesting to combine reflectivity with metalness:

It is best to stick with what everyone else is doing because there are assets being created in a large number of tools that use the Disney/UE4 model. Merging reflectivity and metalness together means you can not really use the metalness maps generated in other tools properly in ThreeJS. I would prefer to discard "reflectivity" all together because it is generally around 0.5,, than to mix it with metalness in a non-standard fashion. I was trying to fix the issue with the previously existing "reflectivity" being non-standard by making it conform to the closest thing in the Disney PBR model -- so that it affected both direct and indirect specular equally in the same way the monochromatic "specular" behaved.

@tschw wrote:

Was reflectivity intended to compensate for the lack of clear coat?

Reflectivity is just the ThreeJS version of the monochromatic "specular" that both Disney PBR and UE4 Physical have -- and both Disney PBR and UE4 Physical have separate clear coat properties. Clear coat behaves differently than the current "reflectivity"/"specular". You can download UE4 for free and try it out there. I allow for full clear coat in Clara.io, but I should probably only allow for 0.25 weighting -- here is an animation of what clear coat (nothing to full) is over top of a slightly rough steel: https://clara.io/view/039396c3-f2f0-42dd-bdb7-4243bfa96507

Clear coat is energy conserving as well.

@tschw
Copy link
Contributor

tschw commented Oct 31, 2015

@bhouston

Merging reflectivity and metalness together means you can not really use the metalness maps generated in other tools properly in ThreeJS.

OK, this is a strong argument to keep it as is.

Reflectivity is just the ThreeJS version of the monochromatic "specular" that both Disney PBR and UE4 Physical have

I think, despite all confusion, the monochromatic approach really makes a lot of sense. Parameters cancelling each other is a little awkward, but seems to be what everyone is doing.

The usefulness of a purely dielectric specular vector is at least debatable. Do you have an example of a dielectric material with a non-monochromatic specular response or any clue why they have it?

Was reflectivity intended to compensate for the lack of clear coat?

Well, that question was pretty much answered as I read the UE4 material API.

Clear coat behaves differently than the current "reflectivity"/"specular".

I know. Still could brighten / darken the reflectivity to crutch around its absence, I guess.

You can download UE4 for free and try it out there.

Good to know they changed the licensing model. I should probably pay more attention to existing technology and what's happening in the world :-).

https://clara.io/view/039396c3-f2f0-42dd-bdb7-4243bfa96507

Very subtle - I was immediately looking for a fader to crank the roughness up.

@bhouston
Copy link
Contributor Author

BTW earlier this year I did a bit of work with Nick Porcino (of Apple now, but I know him from his ILM days) and a little bit with Brent Burley (of Disney) to help find a common PBR definition for both Clara.io and for Apple's Model I/O library (which come out as part of iOS 9) that Nick was working on. This is our working document:

https://docs.google.com/document/d/1EUqnM-aQwl76aZpduQk-EL0vGEPHrJE7h1x7wSqqLlk/edit#

You can see that this model is reflected in the final Model IO material design here:

https://developer.apple.com/library/prerelease/tvos/documentation/ModelIO/Reference/MDLScatteringFunction_Class/index.html#//apple_ref/occ/cl/MDLScatteringFunction

And here:

https://developer.apple.com/library/prerelease/tvos/documentation/ModelIO/Reference/MDLPhysicallyPlausibleScatteringFunction_Class/index.html#//apple_ref/occ/cl/MDLPhysicallyPlausibleScatteringFunction

I love the parameters that Nick chose for the rest of Apple's Model IO classes, like these light parameters, they are so clear and concise:

https://developer.apple.com/library/prerelease/tvos/documentation/ModelIO/Reference/MDLPhysicallyPlausibleLight_Class/index.html#//apple_ref/occ/cl/MDLPhysicallyPlausibleLight

https://developer.apple.com/library/prerelease/tvos/documentation/ModelIO/Reference/MDLLight_Class/index.html#//apple_ref/occ/instm/MDLLight/irradianceAtPoint:

@bhouston
Copy link
Contributor Author

I know. Still could brighten / darken the reflectivity to crutch around its absence, I guess.

Clear coat darkens the underlying specular, while adding bright pure white hilights that are typical of a lacquer. Particularly important for wood furniture like this one (which is V-Ray rendered):

https://clara.io/view/1d984b08-9711-4643-ae01-c3e53b174ace/render

It is a lacquer that makes wood look amazing, it gets nice dark colors that still respond to light but with that shininess/polish.

Also many cars have a clear coat layer that gives them their deep colors coupled with their shine - you just can not get it any other way:

https://docs.unrealengine.com/latest/INT/Engine/Rendering/Materials/MaterialProperties/LightingModels/index.html#clearcoat

Thus if you do not do clear coat, you can not do proper wood furniture nor car paints and other similar materials (e.g. the ceramics in your kitchen/bathroom.)

@bhouston
Copy link
Contributor Author

@tschw wrote:

Do you have an example of a dielectric material with a non-monochromatic specular response or any clue why they have it?

Metals have high absoprtion coefficients for various light types. Non-metals have neglible absorption coefficients because they are not conductors -- absorption == conduction, get it? :)

Also all materials have varying IORs based on the wavelength of light. This is captured by the Abbe Number parameter which attempts to fit a line to the change. This is responsible for dispersion effects, such as rainbows.

https://en.wikipedia.org/wiki/Abbe_number

Btw, you can explain varying IOR and absorption coefficients on this website: http://refractiveindex.info/?shelf=main&book=Ag&page=Rakic

@WestLangley
Copy link
Collaborator

@bhouston wrote:

I would prefer to discard "reflectivity" all together because it is generally around 0.5,, than to mix it with metalness in a non-standard fashion.

If you want to remove it completely, that is fine with me. It is up to you.

If you want to keep it, do you want to maintain suppprt for a refelctivityMap?

@tschw
Copy link
Contributor

tschw commented Oct 31, 2015

@bhouston

[ ... lots of interesting links ...]

Thanks for sharing!

First time I see a thinness parameter. I know exactly what effect it's for.

https://clara.io/view/1d984b08-9711-4643-ae01-c3e53b174ace/render (which is V-Ray rendered)

OK, that's better advertisement for the feature. Won't look much worse in real-time, I figure.

Do you have an example of a dielectric material with a non-monochromatic specular response or any clue why they have it?

[ Only conductors (which we handle differently) ... chromatic dispersion ]

Yes, I get it. But you're not really answering my question - at least not directly. Are you saying that the UE4 specular (only applying to dielectrics) is a spectral vector, so that artists or rendering passes can paint dispersion effects into textures? But that'd be odd - sitting on a silver plate won't keep a glass prism from making a rainbow?!

@mrdoob
Copy link
Owner

mrdoob commented Nov 1, 2015

I would prefer to discard "reflectivity" all together because it is generally around 0.5,, than to mix it with metalness in a non-standard fashion.

If you want to remove it completely, that is fine with me. It is up to you.

I vote for removing 😊

@bhouston
Copy link
Contributor Author

bhouston commented Nov 2, 2015

@tschw wrote:

Yes, I get it. But you're not really answering my question - at least not directly. Are you saying that the UE4 specular (only applying to dielectrics) is a spectral vector, so that artists or rendering passes can paint dispersion effects into textures?

In UE4, specular is a scalar I believe and only affects non-metals (basically they followed the Disney PBR specificiation.) UE4's "specular" has no relationship to dispersion. I've never seen a real-time engine actually have AbbeNumbers support -- I just mentioned that for completeness of understanding Fresnel effects at the interface (varying N (refraction) per wavelength is independent of varying K (absorption) per wavelength). Metals do have RGB channels for N (refraction) in most renderers, but they are not transparent, thus people do not model dispersion with these objects.

@bhouston
Copy link
Contributor Author

bhouston commented Nov 2, 2015

@tschw wrote:

https://clara.io/view/1d984b08-9711-4643-ae01-c3e53b174ace/render (which is V-Ray rendered)
OK, that's better advertisement for the feature. Won't look much worse in real-time, I figure.

Mostly, but trying to get it to look like that with the lights will require fiddling. The reason it is nearly impossible to match V-Ray scenes in ThreeJS is because of ThreeJS's light definitions are non-physical, where as everyone else's (Apple's iOS, Unity, Unreal Engine, Maya, 3DS Max, Blender) are physically based. ThreeJS's lights use a unique formulation that isn't replicated in any other tool. It is a real problem when trying to get an efficient content pipeline into ThreeJS for everyone who wants to create content for ThreeJS without having to fiddle with things.

@mrdoob
Copy link
Owner

mrdoob commented Nov 2, 2015

The reason it is nearly impossible to match V-Ray scenes in ThreeJS is because of ThreeJS's light definitions are non-physical

Maybe we can change that next?

@bhouston
Copy link
Contributor Author

bhouston commented Nov 2, 2015

@mrdoob:

Maybe we can change that next?

That would be absolutely amazing! Did you know that all lights created in Blender look completely different when exported to ThreeJS? The only thing that is transferred correctly is their geometry (position, direction) and their color, but the intensities and falloff are incorrect in nearly all cases. This is one thing that Blend4Web currently has as an advantage over ThreeJS -- Blender, when used with Blend4Web, is really close to a WYSIWYG editor, and that is very powerful.

@mrdoob
Copy link
Owner

mrdoob commented Nov 2, 2015

We'll get there! I wouldn't mind breaking changes in lights.

@tschw
Copy link
Contributor

tschw commented Nov 2, 2015

Did you know that all lights created in Blender look completely different when exported to ThreeJS?

Not just the lights, also texturing.

@WestLangley
Copy link
Collaborator

I would prefer to discard "reflectivity" all together because it is generally around 0.5,, than to mix it with metalness in a non-standard fashion.

If you want to remove it completely, that is fine with me. It is up to you.

I vote for removing 😊

@bhouston Would you like me to remove reflectivity from MeshStandardMaterial with the goal of including it in the future "advanced" material? I think that is reasonable. I would restore the 0.04 value for material.specular for non-metals.

I also notice that since reflectivity is modulating the specular reflectance term for non-metals only, that parameter has been called both specular and reflectance elsewhere. You are correct that there are better names than reflectivity, given how that parameter is now being used.

@bhouston
Copy link
Contributor Author

@bhouston Would you like me to remove reflectivity from MeshStandardMaterial with the goal of including it in the future "advanced" material?

I am fine with this. I thought you already removed it. :)

@WestLangley
Copy link
Collaborator

@bhouston

OK. I will do it. : - )

One more thing. The envMap indirect diffuse component does look pretty bad due to the cube map seams. I would suggest we comment out indirect diffuse component from envMaps temporarily -- until we implement spherical harmonics for that part, or fix the seam problem somehow. Users can always include AmbientLight or HemisphereLight for indirect diffuse lighting. How do you feel about that?

Even one more thing. In MeshStandardMaterial I am not able to get perfect reflections when roughness is 0 anymore. Reflections are blurry. I think the problem is the code that selects the mip level. Do you have plans to improve that?

@bhouston
Copy link
Contributor Author

The envMap indirect diffuse component does look pretty bad due to the cube map seams. I would suggest we comment out indirect diffuse component from envMaps temporarily -- until we implement spherical harmonics for that part, or fix the seam problem somehow.

I think we need to. It doesn't fix the seam problem completely though, the cubemaps for specular also currently have seams with Chrome and Firefox on non-ANGLE-based platforms (e.g. non-Windows.) Having a cubemap generator will fix both of these issues (irradiance and specular cubemaps.)

@bhouston
Copy link
Contributor Author

To be clear, I advocate for using 32x32x6 cubemaps for the irradiance instead of the spherical harmonics approach. The reason is scalability and less pressure on fragment uniforms. In a game level you will want to have different irradiance maps for different regions and interpolate between them. If you are doing that, you likely also want specular blur maps as well. If you combine them into the same cube maps (with the irradiance in the lower levels of the specular blur map), then it is very efficient. But if you keep them separate, you need to have the same number of specular blur maps, but then a huge number of SH parameters in the uniforms. Also in generation, it is easy to generate an irradiance cube map if you are already creating specular blur maps, so it can be efficient in terms of time and code. So for efficient sake, I advocate trying to stay with cubemaps for everything and packing specular and diffuse together to further save on uniform slots in the fragment shader and just overall resource management.

SH I believe is most useful if you only have a single pre-computed one. But otherwise, I think the benefits over a cubemap irradiance map of 32x32x6 are minimal.

@WestLangley
Copy link
Collaborator

@bhouston OK.

I would suggest we comment out indirect diffuse component from envMaps temporarily

Just so I understand, it is OK to comment it out for now?

Also, what about the 0 roughness issue I mentioned above?

@bhouston
Copy link
Contributor Author

@WestLangley Your remapping of the roughness parameter to never actually go down to 0 in the lighting equations is the reason why there are no sharp reflections in ThreeJS as it currently is. Basically this comment of yours explains why we should not have sharp reflections with a roughness of 0 -- because it is remapped to be not zero:

#7419 (comment)

I have advocated in other issues to remove this remapping because the full range of roughness is useful: #7420

@bhouston
Copy link
Contributor Author

To be clear, it is just commit of mine from last month that is required to fix the sharpness of the reflection map at roughness 0:

bhouston@e7db59e

@bhouston
Copy link
Contributor Author

You argued against this change here: #7420 (comment) But if you want sharp reflections you need to remove this remapping. If you want reflections to be crisp but not the light specular to be crisp you are advocating a disjoint physical model that makes no sense -- so you need to pick one side, either keep the non-crisp reflections and keep your remapping or move towards allowing crisp reflections and remove the remapping as I advocated last month.

@WestLangley
Copy link
Collaborator

@bhouston

You are mischaracterizing my statements and my intent. I am not going down that path with you.

@bhouston bhouston deleted the reflectivity_through_specularColor branch March 14, 2016 13:45
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.

4 participants