Restore the Fresnel term in the BRDF. #22483
Conversation
drivers/gles2/shaders/scene.glsl
Outdated
| //float F = mix(cLdotH5, 1.0, F0); | ||
| float F0 = 1.0; | ||
| float cLdotH5 = SchlickFresnel(cLdotH); | ||
| float F = mix(cLdotH5, 1.0, F0); |
There was a problem hiding this comment.
Currently it's doing nothing. F0 is always 1.0 and so the mix() call is also 1.0. I need to check back how it was supposed to work, but was planned to do later.
There was a problem hiding this comment.
Oh, right, I remember it now. This is why I put a FIXME on that line a while ago actually (which got removed in 65fd37c).
Nevertheless, as I also noted in this PR, the value of F0 should be derived, from the IOR. Did Godot have a IOR texture/parameter?
There was a problem hiding this comment.
Or F0 itself can be a material parameter actually (scalar for dielectrics, color value for metals).
Otherwise, can just set it to a typical value of 0.04.
There was a problem hiding this comment.
I am not totally convinced of exposing it, as its not that useful for artists (who just dump stuff from Substance). Most engines I saw simply leave it at 0.04, but in the case of Godot given we use forward rendering, I can imagine exposing it may not hurt anyway,
There was a problem hiding this comment.
I am also not sure that exposing this is very useful for most artists, even though admittedly the 0.5 specular value as a default is very obtuse. It may be better to include the same table that the Google Filament documentation includes. It is the best documentation I have seen on PBR in general. The table is at https://google.github.io/filament/Filament.md.html section 4.8./table 4.
Not sure what the licensing on this documentation is though?
Also this image is superb as far as explaining what the specular value does, and the relationship between the 0.5 value and the 4% F0 :
Maybe there could even be a dropdown selection in the specular field for the various ior's as per this table. This is common in some commercial renderers so that the user doesn't have to guess or memorise these values?
There was a problem hiding this comment.
Ah, right Godot already has "specular" parameter for that.
@fracteed As you can see from the Filament docs (Table 4), F0=0.04 isn't a good value for skin, water/ice and various crystals, so I think it's important to have a knob for that.
@reduz, is it OK if I rename it to specular_reflectance, or just reflectance? It's pretty confusing right now (in the editor, "specular" is confusing because "specular" typically refers to specular texture of the so-called specular workflow, and in the shader code, specular is also sometimes used for specular BRDF).
Then we can do:
vec3 F0 = mix(0.16 * reflectance * reflectance, albedo, metallic).
There was a problem hiding this comment.
Maybe there could even be a dropdown selection in the specular field for the various ior's as per this table. This is common in some commercial renderers so that the user doesn't have to guess or memorise these values?
Or this can simply be added to the docs .
|
Updated with Filament's remapping of specular reflectance. @reduz, in gles2 scene.glsl, can you check the use of specular in these lines:
They're not present in gles3 version and look like a mistake. |
drivers/gles2/shaders/scene.glsl
Outdated
| float dielectric = 0.16 * specular * specular; | ||
| // use albedo * metallic as colored specular reflectance at 0 angle for metallic materials; | ||
| // see https://google.github.io/filament/Filament.md.html | ||
| return mix(dielectric, albedo, metallic); |
There was a problem hiding this comment.
You should be using tab-based indentation here, not space-based indentation.
drivers/gles2/shaders/scene.glsl
Outdated
| } | ||
|
|
||
|
|
There was a problem hiding this comment.
No need for the extra whitespace.
tagcup
force-pushed
the
fresnel
branch
2 times, most recently
from
d2828db to
31be31d
Compare
|
Clang-format has some suggestions for you: https://travis-ci.org/godotengine/godot/jobs/435126094#L538 |
tagcup
force-pushed
the
fresnel
branch
4 times, most recently
from
ff6cff3 to
164f2ba
Compare
Was uncommented in 65fd37c, mostly likely by mistake since its important. Also made a few corrections of specular -> specular_blob_intensity (gles2).
|
@reduz, regarding my last comment comment above, I believe those should be specular_blob_intensity rather than specular, so I made those changes as well. I also adapted filament's remapping of the specular reflectance. |
|
@reduz I also added an optimized version of GGX G function for GLES2, such that GGX can hopefully be kept as default for mobile (from Filament docs, which is ultimately based on this). Filament docs also mention an optimized version of the D function here using half precision floats, but I'm not sure if it would make much of a difference.
|
|
Regarding the meaning of the anisotropy parameter, Filament seems to have adapted an alternative definition from here (slide 24), not sure if we want to do that but it for sure would be faster since it avoids a division and a sqrt. |
Also changed the mapping of anisotropy to match the common definition.
|
@tagcup are these changes likely to break existing materials for gles 3? Is there a noticeable visual difference in a before and after material? |
|
This affects only specular light (GGX variant). Another change is, effective specular reflectance of metallic materials (when using GGX) can made be colored now, by ramping up the "specular" knob (called "reflectance" in the filament pic you linked). But there shouldn't be a noticeable difference when using the default value of 0.5. I wouldn't call it "breaking existing materials" though, this is supposed to be a bug-fix, the implementation so far was incorrect. |
|
Thanks for the details. The ability to ramp up the specular past 0.5 for metallics sounds very useful. |
|
Thanks! |
Oversight by me in godotengine#22483. GLES2 doesn't seem to be supporting anisotropy at the moment anyway ---in case it gets revived.
Was uncommented in 65fd37c, mostly likely by mistake since its important.