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

Make Fresnel darken SSR instead of blending with specular #79921

Merged
merged 1 commit into from
Oct 11, 2023

Conversation

mandryskowski
Copy link
Contributor

@mandryskowski mandryskowski commented Jul 26, 2023

This PR fixes (partially) #79549 due to Fresnel incorrectly blending SSR with specular, but only if fade in and fade out are set to 0.0.
Without fade in/fade out, the reflections are fully opaque and look correct.

This PR with fade in and fade out set to 0.0:
image
image

To avoid specular light blending/passing through SSR reflections, we cannot allow the alpha value of final_color in the SSR shader to drop below 1.0.
Now fade in/fade out introduces blending with specular light. Because the sun in the HDR image is so strong (the brightest sun pixel is RGB(41184.0, 35776.0, 28624.0) compared to around RGB(0.4, 0.4, 0.4) in the SSR reflection), even mixing just a bit of specular light with SSR causes a significant change in color and produces bad results.

This PR with fade in 0.001 and fade out 0.0:
image
image

Not sure how we can fix/if we should fix the fade mixing. Maybe there is a better way to mix HDR values than glsl mix() in specular_merge.glsl.

@mandryskowski mandryskowski requested a review from a team as a code owner July 26, 2023 13:17
@YuriSizov YuriSizov changed the title Fresnel should darken SSR instead of blending with specular Make Fresnel darken SSR instead of blending with specular Jul 26, 2023
@YuriSizov YuriSizov added this to the 4.2 milestone Jul 26, 2023
@Calinou
Copy link
Member

Calinou commented Jul 26, 2023

Now fade in/fade out introduces blending with specular light. Because the sun in the HDR image is so strong (the brightest sun pixel is RGB(41184.0, 35776.0, 28624.0) compared to around RGB(0.4, 0.4, 0.4) in the SSR reflection), even mixing just a bit of specular light with SSR causes a significant change in color and produces bad results.

There is a Clamp HDR Exposure import option that should be enabled on the panorama texture in the Import dock. This is recommended when using real life-sourced panorama images.

@akien-mga akien-mga requested a review from clayjohn August 7, 2023 10:20
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.

After the explanation, this makes sense to me. I was wrong in my earlier concern. This should be good to merge

To be even more clear, there remain issues with the source file because the HDRI is way too bright. So it's not expected that this PR will make that file look perfect.

No need to cherrypick for 4.1

@clayjohn
Copy link
Member

clayjohn commented Oct 6, 2023

The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation https://docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

@akien-mga
Copy link
Member

The final step before merging is to squash all the commits together so that the whole PR only contains 1 big commit with all your changes. We like to merge one commit at a time to keep the git history clean and navigable.

If you don't know how to do that, we have a helpful tutorial in the official documentation docs.godotengine.org/en/latest/community/contributing/pr_workflow.html#the-interactive-rebase

Ping @mandryskowski - are you able to do this update?

@clayjohn clayjohn removed request for a team October 11, 2023 19:01
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 great now!

@akien-mga akien-mga merged commit 09b92a1 into godotengine:master Oct 11, 2023
15 checks passed
@akien-mga
Copy link
Member

Thanks!

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.

6 participants