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

Implement glow/bloom on compatibility renderer #87360

Merged
merged 1 commit into from
Feb 20, 2024

Conversation

BastiaanOlij
Copy link
Contributor

@BastiaanOlij BastiaanOlij commented Jan 19, 2024

This is a fairly straight forward and basic implementation of glow. For now we've left out some of the bells and whistles to keep this performant on mobile devices.

This PR also enabled/abuses the 2D HDR setting on viewports to cause the viewport to use an RGBA16F buffer allowing for high dynamic range for both 2D and 3D output. Colors are still output in sRGB color space so it's more an approximation but it works.

The glow works using "Dual filtering" to blur the source image which is more limited to what we're doing in the Vulkan renderer (but much more bandwidth efficient). Right now we're also using simple blending to apply the glow to the final image, functional and fast, but not as pretty as what the Vulkan render does.

image

See Marius Bjørge presentation at Siggraph 2015 "Bandwidth-Efficient Rendering" for more info on the blur effect being used.

Without HDR enabled we need to apply our luminance multiplier trick that we also use on Vulkan Mobile (but now in sRGB space which in theory should have less banding issues) where we use an RGB10A2 buffer.

Note: Glow on a viewport with a transparent background only works when HDR is enabled. We can not use the luminance multiplier trick in this case due to using RGBA8 buffers.
If glow is used with stereo rendering, this disables the benefits of foveated rendering as we are using an intermediate buffer.

@BastiaanOlij
Copy link
Contributor Author

BastiaanOlij commented Jan 25, 2024

This is now working both in HDR and normal mode. In normal mode we render to RGB10A2 as before, but if glow is enabled we apply a multiplier. Needs testing to see if this leads to unwanted banding. We could play with the multiplier to see if we can find a better balance if needed.

Also still need to test in stereo Stereo seems to work as well :)

@BastiaanOlij
Copy link
Contributor Author

@clayjohn would be good if you could have a first look at this and give some feedback on what else we should do here.

@BastiaanOlij BastiaanOlij force-pushed the gles_glow branch 2 times, most recently from 49e1561 to 3016bbd Compare January 25, 2024 09:13
@clayjohn
Copy link
Member

We discussed hiding the "level" properties and the "normalized" property.

