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 optional depth fog to Environment #84792

Merged
merged 1 commit into from
Feb 18, 2024

Conversation

scriptsengineer
Copy link

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.x
  • fog_depth_end: Same as in Godot 3.x

image

✅ Merged successfully

✅ Removed unnecessary pads

✅ Added enum option in the environment inspector
godot windows editor x86_64_skoj8nb1o5

✅ Added conditional in shader to execute correct fog mode
I was wondering if there is a better way to define this if
scene.glsl:

#ifndef DISABLE_FOG_DEPTH
	if (scene_data.fog_mode == 1) {
		float fog_far = scene_data.fog_depth_end > 0.0 ? scene_data.fog_depth_end : scene_data.z_far;
		float fog_z = smoothstep(scene_data.fog_depth_begin, fog_far, length(vertex));

		float fog_quad_amount = pow(fog_z, scene_data.fog_depth_curve) * scene_data.fog_density;
		fog_amount = fog_quad_amount;
	}
#else
	if (scene_data.fog_mode == 0) {
		fog_amount = 1 - exp(min(0.0, -length(vertex) * scene_data.fog_density));
	}

✅ Possible to add negative values in fog begin

godot.windows.editor.x86_64_cGJmj07bao.mp4

servers/rendering/storage/environment_storage.cpp Outdated Show resolved Hide resolved
servers/rendering/storage/environment_storage.cpp Outdated Show resolved Hide resolved
servers/rendering/storage/environment_storage.cpp Outdated Show resolved Hide resolved
servers/rendering/storage/environment_storage.cpp Outdated Show resolved Hide resolved
doc/classes/Environment.xml Outdated Show resolved Hide resolved
doc/classes/Environment.xml Outdated Show resolved Hide resolved
@scriptsengineer scriptsengineer force-pushed the distance-fog branch 3 times, most recently from 54c45b8 to dd20fb1 Compare November 13, 2023 21:53
servers/rendering_server.compat.inc Outdated Show resolved Hide resolved
servers/rendering_server.compat.inc Outdated Show resolved Hide resolved
servers/rendering_server.h Outdated Show resolved Hide resolved
servers/rendering_server.compat.inc Outdated Show resolved Hide resolved
servers/rendering_server.compat.inc Show resolved Hide resolved
servers/rendering_server.compat.inc Outdated Show resolved Hide resolved
servers/rendering_server.cpp Show resolved Hide resolved
@scriptsengineer scriptsengineer requested review from a team as code owners November 17, 2023 13:00
@RichardEllicott

This comment was marked as off-topic.

@AThousandShips
Copy link
Member

AThousandShips commented Nov 21, 2023

any chance this gets merged? is this a technical issue or a design consideration?

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

@VecterraSoft

This comment was marked as off-topic.

@AThousandShips

This comment was marked as off-topic.

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.

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.

drivers/gles3/shaders/scene.glsl Outdated Show resolved Hide resolved
drivers/gles3/shaders/scene.glsl Outdated Show resolved Hide resolved
doc/classes/Environment.xml Outdated Show resolved Hide resolved
doc/classes/Environment.xml Outdated Show resolved Hide resolved
doc/classes/Environment.xml Outdated Show resolved Hide resolved
drivers/gles3/rasterizer_scene_gles3.h Show resolved Hide resolved
drivers/gles3/shaders/scene.glsl Outdated Show resolved Hide resolved
servers/rendering/renderer_scene_render.cpp Outdated Show resolved Hide resolved
@scriptsengineer scriptsengineer force-pushed the distance-fog branch 2 times, most recently from 6adfd1a to f38fbf4 Compare February 17, 2024 14:00
@scriptsengineer
Copy link
Author

Tested locally on Linux, I get these errors in the terminal after merging this PR, when opening the project manager with OpenGL:

Fixed

@scriptsengineer
Copy link
Author

@clayjohn There is a problem with compatibility mode, i need help with this
Before this PR
image
After this PR
image

@HybridEidolon
Copy link
Contributor

@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.

@scriptsengineer
Copy link
Author

The uniforms in the UBO may be misaligned to std140 alignment in the compatibility shaders.

Fixed

@akien-mga akien-mga merged commit e8755b3 into godotengine:master Feb 18, 2024
15 checks passed
@akien-mga
Copy link
Member

Thanks @HybridEidolon and @scriptsengineer for this amazing joint first contribution! 🎉 🥇

@ghost
Copy link

ghost commented Mar 27, 2024

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?

@clayjohn
Copy link
Member

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?

Yes, using the atmospheric perspective setting

Or a way to get a sky box (or I suppose any other object) excluded from the depth fog?

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

@Calinou
Copy link
Member

Calinou commented Mar 28, 2024

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.

Is there a way to get the depth fog to blend with a sky box?

Fog aerial perspective can be used for this, but it'll be much more effective if #89995 is merged.

@ghost
Copy link

ghost commented Apr 4, 2024

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?

Yes, using the atmospheric perspective setting

I've attached three images using all three types of sky materials. Are these the expected results?

image
image
image

@ghost
Copy link

ghost commented Apr 4, 2024

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.

@Calinou
Copy link
Member

Calinou commented Apr 4, 2024

I've attached three images using all three types of sky materials. Are these the expected results?

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).

I'd like to suggest that "atmospheric perspective" will be better for users than "aerial perspective". The terms seem to be interchangeable.

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.

@ghost
Copy link

ghost commented Apr 4, 2024

Thanks for the response. The new depth fog is a good change.

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).

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 ?

@Calinou
Copy link
Member

Calinou commented Apr 9, 2024

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).

@ghost
Copy link

ghost commented Apr 10, 2024

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.

After some experimentation I'm getting great results. Thanks for all the effort that went into this.

If you set Fog > Sky Affect below 1.0, the appearance of aerial perspective fog won't change (I just tested).

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.

image

@ghost
Copy link

ghost commented Apr 10, 2024

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.

@RichardEllicott
Copy link

RichardEllicott commented Jul 12, 2024

anyone know if this is this gonna be in Godot 4.3?

@Calinou
Copy link
Member

Calinou commented Jul 12, 2024

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.

@RichardEllicott
Copy link

RichardEllicott commented Jul 12, 2024

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

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.

None yet

8 participants