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

Radial fog for clustered renderer #53196

Merged
merged 1 commit into from
Sep 29, 2021
Merged

Conversation

manueldun
Copy link
Contributor

@manueldun manueldun commented Sep 28, 2021

This pull request is for the issue #52707, however, it is for the "clustered renderer" back end only since I could not make the fog appear in the "forward mobile" back end by user means.

It is possibly a bug or there is a way to enable it that I don't know.

Bugsquad edit: Fixes #52707.

@manueldun manueldun requested a review from a team as a code owner September 28, 2021 21:53
@Calinou Calinou added this to the 4.0 milestone Sep 28, 2021
@clayjohn
Copy link
Member

This pull request is for the issue #52707, however, it is for the "clustered renderer" back end only since I could not make the fog appear in the "forward mobile" back end by user means.
It is possibly a bug or there is a way to enable it that I don't know.

It sounds like a bug. Changes that affect core rendering should be made both in the clustered renderer and the mobile renderer.

Also, after testing, can you confirm that this change fixes the issue in #52707?

@manueldun
Copy link
Contributor Author

After this commit the fog behaves radially, which fixes the #52707.

Something that I did not consider is the Sky.glsl shader referred in the issue #52707. It uses scene_data.z_far to calculate the fog amount, which from my testing seems to be the viewport's frustum far distance, and affects the only background rendering. I am not sure if it should be that way and the fog cannot be computed radially with that information.

I will make the changes of the mobile renderer the same way I did for the clustered renderer.

@mrjustaguy
Copy link
Contributor

As the person who opened #52707 I'd say it works just fine so I confirm it fixes the issue, but yeah Mobile renderer is currently in a broken state, Lights are also missing right now.

@clayjohn
Copy link
Member

Looks good. Can you please squash your commits together?

Something that I did not consider is the Sky.glsl shader referred in the issue #52707. It uses scene_data.z_far to calculate the fog amount, which from my testing seems to be the viewport's frustum far distance, and affects the only background rendering. I am not sure if it should be that way and the fog cannot be computed radially with that information.

You don't need to make changes to the sky. Since it uses a constant fog amount it doesn't have the problem described in #52707

@manueldun
Copy link
Contributor Author

I spend more time with the squashing that I'd like to admit, at least i get to practice with source control, I hope is correct now. If not, let me know.

@clayjohn
Copy link
Member

It looks like you added two more commits instead of squashing them. The docs have a very helpful explanation for how to squash commits here https://docs.godotengine.org/en/stable/community/contributing/pr_workflow.html#the-interactive-rebase

@manueldun
Copy link
Contributor Author

How about now? if the source controls is not in the right state I will consider doing another pull request.

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! Good work.

@akien-mga akien-mga added bug and removed enhancement labels Sep 29, 2021
@akien-mga akien-mga merged commit 86a2e10 into godotengine:master Sep 29, 2021
@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.

Vulkan: Fixed Fog is no longer radial and fades according to the view plane instead
5 participants