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

Revert the change of the limit for interpolation of F0 for dielectrics and metals for Screen Space Reflections #79624

Merged
merged 1 commit into from
Jul 21, 2023

Conversation

mandryskowski
Copy link
Contributor

@mandryskowski mandryskowski commented Jul 18, 2023

Commit 2c000cb changed the interpolation limits from (0.04, 1.0) to (0.04, 0.37). This is incorrect, as we want to have an F0 of 0.04 for dielectrics (materials with metalness of 0.0) and an F0 of 1.0 for metals. The Schlick approximation uses an F0 of 0.04 for all dielectrics.
The Fresnel effect does not occur with metals and their F0 has to be 1.0 (they fully reflect normally incident light). Note that the model only assumes metalness of 0.0 or 1.0 but in practice we will have intermediate values when sampling textures which is why interpolating is valid in this case (even if not physically correct).
Relevant to #79549

…o metallic and SSR

Commit 2c000cb changed the interpolation limits from (0.04, 1.0) to (0.04, 0.37). This is incorrect, as we want to have an F0 of 0.04 for dielectrics (materials with metalness of 0.0) and an F0 of 1.0 for metals.
The Schlick approximation uses an F0 of 0.04 for all dielectrics and it's good enough.
Having it lower than 1.0 leads to an incorrect application of the Fresnel effect for metals and leads to bugs like godotengine#79549
@mandryskowski mandryskowski requested a review from a team as a code owner July 18, 2023 17:28
@mandryskowski mandryskowski deleted the patch-1 branch July 18, 2023 18:36
@mandryskowski mandryskowski restored the patch-1 branch July 18, 2023 18:37
@mandryskowski mandryskowski reopened this Jul 18, 2023
@YuriSizov YuriSizov added this to the 4.x milestone Jul 18, 2023
@Calinou Calinou added topic:3d cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release labels Jul 18, 2023
@clayjohn
Copy link
Member

Tested locally:

4.0-stable
Screenshot from 2023-07-19 10-40-12

4.1-stable
Screenshot from 2023-07-19 10-40-57
This PR
Screenshot from 2023-07-19 10-44-23

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. The results are much more convincing.

@clayjohn clayjohn modified the milestones: 4.x, 4.2 Jul 19, 2023
@n0tank3sh
Copy link
Contributor

n0tank3sh commented Jul 21, 2023

The Shlick term uses 0.04 because the air as Air has a refractive index of 1.5 which from equation ((1 - 1.5)/(1 + 1.5))^2 we get 0.04 and we 0.37 from this same equation using the refractive index of germanium about 4, I guess. That's how I got 0.37. Total reflection is the theoretical limit that cannot be achieved practically. By changing it to 1 we once again going to arrive at the same problem mentioned in #75302 (comment).

@clayjohn
Copy link
Member

The Shlick term uses 0.04 because the air as Air has a refractive index of 1.5 which from equation ((1 - 1.5)/(1 + 1.5))^2 we get 0.04 and we 0.37 from this same equation using the refractive index of germanium about 4, I guess. That's how I got 0.37. Total reflection is the theoretical limit that cannot be achieved practically. By changing it to 1 we once again going to arrive at the same problem mentioned in #75302 (comment).

That issue was caused by the erroneous flipping of the normal's Y-coordinate. The F0 term only controls the blend between the computed m value and 1.0 which controls how much of the radiance map to include in the reflection.

Ultimately, we need to store F0 in the scene shader instead of metallic so we can do better than that approximation

@n0tank3sh
Copy link
Contributor

I am not mentioning the main issue but the sub issue mentioned there in the comment. Which was there was sudden change in the metallic material. plus f0 which is 1 that is 100 percent reflection of incident ray is not possible practically.

@mandryskowski
Copy link
Contributor Author

The Shlick term uses 0.04 because the air as Air has a refractive index of 1.5 which from equation ((1 - 1.5)/(1 + 1.5))^2 we get 0.04 and we 0.37 from this same equation using the refractive index of germanium about 4, I guess. That's how I got 0.37. Total reflection is the theoretical limit that cannot be achieved practically. By changing it to 1 we once again going to arrive at the same problem mentioned in #75302 (comment).

I think this is the right formula to use but I also believe that there are materials with F0 very close to 1.0 (e.g. silver has F0 = 0.97 with wavelength set to 0.7 µm which is red light and F0 = 0.93 with 0.4 µm which is violet).

Your calculations for germanium are correct but this material does not have the highest possible reflectance. It might seem so since it has such a high (the highest?) refractive index.

However, calculating F0 is a bit different with metals since they have a complex refractive index (n + ik). We apply the same formula but using the complex value, so ((1-n-ik)/(1+n+ik))^2.

The result of this calculation will be a complex value. We take the magnitude (modulus) of this value to get our F0.

@YuriSizov YuriSizov merged commit 1e856b6 into godotengine:master Jul 21, 2023
@YuriSizov
Copy link
Contributor

Thanks! And congrats on your first merged Godot pull-request!

@mandryskowski mandryskowski deleted the patch-1 branch July 25, 2023 20:35
@YuriSizov YuriSizov removed the cherrypick:4.1 Considered for cherry-picking into a future 4.1.x release label Sep 21, 2023
@YuriSizov
Copy link
Contributor

We decided against cherry-picking it for 4.1.x because it may cause issues in existing Godot 4.1 projects that have to deal with the current behavior. We prefer to preserve compatibility in such situations, even if it's a compatibility with workarounds. If someone needs this fix, upgrading to 4.2 is their option.

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.

5 participants