You can hide them from the editor here (note this only hides it from the editor, it doesn't change saving/loading):

if (p_property.name == "glow_intensity" && glow_blend_mode == GLOW_BLEND_MODE_MIX) {
p_property.usage = PROPERTY_USAGE_NO_EDITOR;
}
if (p_property.name == "glow_mix" && glow_blend_mode != GLOW_BLEND_MODE_MIX) {
p_property.usage = PROPERTY_USAGE_NO_EDITOR;
}

Then you just have to remember to call notify_property_list_changed() here:

void Environment::set_glow_level(int p_level, float p_intensity) {
ERR_FAIL_INDEX(p_level, RS::MAX_GLOW_LEVELS);
glow_levels.write[p_level] = p_intensity;
_update_glow();
}
float Environment::get_glow_level(int p_level) const {
ERR_FAIL_INDEX_V(p_level, RS::MAX_GLOW_LEVELS, 0.0);
return glow_levels[p_level];
}
void Environment::set_glow_normalized(bool p_normalized) {
glow_normalize_levels = p_normalized;
_update_glow();
}

@BastiaanOlij BastiaanOlij force-pushed the gles_glow branch 2 times, most recently from c8227d1 to 96f8ca3 Compare January 30, 2024 05:13
@BastiaanOlij
Copy link
Contributor Author

We discussed hiding the "level" properties and the "normalized" property.

I've hidden all glow properties we currently don't support. There might be one or two that we can add support for.
Clang format was being a bit annoying though.

I wasn't sure if notify_property_list_changed was required, it's called on set_glow_enabled which is the only thing that impacts visibility here.

@BastiaanOlij BastiaanOlij marked this pull request as ready for review January 30, 2024 05:18
@BastiaanOlij BastiaanOlij requested a review from a team as a code owner January 30, 2024 05:18
Copy link
Member

@Calinou Calinou left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tested locally, it works as expected in 3D.

Testing project: test_glow_compatibility.zip

One issue however is that since the preview environment has glow enabled by default, it looks very focused and very strong in Compatibility:

Forward+ Mobile Compatibility
Screenshot_20240130_174402 Screenshot_20240130_174416 Screenshot_20240130_174349

To avoid this, I suggest disabling glow in the preview environment (at least when Compatibility is used), which will also help improve performance on integrated graphics.

In 2D, setting background mode to Canvas then enabling glow results in a black viewport:

Glow disabled Glow enabled
image image

This occurs regardless of whether HDR 2D is enabled in the project settings.

Also, I'm surprised only the Intensity setting is available. Shouldn't there be Bloom and HDR Threshold properties at least, so you get more control over where glow appears?

@BastiaanOlij
Copy link
Contributor Author

@Calinou I added support for bloom and the thresholds, turned out to be easier than I thought, that makes it work pretty close to the original.

I'll have a look at the canvas mode, I think that might be a result of 2D stuff not being copied to the new intermediate buffer.

@BastiaanOlij
Copy link
Contributor Author

Ok, fixed the canvas mode as well. Do note that 2D rendering caps the values so glow requires some bloom or a lower hdr threshold. Hopefully that is ok enough.

drivers/gles3/shaders/effects/glow.glsl Outdated Show resolved Hide resolved
@DanielSnd
Copy link
Contributor

I was testing this PR and disabling Depth Prepass in project settings seems to completely break rendering for me. Can reproduce the issue in @Calinou 's test project.

driver/depth_prepass/enable=false

@BastiaanOlij
Copy link
Contributor Author

I was testing this PR and disabling Depth Prepass in project settings seems to completely break rendering for me. Can reproduce the issue in @Calinou 's test project.

driver/depth_prepass/enable=false

Good catch Daniel. Actually possibly not broken by this PR but already broken, writing to depth was disabled when clearing the depth buffer. Moved some code forward to fix.

@BastiaanOlij BastiaanOlij force-pushed the gles_glow branch 2 times, most recently from 31018d5 to 092d49e Compare February 15, 2024 12:12
Copy link
Contributor

@DanielSnd DanielSnd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found some issues when trying to run with this PR on android, it failed the shader compilation.

E 0:00:14:0887   _display_error_with_code: PostShaderGLES3: Fragment shader compilation failed:
ERROR: 0:250: 'textureLod' : no matching overloaded function found 
ERROR: 0:250: '=' :  cannot convert from 'const float' to '4-component vector of float'
ERROR: 0:251: 'textureLod' : no matching overloaded function found 
ERROR: 0:252: 'textureLod' : no matching overloaded function found 
ERROR: 0:253: 'textureLod' : no matching overloaded function found 
ERROR: 0:254: 'textureLod' : no matching overloaded function found 
ERROR: 0:255: 'textureLod' : no matching overloaded function found 
ERROR: 0:256: 'textureLod' : no matching overloaded function found 
ERROR: 0:257: 'textureLod' : no matching overloaded function found 
ERROR: 9 compilation errors.  No code generated.

I found out that it's because on the textureLod calls it's using 0 instead of 0.0 so it's being interpreted as an integer. Changing it to 0.0 in all the textureLod calls fixed the issue.

vec4 get_glow_color(vec2 uv) {
vec2 half_pixel = pixel_size * 0.5;

vec4 color = textureLod(glow_color, uv + vec2(-half_pixel.x * 2.0, 0.0), 0);
Copy link
Contributor

@DanielSnd DanielSnd Feb 15, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated it becomes:

vec4 color = textureLod(glow_color, uv + vec2(-half_pixel.x * 2.0, 0.0), 0.0);
	color += textureLod(glow_color, uv + vec2(-half_pixel.x, half_pixel.y), 0.0) * 2.0;
	color += textureLod(glow_color, uv + vec2(0.0, half_pixel.y * 2.0), 0.0);
	color += textureLod(glow_color, uv + vec2(half_pixel.x, half_pixel.y), 0.0) * 2.0;
	color += textureLod(glow_color, uv + vec2(half_pixel.x * 2.0, 0.0), 0.0);
	color += textureLod(glow_color, uv + vec2(half_pixel.x, -half_pixel.y), 0.0) * 2.0;
	color += textureLod(glow_color, uv + vec2(0.0, -half_pixel.y * 2.0), 0.0);
	color += textureLod(glow_color, uv + vec2(-half_pixel.x, -half_pixel.y), 0.0) * 2.0;

drivers/gles3/shaders/effects/glow.glsl Outdated Show resolved Hide resolved
@DanielSnd
Copy link
Contributor

Not sure if this is the right place to put this, but I've been trying to check how the glow would run on my quest 2. After the fixes I pointed out above I managed to get it to run, however if I have both Glow and the MSAA enabled (in compatibility mode with the merged PR that introduces MSAA to compatibility mode) the game freezes on load and I have to force-restart the device.

If I disable MSAA the game starts just fine and I can see the glow effect, no errors, no problems.

If I disable glow and leave MSAA enabled the game starts just fine and I can see the MSAA effect, no glow but no errors no problems.

Having both enabled at the same time causes the freeze on load. (Black screen frozen)

I also tried to cheese the device by starting with MSAA on and glow off, and added a timer to a GDScript that enabled glow after 5 seconds. It still froze the device but in a different way. It took me to passthrough, and didn't recognize controllers or anything. Pressing power twice took me back to the game, I could look around for a moment, I didn't see any glow, but then it took me right back to passthrough.

@BastiaanOlij
Copy link
Contributor Author

Found a new issue with it, if there's a shader that's has the render modes cull_front and also depth_draw_never in the scene it either freezes the scene view or makes it really slow (depending on the complexity of the mesh I believe).

Haven't been able to reproduce this so I think there is more going on in this example. Could use an MRP for this

@BastiaanOlij BastiaanOlij force-pushed the gles_glow branch 2 times, most recently from ff1c900 to 5bc9e72 Compare February 16, 2024 02:59
@BastiaanOlij
Copy link
Contributor Author

Having both enabled at the same time causes the freeze on load. (Black screen frozen)

I'm able to reproduce this but it has me stumped. There is nothing in the error log, renderdoc just shows that we're skipping rendering all together. Android is such a PITA to debug.

When using glow we're rendering to an intermediate buffer. When using MSAA on devices that have this capability (Quest does, PC does not), we're using a special MSAA GLES extension that does MSAA in tile memory and then directly resolves and writes to texture. So we're using that here on the intermediate buffer.
My guess is that this is working correctly with our normal framebuffer, but failing on the intermediate buffer.

@DanielSnd
Copy link
Contributor

DanielSnd commented Feb 16, 2024

Found a new issue with it, if there's a shader that's has the render modes cull_front and also depth_draw_never in the scene it either freezes the scene view or makes it really slow (depending on the complexity of the mesh I believe).

Haven't been able to reproduce this so I think there is more going on in this example. Could use an MRP for this

Makes sense, I tested on the new push and it's still happening, MRP attached:
ReproGlowIssue.zip

- Edit -

It has now been fixed by the new commit :)

@BastiaanOlij BastiaanOlij force-pushed the gles_glow branch 2 times, most recently from ae317b3 to 364e051 Compare February 19, 2024 02:21
@BastiaanOlij
Copy link
Contributor Author

I found out a big problem with our code, and this is probably not even introduced by glow but it came to the surface, is our handling of global state in OpenGL. We often leave stuff as is after a process finishes with the assumption that the next process doesn't mind.

One such thing is leaving shaders bound, we are finding out that more and more functions that seemingly do nothing with shaders, can still fail if the current bound shader does not match the current framebuffer setup. In our case here I got a bunch of error spam from a glClear in the canvas renderer, because of the last shader used during 3D rendering.

The other issue had to do with setting and retaining the last culling state and the last depth buffer state, both causing issues with the execution of our glow shader causing the UI to seemingly freeze (it was just not processing the glow shader).

I've thus did a LOT of cleanup on the way we handle global state in the 3D rendering part. There is more here to do but I've done the basics. I don't like having this logic where it is however, it should be more central. One thing @clayjohn and I have been discussing in the past is to rename our config class to an more general purpose opengl(_wrapper) class. This class is already a mix of OpenGL status info and additional function handling, moving this into the drivers/gles3 folder and implementing it to hold and modify global state, and do stuff such as tracking resources, seems like a good idea. Out of scope for this PR however.

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.

I think there are a few more improvements we can make to this. But seeing how long the process has been, I'm happy to merge this for now and then make the improvements in follow up PRs.

Particularly, we should make follow up PRs to:

  1. Improve the luminance calculation. Using the current method means that blue objects don't glow
  2. start with a 1/4 res downsample instead of 1/2 res. This will expand the size of the glow a bit, but it will also reduce the cost of glow.

@clayjohn clayjohn modified the milestones: 4.x, 4.3 Feb 20, 2024
@BastiaanOlij
Copy link
Contributor Author

  1. Improve the luminance calculation. Using the current method means that blue objects don't glow

cc @Ansraer :)

  1. start with a 1/4 res downsample instead of 1/2 res. This will expand the size of the glow a bit, but it will also reduce the cost of glow.

