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

Fix panic on wgpu GL backend due to new screenshot capability #3078

Merged
merged 8 commits into from
Jun 15, 2023

Conversation

amfaber
Copy link
Contributor

@amfaber amfaber commented Jun 14, 2023

This is a first-pass triage for #3068. The fix involves simply disabling the screen shot feature when using the GL backend with wgpu. If a screenshot is requested on this backend, None will be provided and an error is logged out. The docs have been updated with an small comment on how the feature isn't available with the GL backend.

The reason that the feature is incompatible with the GL backend is that this backend doesn't allow the surface to have the TextureUsages::COPY_DST flag, and so the approach of rendering to a texture and subsequently copying the result to the surface and a buffer for extracting to the CPU is fundamentally incompatible.

Perhaps @Wumpf can help with ideas on how to circumvent this?

Copy link
Collaborator

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

Great catch, makes sense!

But the culprit isn't really GL backend not supporting a thing, but that that we blindly assume the COPY_DST usage is supported. This can happen on any backend in theory, so therefore we should check the availability of that usage flag and then draw the right conclusion.... only that I noticed now that this is not possible right now, I opened an issue here gfx-rs/wgpu#3869

Even better would be ofc to just not use texture copies here, but instead do screen triangle pass to render the final texture, but let's not get into additional complexities right now :)

So given the situation I think we can move on with this, but I'd like some small improvements in here, see comments :)

crates/eframe/src/epi/mod.rs Outdated Show resolved Hide resolved
crates/egui-wgpu/src/winit.rs Outdated Show resolved Hide resolved
crates/egui-wgpu/src/winit.rs Outdated Show resolved Hide resolved
@Wumpf Wumpf added bug Something is broken egui-wgpu labels Jun 14, 2023
Copy link
Collaborator

@Wumpf Wumpf left a comment

Choose a reason for hiding this comment

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

ci is unhappy, otherwise good to go imho

@amfaber
Copy link
Contributor Author

amfaber commented Jun 14, 2023

Thanks for the input! As I am still getting into graphics programming, I am especially interested in the benefits of doing a triangle screen (re) render rather than a copy: Are there any benefits other than not requiring COPY_DST on the surface (though this benefit is clearly sufficient on its own)? In my mind I would have thought that a straight copy would be one of the most efficient operations that we could perform. Really looking to learn ^^

@Wumpf
Copy link
Collaborator

Wumpf commented Jun 15, 2023

It should generally be way better to do the copy via the copy texture operation, but with these things it's very much dependent on the driver and what other loads are in flight. But yes the only actual reason here would be compatibility

@Wumpf Wumpf merged commit 9478e50 into emilk:master Jun 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is broken egui-wgpu
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants