-
-
Notifications
You must be signed in to change notification settings - Fork 21.5k
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 optional depth fog to Environment #84792
Conversation
fd97c77
to
53e0f97
Compare
servers/rendering/renderer_rd/shaders/forward_clustered/scene_forward_clustered.glsl
Outdated
Show resolved
Hide resolved
servers/rendering/renderer_rd/shaders/forward_mobile/scene_forward_mobile.glsl
Outdated
Show resolved
Hide resolved
54c45b8
to
dd20fb1
Compare
620613e
to
878832e
Compare
ec558c7
to
5cbae2d
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
When it has been reviewed, we are in feature freeze right now so it won't be evaluated until after 4.2 has been released, please have patience 🙂 Please add your opinions to: This is not the place for these points |
0a84eb0
to
f89253d
Compare
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! This is looking really good. I tested locally and this appears to work well on the RD backends. Its currently broken in the compatibility backend but I have left comments on how to fix that.
I think overall this is very close to being ready to merge, but I have left a few suggestions for things that are needed to get it fully ready.
When you update I suggest that you also rebase to squish the commits together to get this ready for merging as I suspect we should be able to merge pretty quickly after you update to address my comments.
servers/rendering/renderer_rd/shaders/forward_clustered/scene_forward_clustered.glsl
Outdated
Show resolved
Hide resolved
servers/rendering/renderer_rd/storage_rd/render_scene_data_rd.h
Outdated
Show resolved
Hide resolved
6adfd1a
to
f38fbf4
Compare
Fixed |
@clayjohn There is a problem with compatibility mode, i need help with this |
The uniforms in the UBO may be misaligned to std140 alignment in the compatibility shaders. |
3a7663d
to
08f4560
Compare
Fixed |
Thanks @HybridEidolon and @scriptsengineer for this amazing joint first contribution! 🎉 🥇 |
I am testing out the new depth fog in 4.3 dev 5. It seems to be working well with a procedural sky. Is there a way to get the depth fog to blend with a sky box? Or a way to get a sky box (or I suppose any other object) excluded from the depth fog? |
Yes, using the atmospheric perspective setting
For regular objects, there is a flag to ignore fog in their material. For sky materials, I don't think we expose the flag, but we probably could |
There's a Sky Affect property you can adjust in the fog properties. Setting it to 0 will prevent fog from affecting the sky at all, regardless of which sky material it currently is.
Fog aerial perspective can be used for this, but it'll be much more effective if #89995 is merged. |
I've attached three images using all three types of sky materials. Are these the expected results? |
I'd like to suggest that "atmospheric perspective" will be better for users than "aerial perspective". The terms seem to be interchangeable. Or possibly "Background Blend" since that is a more direct description of what it is actually doing. |
Yes. If you want more precise fading, you need something like #89995 which isn't merged (and may require a different approach as per the comments).
We can't rename existing properties without breaking compatibility with existing projects, so we should stick to aerial perspective for the time being. If we add a dedicated background blend property, then it should have its own name that can't be mistaken for aerial perspective. |
Thanks for the response. The new depth fog is a good change.
I'm not sure if I need more precision, exactly, but some way to blend the horizon and the terrain together while also having an unfogged sky is what I ultimately need. I may be able to get the results I want with what you've already done. The end result can be artistically correct instead of physically correct and I'm still happy. The help text for aerial perspective says the fog's color will be blended with the "color of the background sky". Is it then blending to a single average color of the sky, or is it using the average of the color at the horizon of the sky, or ? |
Aerial perspective blends the fog color with the (blurred) radiance map of the sky. The radiance maps is what's used for material reflections when no GI method is used and there are no reflection probes nearby. If you set Fog > Sky Affect below 1.0, the appearance of aerial perspective fog won't change (I just tested). |
After some experimentation I'm getting great results. Thanks for all the effort that went into this.
What I'm seeing is if I set Aerial Perspective to 1.0, then the Sky Affect value has no effect. Not quite the opposite of what you said, but I wonder if that was a typo? Here is a screenshot of what I'm working on so you can see how the depth fog is working. |
One thing I just noticed in 4.3 dev 5 is that if I enable "Volumetric Fog" the fog effect fills the entire screen. I'd file a separate issue, but I think it must be related. |
anyone know if this is this gonna be in Godot 4.3? |
It's already implemented as of 4.3.beta3 🙂 Set the fog mode to Depth in the Environment properties. |
thanks man that is epic news for me, maybe i'll just jump to beta |
Continuation of the PR created as a draft here #66030
Summary:
fog_mode
: Adds option to choose between normal fog called here exponential and new fog called fog depth, fog depth used in a specialization constant in the relevant scene shaders. This addresses the performance caveat discussed in Automatically fade to Z far in environment depth fog #55388 by ensuring that when only exponential fog is used, the branch is never present in the compiled program. It is not possible to use quadratic fog exclusively at the shader level in this implementation, but you can set the parameters of the environment such that the exponential fog density is 0 while still having quadratic fog if you must have no exponential fog at all. During project conversion, this property should be set to true if fog was enabled, the new fog density should be set to 0, and the sky affect scale should also be set to 0.fog_depth_curve: Same as in Godot 3.x
fog_density
: In fog exponential it continues with its same functionality, but in fog depth it replaces the alpha value of fog_depth_color in Godot 3.x. Project conversion should take the fog color's alpha and put it in this property instead.fog_depth_begin
: Same as in Godot 3.xfog_depth_end
: Same as in Godot 3.x✅ Merged successfully
✅ Removed unnecessary pads
✅ Added enum option in the environment inspector
✅ Added conditional in shader to execute correct fog mode
I was wondering if there is a better way to define this if
scene.glsl:
✅ Possible to add negative values in fog begin
godot.windows.editor.x86_64_cGJmj07bao.mp4