I think this should be a setting, and we'll need to see how we can modify the downsample and upsample as the logic from the original dual filtering spec is purely 1/2 res downstep/upstep.
I also think that in this case we should have the final upstep go to a glow buffer, right now I'm doing the final upstep as part of writing into the render buffer to save an extra step. But I don't think we can efficiently do that especially when combined with scaling.

It would definitely be nice to do these as follow up PRs.

@akien-mga akien-mga merged commit 652438a into godotengine:master Feb 20, 2024
16 checks passed
@akien-mga
Copy link
Member

Thanks! Amazing work that should make a lot of happy users 🎉

@theromis
Copy link
Contributor

theromis commented Mar 11, 2024

@BastiaanOlij thank you for great improvement, trying to use it with my project, I have unshaded shaders:

shader_type spatial;
render_mode unshaded;

uniform float glow_power : hint_range(1.0, 100.0) = 30.0;
uniform vec3 color : source_color = vec3(0.0, 0.686, 0.757);

void fragment() {
    ALBEDO = color * glow_power;
}

simple and stupid, but it works with mobile and forward+, but doesn't work with compatibility in current main build(shaded emission works fine), is it possible to make it work or maybe some workaround?

@BastiaanOlij
Copy link
Contributor Author

@theromis worth raising an issue for so it's not overlooked. In theory this should work, I don't see anything that stands out in the scene shader that would limit the output.

@theromis
Copy link
Contributor

@BastiaanOlij finally found env setup to make this shader work, sorry, false alarm, palette looks different but at least it works.

Screenshot 2024-03-11 at 10 47 42

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.

OpenGL: Environment glow not implemented yet
7 participants