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 custom fog, and make FOG built-in readable #54292

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

briansemrau
Copy link
Contributor

@briansemrau briansemrau commented Oct 26, 2021

Fixes shader custom fog (#41415), which was likely broken since #45023.

Additionally, the fragment shader built-in FOG allows for modification of standard fog, rather than just overwriting it. This allows for simple tweaks, such as:

FOG *= vec4(0.5, 0.5, 1, 1); // Modulate fog color

image

Linear fog could be implemented like so (taken from #54248):

float fog_linear = clamp((length(VERTEX) - fog_linear_start) / (fog_linear_end - fog_linear_start), 0, 1);
FOG.a = max(FOG.a, fog_linear);

One difference to note from the original custom fog PR is that custom fog now mixes before volumetric fog rather than after.

Will need updating in the docs (see: godotengine/godot-docs#4301)

@briansemrau briansemrau requested a review from a team as a code owner October 26, 2021 23:24
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.

Why calculate volumetric fog after? Wouldn't it make sense to do fixed fog and volumetric fog before custom fog?

@briansemrau
Copy link
Contributor Author

briansemrau commented Oct 26, 2021

Why calculate volumetric fog after? Wouldn't it make sense to do fixed fog and volumetric fog before custom fog?

I thought this would be better because I consider volumetric fog to be an additional effect beyond standard fog, and that this custom fog applies to standard fog only. For the linear fog example, this ordering is important.

However, I can see why the opposite might be preferred for other uses.

Perhaps this could be made configurable by adding a render_mode option such as fog_premix_volumetric.

@briansemrau briansemrau marked this pull request as draft October 26, 2021 23:52
@briansemrau briansemrau marked this pull request as ready for review October 27, 2021 00:10
@YuriSizov YuriSizov added this to the 4.0 milestone Oct 27, 2021
@briansemrau
Copy link
Contributor Author

@clayjohn Would you prefer I calculate volumetric fog alongside regular fog before custom fog, restoring original behavior? Or do you think adding a render mode option is a good idea?

@clayjohn
Copy link
Member

clayjohn commented Nov 1, 2021

@briansemrau That is a difficult question. To me it made sense to do them together, but for you it was more intuitive to have FOG only override fixed fog.

This highlights the issue when devs add features without proposals. I clearly don't have a good sense of what users need and didn't realize that users may have different expectations for this feature.

You or I should write a proposal and try to get some feedback from users.

@clayjohn
Copy link
Member

I was thinking about this again. I wonder if the ideal approach is just to expose the fog_process and volumetric_fog_process to users and then leave the current fog alone. I.e. if users use FOG they will be responsible for integrating the other types of fog.

@clayjohn clayjohn modified the milestones: 4.0, 4.x Feb 13, 2023
@RPicster
Copy link
Contributor

I was thinking about this again. I wonder if the ideal approach is just to expose the fog_process and volumetric_fog_process to users and then leave the current fog alone. I.e. if users use FOG they will be responsible for integrating the other types of fog.

I fear that will make things a bit more complicated than most use cases would demand it.
So far my experiences were almost always fragment related, just having FOG exposed and useable would help me much more.

Another reason is that volumetric and normal fog are blended in magical ways no one understands, so having to reliably "reconstruct" the real fog color that is applied from two different functions... wouldn't really help me personally a lot 😵

Why not both? :D

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.

4 participants