godotengine / godot

Godot Engine – Multi-platform 2D and 3D game engine
https://godotengine.org
MIT License
89.03k stars 20.19k forks source link

4.2 World Environment Glow Effect - Makes Everything Glow #86098

Open BQKdev opened 9 months ago

BQKdev commented 9 months ago

Tested versions

4.2 Stable

System information

WIndows 10 Godot Steam Version - Mobile Mode

Issue description

In Godot 3x World Environment used to be simple on choosing what object should glow, you just had to add the World Environment change Background to Canvas , and Enable Glow.

But in Godot 4x The World Environment glow makes everything to glow even the things they aren't supposed to glow.

Example Imagen

image

Steps to reproduce

  1. Go to Project -> Project Settings, Scroll into section of Rendering -> Viewport -> Enable HDR 2D
  2. Add a Node
  3. In the Node, add two Sprite2d node and set one to RAW(1.5,1.5,1.5)
  4. Add a WorldEnvironment+Environment, set background to Canvas, enable Glow
  5. Change Glow Mode to Screen
  6. See both of them are glowing

Minimal reproduction project (MRP)

Godot Test FIles.zip

clayjohn commented 9 months ago

Does the glow go away if you set the "HDR threshold to 0.5"?

To me this looks like an issue with the way that we scale colors in the Mobile renderer.

The framebuffer format used by the mobile renderer only supports colors between 0 and 1. But we want to allow for a wider range of colors (for things like Glow). So when we draw things we multiply their color value by 0.5, so we have an effective range of 0-2. Then when we tonemap the scene (and apply glow etc.) we multiply the color value by 2.

I have a feeling that color values from the mobile renderer are not getting that 0.5 scaling, so they are twice as bright as they should be. So a color of (1, 1, 1), ends up being treated like its (2, 2, 2).

BQKdev commented 9 months ago

Does the glow go away if you set the "HDR threshold to 0.5"?

To me this looks like an issue with the way that we scale colors in the Mobile renderer.

The framebuffer format used by the mobile renderer only supports colors between 0 and 1. But we want to allow for a wider range of colors (for things like Glow). So when we draw things we multiply their color value by 0.5, so we have an effective range of 0-2. Then when we tonemap the scene (and apply glow etc.) we multiply the color value by 2.

I have a feeling that color values from the mobile renderer are not getting that 0.5 scaling, so they are twice as bright as they should be. So a color of (1, 1, 1), ends up being treated like its (2, 2, 2).

I checked if this method work, and it did when i changed the Sprite2D that didn't glow to Modulate RAW(0.9, 0.9, 0.9).

Just one little problem, this would also mean if we have Glow on a scene, Then we would have to change everything to a lower value of color, And i sometimes could be tedious if you have a lot of objects on a scene.

Godot_Editor_glow_problem

clayjohn commented 9 months ago

Just one little problem, this would also mean if we have Glow on a scene, Then we would have to change everything to a lower value of color, And i sometimes could be tedious if you have a lot of objects on a scene.

Oh no, I'm just asking to help narrow down the bug. I'm not suggesting that you change your actual scene at all. There is a bug here that needs to be fixed.

clayjohn commented 9 months ago

If my theory is correct, then this will be fixed by https://github.com/godotengine/godot/pull/84169

BQKdev commented 9 months ago

I wanted to fix the problem in the mean time, So i created a script that anyone can add to their scene, it helps changing the Nodes modulates from (1, 1, 1) to a lower value (0.9, 0.9, 0.9). this is just a little fix in the mean time hope it helps. func _ready(): for node in self.get_children(): if node.name == "WorldEnvironment": continue if node.modulate == Color(1, 1, 1): node.modulate = Color(0.9, 0.9, 0.9)

But I still wander if it would be fixed.

Godot_Editor_glow_problem

BQKdev commented 9 months ago

If my theory is correct, then this will be fixed by #84169

What version would be fixed though, Just want to know😅

clayjohn commented 9 months ago

If my theory is correct, then this will be fixed by #84169

What version would be fixed though, Just want to know😅

~4.3 and 4.2.2 (or 4.2.3, depending on when it gets merged)~

I just checked, #84169 isn't enough to fix this issue :disappointed:

clayjohn commented 9 months ago

Okay, after a bit of investigation, here is a diff that fixes the issue (but its not a correct fix):

