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

Extend WgpuSetup, egui_kittest now prefers software rasterizers for testing #5506

Merged
merged 12 commits into from
Jan 8, 2025

Conversation

Wumpf
Copy link
Collaborator

@Wumpf Wumpf commented Dec 20, 2024

WgpuSetup is now a lot more powerful:

  • fully configure instance
  • manual adapter selection on native

egui_kittest now uses this to prefer software rasterizers when possible.

Sidenote: request_adapter's force_fallback_adapter is not exposed since it will fail if there's no software rasterizer around (which is unfortunately the regular state of things on Mac). Since it also doesn't have any effect on WebGPU (unlike power preference!), I removed it again from WgpuSetup as it doesn't seem to be all that useful in general to me 🤷

TODO:

Copy link

Preview available at https://egui-pr-preview.github.io/pr/5506-configurable-test-renderer
Note that it might take a couple seconds for the update to show up after the preview_build workflow has completed.

@lucasmerlin
Copy link
Collaborator

I refactored how the snapshots are rendered in #5539, introducing a new TestRenderer trait and a WgpuTestRenderer.
The renderer can now be initialized when the Harness is created, meaning we can now create a Harness from an eframe::App, tell it to create a wgpu renderer. Then custom rendering relying on eframe::Frame::wgpu_render_state and PaintCallback will just work ✨

@Wumpf
Copy link
Collaborator Author

Wumpf commented Jan 2, 2025

My attempts to try out software rasterizing on Dx12 ran into a recent wgpu regression, took me quite a while

@Wumpf Wumpf force-pushed the configurable-test-renderer branch from db45bfa to d28dd8b Compare January 2, 2025 19:21
@Wumpf Wumpf changed the title Extend WgpuSetup, make egui_kittest wgpu setup configurable & prefer software rasterizers for testing Extend WgpuSetup, make egui_kittest now prefers software rasterizers for testing Jan 3, 2025
@Wumpf Wumpf force-pushed the configurable-test-renderer branch from b4c2d05 to a837994 Compare January 3, 2025 11:08
let backends = if let WgpuSetup::CreateNew(create_new) = &config.wgpu_setup {
create_new.instance_descriptor.backends
} else {
wgpu::Backends::all()
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm a lil bit uneasy about this actually. Requesting adapters for all backends when the user already settled on one has the potential to waste a lot of startup time. But I didn't want to break the list of adapters we provide.
Should reconsider this in the future.

@Wumpf Wumpf marked this pull request as ready for review January 8, 2025 10:11
@Wumpf Wumpf requested a review from lucasmerlin as a code owner January 8, 2025 10:11
@Wumpf Wumpf changed the title Extend WgpuSetup, make egui_kittest now prefers software rasterizers for testing Extend WgpuSetup, egui_kittest now prefers software rasterizers for testing Jan 8, 2025
@lucasmerlin
Copy link
Collaborator

I gave this a try on windows to see if the snapshot tests would work, they all seem fine, except for the rendering test, where the snapshot fails in the linear gradient section (but only at a scale of 1.0, for some reason):

Screen.Recording.2025-01-08.at.13.22.55.mov

I had to increase the threshold to 15.0 for the test to succeed. But this test is also failing on master, so it's not related to the software rasterizer.

Also, logging the AdapterInfo it is choosing vulkan over dx12, meaning it doesn't use the dx12 software rasterizer. I guess this would be fixed by gfx-rs/wgpu#6843?

[crates\egui-wgpu\src\lib.rs:238:9] adapter.get_info() = AdapterInfo {
    name: "AMD Radeon RX 5700 XT",
    vendor: 4098,
    device: 29471,
    device_type: DiscreteGpu,
    driver: "AMD proprietary driver",
    driver_info: "24.10.1 (AMD proprietary shader compiler)",
    backend: Vulkan,
}

@Wumpf
Copy link
Collaborator Author

Wumpf commented Jan 8, 2025

Also, logging the AdapterInfo it is choosing vulkan over dx12, meaning it doesn't use the dx12 software rasterizer. I guess this would be fixed by gfx-rs/wgpu#6843?

Yep. That's fixed there, tried that locally :)
Note that generally (not using that custom selection there) wgpu prefers Vulkan over DX12 because the DX12 implementation has some shortcomings today (none of which are super relevant for our purposes here :))

Copy link
Collaborator

@lucasmerlin lucasmerlin left a comment

Choose a reason for hiding this comment

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

Looks great!

@Wumpf Wumpf merged commit 443df84 into master Jan 8, 2025
46 checks passed
@Wumpf Wumpf deleted the configurable-test-renderer branch January 8, 2025 13:25
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.

2 participants