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

Add fixed fog to the sky in the Compatibility renderer #95662

Merged
merged 1 commit into from
Aug 19, 2024

Conversation

clayjohn
Copy link
Member

Fixes: #66456

The entire implementation was missing. Some of the infrastructure was already there and just needed to be plugged in and finished

@clayjohn clayjohn added bug topic:rendering cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release labels Aug 16, 2024
@clayjohn clayjohn added this to the 4.4 milestone Aug 16, 2024
@clayjohn clayjohn requested a review from a team as a code owner August 16, 2024 22:30
@@ -777,7 +777,6 @@ void RasterizerSceneGLES3::_draw_sky(RID p_env, const Projection &p_projection,
ERR_FAIL_COND(p_env.is_null());

Sky *sky = sky_owner.get_or_null(environment_get_sky(p_env));
ERR_FAIL_NULL(sky);
Copy link
Member Author

Choose a reason for hiding this comment

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

When doing a custom sky pass for fog we can continue without the sky. This is used when you have fog, but are using a custom color background.

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Tested locally with #66456 MRP, it mostly works as expected. There seems to be a sRGB/linear color conversion issue though:

Forward+ Mobile Compatibility
Screenshot_20240817_194256 Screenshot_20240817_194242 Screenshot_20240817_194228

With tonemapping set to Linear in the MRP:

Forward+ Mobile Compatibility
Screenshot_20240817_194456 Screenshot_20240817_194510 Screenshot_20240817_194523

With tonemapping set to Linear in the MRP and fog density set to 1 (so you can compare the actual fog color):

Forward+ Mobile Compatibility
Screenshot_20240817_194720 Screenshot_20240817_194705 Screenshot_20240817_194651

Sky colors with Linear tonemapping:

Forward+ Mobile Compatibility
#858c9c #b5c1d3 #3b4354

The Mobile rendering method has a similar issue, but the sky color is too bright instead of being too dark.

@clayjohn
Copy link
Member Author

@Calinou Oh! good catch there, I was testing with white fog, so it looked the same in all backends to me.

I think I know what is wrong in the mobile backend, the code looked a little funny to me, but I left it alone as it was out of scope. I can probably do a quick fix for both

And apply luminance multiplier after fog in RD renderer
@clayjohn
Copy link
Member Author

Pushed a fix for your comment.

Before

Forward+ Mobile Compatibility
Screenshot from 2024-08-17 23-29-52 Screenshot from 2024-08-17 23-30-13 Screenshot from 2024-08-17 23-30-48

After

Forward+ Mobile Compatibility
Screenshot from 2024-08-17 23-33-00 Screenshot from 2024-08-17 23-32-41 Screenshot from 2024-08-17 23-32-20

Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

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

Works great now 🙂

Fog Density = 0.05

Forward+ Mobile Compatibility
Screenshot_20240818_184214 png webp Screenshot_20240818_184228 png webp Screenshot_20240818_184239 png webp

Fog Density = 1.0

Forward+ Mobile Compatibility
Screenshot_20240818_184418 png webp Screenshot_20240818_184432 png webp Screenshot_20240818_184440 png webp

@akien-mga akien-mga merged commit c6400a8 into godotengine:master Aug 19, 2024
18 checks passed
@akien-mga
Copy link
Member

Thanks!

@akien-mga
Copy link
Member

Cherry-picked for 4.3.1.

@akien-mga akien-mga removed the cherrypick:4.3 Considered for cherry-picking into a future 4.3.x release label Sep 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug cherrypick:4.2 Considered for cherry-picking into a future 4.2.x release topic:rendering
Projects
None yet
Development

Successfully merging this pull request may close these issues.

OpenGL: Environment fog does not affect sky rendering
3 participants