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

Add screenshot support for eframe web #5438

Merged
merged 22 commits into from
Dec 12, 2024
Merged

Conversation

lucasmerlin
Copy link
Collaborator

@lucasmerlin lucasmerlin commented Dec 5, 2024

This implements web support for taking screenshots in an eframe app (and adds a nice demo).
It also updates the native screenshot implementation to work with the wgpu gl backend.

The wgpu implementation is quite different than the native one because we can't block to wait for the screenshot result, so instead I use a channel to pass the result to a future frame asynchronously.

Bildschirmaufnahme.2024-12-05.um.15.27.52.mov

Copy link

github-actions bot commented Dec 5, 2024

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

@lucasmerlin lucasmerlin added feature New feature or request eframe Relates to epi and eframe labels Dec 5, 2024
@lucasmerlin lucasmerlin force-pushed the lucas/eframe-web-screenshots branch from 9abe20e to 278616b Compare December 5, 2024 15:02
@lucasmerlin lucasmerlin marked this pull request as ready for review December 5, 2024 15:28
@lucasmerlin lucasmerlin requested a review from Wumpf as a code owner December 5, 2024 15:28
@lucasmerlin lucasmerlin marked this pull request as draft December 5, 2024 15:51
@lucasmerlin
Copy link
Collaborator Author

lucasmerlin commented Dec 5, 2024

I put this back to draft because surface.get_capabilities(&render_state.adapter).usages.contains(wgpu::TextureUsages::COPY_DST); doesn't really work as expected, @Wumpf will open a wgpu issue about it.

I'll need to update this to render the texture to the surface via a triangle instead of using copy_texture_to_texture, so we can properly support all platforms. I'll also do this for the native implementation, since there screenshots also don't work with the wgpu gl backend.

@Wumpf
Copy link
Collaborator

Wumpf commented Dec 5, 2024

@lucasmerlin lucasmerlin force-pushed the lucas/eframe-web-screenshots branch from 40f0999 to 289559a Compare December 12, 2024 11:54
@lucasmerlin lucasmerlin marked this pull request as ready for review December 12, 2024 12:20
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.

nice!
PR description is now outdated.
Comments are mostly nits about styling and efficiency (if anything the encoder one bugs me most actually) but what I'd like to insist on is refactoring/removing the blocking path - this code dupl doesn't really make sense and I don't think we should have a blocking path in the first place.

crates/egui-wgpu/src/blit.wgsl Outdated Show resolved Hide resolved
crates/egui-wgpu/src/blit.wgsl Outdated Show resolved Hide resolved
crates/egui-wgpu/src/capture.rs Outdated Show resolved Hide resolved
crates/egui-wgpu/src/capture.rs Outdated Show resolved Hide resolved
crates/egui-wgpu/src/capture.rs Outdated Show resolved Hide resolved
crates/egui-wgpu/src/winit.rs Outdated Show resolved Hide resolved
crates/egui-wgpu/src/capture.rs Outdated Show resolved Hide resolved
Comment on lines 212 to 213
wgpu::TextureFormat::Rgba8Unorm => [0, 1, 2, 3],
wgpu::TextureFormat::Bgra8Unorm => [2, 1, 0, 3],
Copy link
Collaborator

Choose a reason for hiding this comment

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

as noted above, it would be faster & easier to force the temp texture to be rgba

crates/egui-wgpu/src/capture.rs Outdated Show resolved Hide resolved
crates/egui-wgpu/src/winit.rs Outdated Show resolved Hide resolved
@lucasmerlin
Copy link
Collaborator Author

@Wumpf I've implemented everything you asked for (except for the texture_format changes, as discussed)

@lucasmerlin lucasmerlin requested a review from Wumpf December 12, 2024 16:36
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.

thank you so much for addressing all those! 🚢

@Wumpf Wumpf merged commit 6c1d695 into master Dec 12, 2024
46 checks passed
@Wumpf Wumpf deleted the lucas/eframe-web-screenshots branch December 12, 2024 18:17
Wumpf pushed a commit to rerun-io/rerun that referenced this pull request Dec 17, 2024
### Related
* Part of #8264
* Using emilk/egui#5438

Still missing: copy screenshot to clipboard

Also fixes a crash when screenshotting graph views.

### Usage

![image](https://github.com/user-attachments/assets/88948342-8279-4bf6-9bc6-13722229dc44)

### Result
![Node-link
diagram](https://github.com/user-attachments/assets/b2ff8dd5-ae55-436a-9cd2-263eeb72a0df)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
eframe Relates to epi and eframe feature New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Implement ViewportCommand::Screenshot on web
2 participants