diff --git a/servers/rendering/renderer_rd/effects/copy_effects.cpp b/servers/rendering/renderer_rd/effects/copy_effects.cpp
index c9f0008998..7a6ba0dbd3 100644
--- a/servers/rendering/renderer_rd/effects/copy_effects.cpp
+++ b/servers/rendering/renderer_rd/effects/copy_effects.cpp
@@ -561,8 +561,8 @@ void CopyEffects::copy_to_fb_rect(RID p_source_rd_texture, RID p_dest_framebuffe
                // Used for copying to a linear buffer. In the mobile renderer we divide the contents of the linear buffer
                // to allow for a wider effective range.
                copy_to_fb.push_constant.flags |= COPY_TO_FB_FLAG_LINEAR;
-               copy_to_fb.push_constant.luminance_multiplier = prefer_raster_effects ? 2.0 : 1.0;
        }
+       copy_to_fb.push_constant.luminance_multiplier = prefer_raster_effects ? 2.0 : 1.0;

        // setup our uniforms
        RID default_sampler = material_storage->sampler_rd_get_default(RS::CANVAS_ITEM_TEXTURE_FILTER_LINEAR, RS::CANVAS_ITEM_TEXTURE_REPEAT_DISABLED);
diff --git a/servers/rendering/renderer_rd/storage_rd/texture_storage.cpp b/servers/rendering/renderer_rd/storage_rd/texture_storage.cpp
index 307cbd703a..a8880ed468 100644
--- a/servers/rendering/renderer_rd/storage_rd/texture_storage.cpp
+++ b/servers/rendering/renderer_rd/storage_rd/texture_storage.cpp
@@ -1272,8 +1272,7 @@ Ref<Image> TextureStorage::texture_2d_get(RID p_texture) const {
        ERR_FAIL_COND_V(data.size() == 0, Ref<Image>());
        Ref<Image> image;

-       // Expand RGB10_A2 into RGBAH. This is needed for capturing viewport data
-       // when using the mobile renderer with HDR mode on.
+       // Expand RGB10_A2 into RGBAH.
        if (tex->rd_format == RD::DATA_FORMAT_A2B10G10R10_UNORM_PACK32) {
                Vector<uint8_t> new_data;
                new_data.resize(data.size() * 2);
@@ -3051,7 +3050,7 @@ void TextureStorage::_update_render_target(RenderTarget *rt) {
                return;
        }
        if (rt->use_hdr) {
-               rt->color_format = RendererSceneRenderRD::get_singleton()->_render_buffers_get_color_format();
+               rt->color_format = RD::DATA_FORMAT_R16G16B16A16_SFLOAT;
                rt->color_format_srgb = rt->color_format;
                rt->image_format = rt->is_transparent ? Image::FORMAT_RGBAH : Image::FORMAT_RGBH;
        } else {

I need to discuss with @BastiaanOlij to decide on a proper fix

BQKdev commented 9 months ago

I actually confuse, Do i make a Godot Module and pasted on the script that need's to be in? but what file do i change it to?😅

Edited: It's this script for the rendering file?

clayjohn commented 9 months ago

I actually confuse, Do i make a Godot Module and pasted on the script that need's to be in? but what file do i change it to?😅

Don't worry about the code I posted. Its more for us to work off of as we try to fix the bug.

BastiaanOlij commented 9 months ago

I would definitely not go for using DATA_FORMAT_R16G16B16A16_SFLOAT, we have to stick to 32bit color formats on mobile or we loose a lot of performance. At the least it doubles bandwidth, but on GPU architectures that apply compression we can triple or quadruple bandwidth usage. I guess though that for pure 2D we could take that hit, is there a way we can do this if we must only if there are no 3D elements being rendererd?

If we combine 2D and 3D rendering, and we do not reverse the luminance multiplier until we resolve the HDR buffer to screen, then the 2D rendering should apply the luminance multiplier as well. But I believe you mentioned that leads to banding Clay?

clayjohn commented 9 months ago

I would definitely not go for using DATA_FORMAT_R16G16B16A16_SFLOAT, we have to stick to 32bit color formats on mobile or we loose a lot of performance. At the least it doubles bandwidth, but on GPU architectures that apply compression we can triple or quadruple bandwidth usage. I guess though that for pure 2D we could take that hit, is there a way we can do this if we must only if there are no 3D elements being rendererd?

Keep in mind, this problem only arises when using HDR rendering and using canvas background/glow (which necessarily require 3D rendering to take place). For pure 2D rendering, we can get by without a floating point framebuffer format.

Users who are enabling HDR have already given up on supporting mobile devices as they are either using glow, or are targeting HDR screens (in which case they need an HDR float format).

IMO it makes sense to keep our non HDR buffer 32-bit, but once users explicitly ask for HDR, then we should give them a floating point buffer that can actually use HDR values. Right now, on the mobile renderer when users enable HDR, they still have a normalized value buffer, just with slightly more precision.

If we combine 2D and 3D rendering, and we do not reverse the luminance multiplier until we resolve the HDR buffer to screen, then the 2D rendering should apply the luminance multiplier as well. But I believe you mentioned that leads to banding Clay?

It would lead to increased banding yes. But it wouldn't be much different then the banding that already exists with HDR disabled.

Our two options for solutions are:

  1. Apply the luminance multiplier to 2D content that is rendered before the 3D pass (after the 3D pass we will have scaled our content back to 0-1, so we don't want to apply the multiplier anymore)
  2. Use a floating point color format for the 2D buffer.
BastiaanOlij commented 9 months ago

Users who are enabling HDR have already given up on supporting mobile devices as they are either using glow, or are targeting HDR screens (in which case they need an HDR float format).

@clayjohn then why are they using the mobile renderer? If you're using the 64bit HDR formats than there isn't much, if anything, to be won using the mobile renderer over using the desktop renderer with all other features turned off?

Calinou commented 9 months ago

There's been some demand for supporting 16-bit floating-point rendering for mobile in 3D as well, since it allows debanding to actually do its job correctly.

That said, I agree this should be an opt-in setting and not something enabled by default automatically when using the Mobile rendering method.

clayjohn commented 9 months ago

Users who are enabling HDR have already given up on supporting mobile devices as they are either using glow, or are targeting HDR screens (in which case they need an HDR float format).

@clayjohn then why are they using the mobile renderer? If you're using the 64bit HDR formats than there isn't much, if anything, to be won using the mobile renderer over using the desktop renderer with all other features turned off?

That's a good point. Glow will be slightly faster for mobile devices if using the mobile backend (as it doesn't use compute), but the quality will be much higher when using the Forward+ backend.

I guess the only option is to apply the luminance multiplier to 2D renderer as well, which will be a total pain

jbromberg commented 6 months ago

Does this mean that selective glow in 3D using the mobile renderer isn't currently functional? It seems the way to make certain objects glow is to boost their color values to be HDR beyond 1 while leaving non glowing objects in the standard 0-1 range. I'm struggling to get this to work in my project using the mobile renderer and thinking this may be why.

Calinou commented 6 months ago

Does this mean that selective glow in 3D using the mobile renderer isn't currently functional? It seems the way to make certain objects glow is to boost their color values to be HDR beyond 1 while leaving non glowing objects in the standard 0-1 range.

The Mobile renderer's HDR luminance can go up to 2.0, so you can still do that in 3D. However, as mentioned above, this can't be done in 2D.

In 4.3, Glow was implemented in the Compatibility rendering method but it has the same HDR range limitations as the Mobile rendering method (if not more limited for 3D, in fact).

jbromberg commented 6 months ago

Could just be my implementation, but I haven't been able to get it to successfully ignore other colors in the standard 0-1 range. If I have a white box that I don't want to glow and another that I do want to glow, I haven't been able to get the glow to not show up on the part of the white box where the directional light hits. I assume it's due to the clamping of luminance or something like that.

I've spent a while playing with the HDR threshold on the glow and if there's a white material in the scene you don't want to glow it will still glow when hit with the light. The only way I've gotten it to not glow is having the glow threshold so high that nothing else in the scene glows regardless of its HDR color.

zfrhv commented 2 months ago

Is there any working/planned solution other than setting all objects modulate to 0.9?

zfrhv commented 2 months ago

oh nvm I had to enable HDR 2d in the project settings: https://www.reddit.com/r/godot/comments/189vclp/comment/kbxn2mn/?share_id=MrGCV1MKeoQMXl3du-cv-&utm_content=2&utm_medium=android_app&utm_name=androidcss&utm_source=share&utm_term=1