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

Fix the limit for interpolation of R0 with respect to metallic and the calculation of the cos theata in the Fresnel Shlick term in SSR #75368

Merged
merged 1 commit into from
Apr 7, 2023

Conversation

n0tank3sh
Copy link
Contributor

@n0tank3sh n0tank3sh commented Mar 26, 2023

The default of Ro is assumed to be around 1.5 after applying the formula which is ((u -1)/(u +1))^2 => ((1.5 -1)/(1.5+1))^2=>0.04. The element with the highest refractive index found in nature is considered to be Germanium. It has a range of refractive index from 4.05 to 4.1. After applying the formula to the maximum of the range ((4.1 - 1)/(4.1 +1))^2=>(Approx)0.37. So, the rational limit should be this.
Fix #75302

@RedMser
Copy link
Contributor

RedMser commented Mar 27, 2023

The rationale for 0.37 should probably be mentioned in a code comment, since it currently looks like an arbitrarily chosen magic number.

@n0tank3sh n0tank3sh force-pushed the fix-limit-interpolation-R0 branch 2 times, most recently from 4021bf0 to 31a7754 Compare March 27, 2023 11:13
@logzero
Copy link

logzero commented Mar 27, 2023

FYI: The refractive index of Germanium listed there is for infrared. For visible light diamond with 2.4 would be the limit.

@n0tank3sh
Copy link
Contributor Author

n0tank3sh commented Mar 27, 2023

@logzero FYI: Fresnel equation is about reflection, not refraction.

@jcostello
Copy link
Contributor

@MightiestGoat Before and after?

@n0tank3sh
Copy link
Contributor Author

@jcostello wdym?

@jcostello
Copy link
Contributor

@MightiestGoat how it looks before and after the fix

@armmah
Copy link

armmah commented Mar 27, 2023

I tried using proper f0 and f90 like in frostbite paper as well, as it stood out to me on first glance. However as could be expected it makes no difference for dielectrics, and only makes a difference on metals. The problem on #75302 mentions non-metals only.

I checked your changes (with 0.37 instead of 1.0) and as expected only changes the behavior for metals.

@n0tank3sh
Copy link
Contributor Author

n0tank3sh commented Mar 27, 2023

@armmah I started working on the non-metal part as soon as jcostello demanded before and after the fix picture.

@armmah
Copy link

armmah commented Mar 27, 2023

Oh nice catch on the reflect vector. All of the experiments I've done I have been using either view_dir or -view_dir, didn't think to try with reflect over normal.

It ends up solving the problem in a proper physically accurate and consistent way.

P.S. on the tall cube the visual was similar, but your ray dir looks much nicer on some other scenarios I have set up in the material tests demo project.

@n0tank3sh
Copy link
Contributor Author

@armmah Thank you

@fire
Copy link
Member

fire commented Mar 29, 2023

Do you think it's possible to have only one commit?

@n0tank3sh
Copy link
Contributor Author

n0tank3sh commented Mar 29, 2023

@fire I think of having two commits one fixing the interpolation of R0 and another fixing the Shlick term. Is it reasonable to you? I squash all the commit @fire

@n0tank3sh n0tank3sh changed the title Fix the limit for interpolation of Ro with respect to metallic Fix the limit for interpolation of R0 with respect to metallic and the calculation of the cos theata in the Fresnel Shlick term in SSR Apr 5, 2023
@clayjohn
Copy link
Member

clayjohn commented Apr 6, 2023

There are two things going on in this PR. The change of R0 is one thing, it only changes the behaviour for metals and does not help #75302. Essentially it adds in some schlick reflectance for metallic metals.

The second change is using ray_dir instead of -view_dir which kind of addresses the issue in #75302 but it doesn't quite work. For non-metallics, reflections should be most visible at a glancing angle and then they should smoothly fade in strength as the view direction approaches the normal. With this change the reflection is strongest midway between a perpendicular view and parallel view. At a glancing angle reflections disappear.

Nearly parallel, reflection is fading away as it should
Screenshot from 2023-04-05 19-24-25

Midpoint, reflection is still strong
Screenshot from 2023-04-05 19-24-18

Glancing angle, reflections should be at their strongest, but they are faded out
Screenshot from 2023-04-05 19-24-11

@clayjohn
Copy link
Member

clayjohn commented Apr 6, 2023

Taking a look it seems like removing the reversal of the normal makes it work. I.e. the correct code is :

//normal.y = -normal.y;
float m = clamp(1.0 - dot(normal, -view_dir), 0.0, 1.0);

@n0tank3sh
Copy link
Contributor Author

@clayjohn Thank that makes sense but why not use view_dir instead of -view_dir?

@clayjohn
Copy link
Member

clayjohn commented Apr 6, 2023

@clayjohn Thank that makes sense but why not use view_dir instead of -view_dir?

-view_dir is the "eye vector" i.e. the vector from the fragment to the eye. Which is what we need for this. What we are measuring is the difference in angle between the normal and the view direction. If the camera is looking straight down at a plane the eye vector will be the same as the normal (because both are normalized) and the dot product will return a 1. If the camera is perpendicular to the normal then the dot product will return 0. This is what we want. If we were to use the view direction then when looking straight down the view vector would be -1 and would also scale to 0 as the view becomes perpendicular to the normal

@n0tank3sh
Copy link
Contributor Author

n0tank3sh commented Apr 6, 2023

Yes, I understood that we are calculating the cos theta of the two vector vectors. view_dir is a little confusing as to know which side the view_dir pointing to. Thank you for clearing it out.

@n0tank3sh n0tank3sh force-pushed the fix-limit-interpolation-R0 branch 2 times, most recently from 0968a81 to a6bf56e Compare April 6, 2023 17:42
Copy link
Member

@clayjohn clayjohn left a comment

Choose a reason for hiding this comment

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

Looks good to me. In the long term we should probably just write out f0 from the main pass instead of metallic. Then we can take specular color into account (even if its just grayscale)

@YuriSizov YuriSizov merged commit cbb2e17 into godotengine:master Apr 7, 2023
@YuriSizov
Copy link
Contributor

Thanks!

@n0tank3sh n0tank3sh deleted the fix-limit-interpolation-R0 branch April 7, 2023 14:18
@YuriSizov
Copy link
Contributor

Cherry-picked for 4.0.3.

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

Successfully merging this pull request may close these issues.

SSR Reflections fade out very early on non-metallic materials
9 participants