Introduce dithering to reduce banding #4497
Conversation
| @@ -87,5 +114,6 @@ fn fs_main_gamma_framebuffer(in: VertexOutput) -> @location(0) vec4<f32> { | |||
| let tex_linear = textureSample(r_tex_color, r_tex_sampler, in.tex_coord); | |||
| let tex_gamma = gamma_from_linear_rgba(tex_linear); | |||
| let out_color_gamma = in.color * tex_gamma; | |||
| return out_color_gamma; | |||
| let out_color_dithered = vec4<f32>(dither_interleaved(out_color_gamma.rgb, 256.0, in.position * r_locals.pixels_per_point), out_color_gamma.a); | |||
There was a problem hiding this comment.
Should have the same comments as the code above, and probably a linebreak
| let out_color_linear = linear_from_gamma_rgb(out_color_gamma.rgb); | ||
| // Dither the float color down to eight bits to reduce banding. | ||
| // This step is optional for egui backends. | ||
| let out_color_dithered = dither_interleaved(out_color_linear, 256.0, in.position); |
There was a problem hiding this comment.
Are you sure we should apply it to the linear color and not the gamma color? 🤔
There was a problem hiding this comment.
I agree, this should be done on the gamma color. It's important to know the context of when this fragment shader is used:
egui/crates/egui-wgpu/src/renderer.rs
Line 319 in ff7a383
gamma_from_linear_rgba.
Therefore, we have to make sure that all the meaningful value changes happen on the gamma color.
Note that it would be an entirely different story if we're outputing anything that isn't ending up on a wgpu::Surface that is targeting a srgb interpreting monitor/window. Meaning that all of this breaks down for HDR.
There was a problem hiding this comment.
I assumed that the output buffer was linear in that case dithering the linear color is what is wanted. If it's converted back to srgb before storage, it isn't optimal. I'll need to double check that.
There was a problem hiding this comment.
Just saw the comment be @Wumpf bellow, so you are right, this is off.
There was a problem hiding this comment.
great addition, however I think there should be an easy way to disable dithering:
- this is srgb dithering and doesn't make sense if someone wants to use an HDR target (e.g. WebGPU can also support P3 instead of srgb)
- there may be corner cases where a user doesn't actually want to dither everything drawn with egui indiscriminately as the added noise may destroy otherwise sharp corners
| let out_color_linear = linear_from_gamma_rgb(out_color_gamma.rgb); | ||
| // Dither the float color down to eight bits to reduce banding. | ||
| // This step is optional for egui backends. | ||
| let out_color_dithered = dither_interleaved(out_color_linear, 256.0, in.position); |
There was a problem hiding this comment.
I agree, this should be done on the gamma color. It's important to know the context of when this fragment shader is used:
egui/crates/egui-wgpu/src/renderer.rs
Line 319 in ff7a383
gamma_from_linear_rgba.
Therefore, we have to make sure that all the meaningful value changes happen on the gamma color.
Note that it would be an entirely different story if we're outputing anything that isn't ending up on a wgpu::Surface that is targeting a srgb interpreting monitor/window. Meaning that all of this breaks down for HDR.
| @@ -87,5 +114,6 @@ fn fs_main_gamma_framebuffer(in: VertexOutput) -> @location(0) vec4<f32> { | |||
| let tex_linear = textureSample(r_tex_color, r_tex_sampler, in.tex_coord); | |||
| let tex_gamma = gamma_from_linear_rgba(tex_linear); | |||
| let out_color_gamma = in.color * tex_gamma; | |||
| return out_color_gamma; | |||
| let out_color_dithered = vec4<f32>(dither_interleaved(out_color_gamma.rgb, 256.0, in.position * r_locals.pixels_per_point), out_color_gamma.a); | |||
There was a problem hiding this comment.
I don't get what * r_locals.pixels_per_point is about here. in.position is @builtin(position) which makes this raw pixel coordinates ranging from 0 to target size, see https://www.w3.org/TR/WGSL/#built-in-values-position. So scaling it with eguis points->pixel factor doesn't make any sense since we're starting with pixels anyways.
you also didn't apply this to either glsl or the fs_main_linear_framebuffer version. What's the intention here?
There was a problem hiding this comment.
Thanks for spotting this @Wumpf. I think I got confused between the input vertex coordinates and the position input to the fragment shader. Should have tested this better. Sorry for the oversight.
| @@ -132,9 +132,10 @@ impl ScreenDescriptor { | |||
| #[repr(C)] | |||
| struct UniformBuffer { | |||
| screen_size_in_points: [f32; 2], | |||
| pixels_per_point: f32, | |||
There was a problem hiding this comment.
as per other comment, I don't see why this is needed
|
I think a runtime feature flag is likely superior, as it is simpler, and lets the same user use Checking a bool in the shader should be fine |
|
I'll do a revision of the PR adding an option and fixing the coordinates in wgpu. I'm not certain where to add it yet. Adding it to native / web options in eframe, and then an implementation specific option in the specific backend (WgpuConfiguration for wgpu , EguiGlow::new for glow) seems about right. If you'd like it to be added another way, just let me know. |
That works for me, and that's probably the best way to do it, unless you want to mess with feature flags to reduce a tiny amount of runtime overhead |
This PR introduces dithering in the egui_glow and egui_wgpu backends to reduce banding artifacts.
It's based on the approach mentioned in #4493 with the small difference that the amount of noise is scaled down slightly to avoid dithering colors that can be represented exactly. This keeps flat surfaces clean.
Exaggerated dithering to show what is happening:
Subtle dithering as commited.
Closes #4493