egui

egui copied to clipboard

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

@Wumpf @emilk for disabling dithering, would a feature on the crate do the job or should this be a runtime option?

I think a runtime feature flag is likely superior, as it is simpler, and lets the same user use egui_wgpu with different options depending on their render target, which may depend on the adapter capabilities.

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.

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.

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

I got a first draft of configurable dithering to work but I'm running out of time for today. I changed to state of the PR back to draft for now.

I updated and cleaned up the code and will mark the PR as 'Ready for review' again.

Testing performed

I tested the demo app using both wgpu and glow, with dithering enabled and disabled, on two different systems (Linux and Mac), as well as on the web using Chrome and Firefox. The dithering works as expected in all cases. While the results from wgpu and glow are not identical, they are very close.

There were some minute differences with dithering disabled as well. So I guess producing identical output between different backends isn't a requirement (and likely not a reasonable goal across systems anyways).

In addition to the demo app, I tested the proposed changes with one of my own applications.

Potential Optimizations

egui_glow::Painter::new, RenderState::create and Renderer::new, now all take quite a few arguments. If desired I can change them to take a struct (with a default) instead.

The implementation of the condition is different for wgpu and glow.

The glow implementation makes use of the glsl preprocessor using a define and if, similar to the existing code for SRGB_TEXTURES. This should result in zero shading cost when dithering is disabled.

The wgpu implementation uses a uniform instead. At least as far as I know wgsl doesn't have a preprocessor and I wasn't to keen to introduce some code to process the shader source. Instead it's using a uniform and a condition in the shader as suggested by @emilk. I'm not sure about the optimizations applied in wgpu, let alone it's various backends but this is likely very cheap (coherent branching) but not zero cost in either case.

Lastly it would be possible to make the amount if dithering configurable, for instance for using egui in an application with a bit depth lower than 8 bit. I don't think this is a very common use case though.

At least as far as I know wgsl doesn't have a preprocessor and I wasn't to keen to introduce some code to process the shader source.

there's no preprocessor, but this is a great usecase for a pipeline-overridable constant! :) There's an example in the wgpu 0.20.0 release notes. Unfortunately, they got only added in wgpu 0.20 and egui had to back out of 0.20 for the moment. So would be good to leave a todo note towards (and subsequent patch in) https://github.com/emilk/egui/pull/4560 (not a big deal anyways, this kind of static condition is extremely cheap)

there's no preprocessor, but this is a great usecase for a pipeline-overridable constant! :) There's an example in the wgpu 0.20.0 release notes. Unfortunately, they got only added in wgpu 0.20 and egui had to back out of 0.20 for the moment. So would be good to leave a todo note towards (and subsequent patch in) #4560 (not a big deal anyways, this kind of static condition is extremely cheap)

That sounds like a great solution. I'll need to do a bit more digging to figure out what it does in practice. If it's ok, I'll do a follow up on this PR once it is merged.

Sorry for the delayed response. Life happened.

I rebased the branch and applied the suggestions. I'm not sure if the more detailed descriptions should maybe go into the lower level crates (egui_glow, egui-wgpu) rather than eframe since users of those crates are more likely to care about the specifics.

Another consideration is api compatibility. I guess we don't want to promise that dithering true will always dither to 8 bit. So maybe _Currently_ dithering assumes an sRGB output and thus will apply noise to any input value that lies between would be a slightly softer comment.

Both are minor concerns though so I'd be happy to get the change merged as is.

Thank you for your patience @Wumpf @emilk :)

Re api compatibility: I think for the renderers to handle hdr output correctly some other stuff needs to happen so not too concerned, whoever looks into that can also do something about that. Worst case they have to break the variable name to make it more obvious