Add screenshot api #7163

TheRawMeatball · 2023-01-11T19:25:55Z

Fixes #1207

Objective

Right now, it's impossible to capture a screenshot of the entire window without forking bevy. This is because

The swapchain texture never has the COPY_SRC usage
It can't be accessed without taking ownership of it
Taking ownership of it breaks a lot of stuff

Solution

Introduce a dedicated api for taking a screenshot of a given bevy window, and guarantee this screenshot will always match up with what gets put on the screen.

Changelog

Added the ScreenshotManager resource with two functions, take_screenshot and save_screenshot_to_disk

StarArawn

Looks pretty good over all. Can we add an example showcasing this? Right now it's a little hard to test and verify the work.

StarArawn · 2023-01-11T19:36:27Z

crates/bevy_render/src/view/window.rs

@@ -128,6 +138,11 @@ fn extract_windows(
    for closed_window in closed.iter() {
        extracted_windows.remove(&closed_window.id);
    }
+    for (window, screenshot_func) in screenshot_manager.callbacks.lock().drain() {


Probably should use try_lock here to avoid a deadlock if a screenshot is being taken at the same time? Although I guess the bevy scheduler prevents that from happening here..

Yep, this can't really happen because callbacks is pub(crate) and this is the singular callsite where it's called. Even if somehow a user ended up with two of these systems, one would simply take the lock and they'd run in series. Only one lock isn't enough to cause a deadlock.

Nile, your comment here would make a great comment for this line of code.

crates/bevy_render/src/lib.rs

DevinLeamy · 2023-01-11T20:44:21Z

crates/bevy_render/src/texture/image_texture_conversion.rs

@@ -196,6 +197,18 @@ impl Image {
                self.data,
            )
            .map(DynamicImage::ImageRgba8),
+            TextureFormat::Bgra8UnormSrgb => ImageBuffer::from_raw(


Why do we introduce the TextureFormat::Bgra8UnormSrgb here? Is there a reason bgra might be preferred over rgba?

Seconding the confusion about why this is part of this PR. I don't mind it, just a bit confused.

wgpu apparently quite likes this format for swapchain textures - this was necessary to get the save to disk functionality working on my machine.

Interesting. Can you please leave a comment here to that effect then?

alice-i-cecile

Useful functionality, generally looks solid. I'd like to see a top-level example before this gets merged.

Code quality and docs otherwise LGTM. I'll defer to rendering experts on the implementation strategy.

mockersf · 2023-01-11T23:34:48Z

crashes for me on macOS:

thread 'main' panicked at 'Error in Surface::configure: requested usage is not supported', wgpu-0.14.2/src/backend/direct.rs:274:9

TheRawMeatball · 2023-01-12T19:39:15Z

Changed the code to ensure a panic like the one @mockersf encountered is no longer possible, but it's a lot more code now.

IceSentry · 2023-01-12T21:24:26Z

crates/bevy_render/src/renderer/mod.rs

@@ -33,6 +33,9 @@ pub fn render_system(world: &mut World) {
        render_device.clone(), // TODO: is this clone really necessary?
        &render_queue.0,
        world,
+        |encoder| {
+            crate::view::screenshot::submit_screenshot_commands(world, encoder);


Is there a reason why you didn't just import the screenshot module?

Not really, other than not wanting to pollute the namespace i guess.

Fair, I don't mind either way. I'd prefer having the screenshot module in scope just to not have crate::view, but really it's up to you, really not a big deal.

crates/bevy_render/src/view/window.rs

crates/bevy_render/src/view/window/screenshot.rs

crates/bevy_render/src/view/window/screenshot.wgsl

mockersf · 2023-01-12T21:48:55Z

Changed the code to ensure a panic like the one @mockersf encountered is no longer possible, but it's a lot more code now.

Works for me now! Thanks

mockersf · 2023-01-12T21:57:02Z

There is an issue around screenshooting something with HDR & bloom enabled. I enabled them in example animated_fox

OS Screenshot

This PR Screenshot

mockersf · 2023-01-12T22:31:32Z

Crashes in WASM too:

Caused by:
    In a RenderPass
      note: encoder = `<CommandBuffer-(0, 726, Gl)>`
    In a set_pipeline command
      note: render pipeline = `<RenderPipeline-(0, 1, Gl)>`
    Render pipeline targets are incompatible with render pass
    Incompatible color attachment: the renderpass expected [Some(Rgba8UnormSrgb)] but was given [Some(Bgra8UnormSrgb)]

TheRawMeatball · 2023-01-13T14:57:04Z

There is an issue around screenshooting something with HDR & bloom enabled. I enabled them in example animated_fox.

This seems to be a result of the brightness values being stored in the alpha channel - the two images are once again identical when alpha is disabled.

alice-i-cecile · 2023-02-23T14:27:25Z

Thanks for putting this together, @TheRawMeatball. What are the blockers for merging this?

This needs review mostly, especially from folks with rendering expertise. This ended up trickier than I would have expected.

Community review counts, so please feel free to go over this carefully and approve it if you think it's ready :)

mockersf · 2023-02-23T14:38:17Z

the PR also needs to be updated for #7681 I think

hymm · 2023-04-16T02:33:59Z

What's the current status of this PR?

WannaBeKratos · 2023-04-16T20:49:41Z

Does this snippet work without the WinitPlugin ?
// camera commands.spawn(Camera3dBundle { camera_3d: Camera3d { clear_color: ClearColorConfig::Custom(Color::WHITE), ..default() }, camera: Camera { // render before the "main pass" camera order: -1, target: RenderTarget::Image(image_handle), ..default() }, transform: Transform::from_xyz(0.0, 50.0, 5.0).looking_at(Vec3::ZERO, Vec3::Y), ..default() }); }

mockersf · 2023-04-18T11:11:29Z

@TheRawMeatball I think there has been an issue with your merge

DGriffin91 · 2023-04-18T19:38:27Z

What's the purpose of the render pass? Is it to avoid putting TextureUsages::COPY_SRC on the render target?

crates/bevy_render/src/view/window/screenshot.rs

IceSentry

I feel like there should probably be a bit more docs in screenshot.rs, but in general the code works and I haven't seen anything obviously wrong.

I don't fully like the idea of passing a finalizer callback to the graph runner, but I'm not sure how to avoid that.

cart

I think this is a reasonable solution to the specific problem of taking screenshots of a window. But I don't love how "hacked in" it is. I think this impl points to a fundamental "render output feedback" problem that should (ultimately) be solved generically. The screenshot api is pretty opinionated and isn't a full solve to the problem. People should be able to build similar abstractions on their own.

I see a number of paths forward:

Continue down the finalizer path, but just modularize it (ex let people add arbitrary functions to the process via some new abstraction)
Build this into our graph abstraction ... this would mean screenshotting would not be a global feature, but rather one that exists in specific graphs ... this seems ok to me. We could build common "feedback" nodes (including a screenshot node). It seems to me like this PR could have already been expressed that way, rather than adding the finalizer. Functionally submit_screenshot_commands is really equivalent to a "render graph node that exists in every graph and always runs last". This doesn't solve the "window / swap chain / image management stuff" though, which is definitely the bigger challenge. It feels like a chunk of this could/should be expressed as some generic graph node that says "at this point in the graph, write this COPY_SRC texture view to a buffer of appropriate size and (maybe optionally) create an image from it". That leaves the problem of "writing the swap chain to a COPY_SRC texture" to be solved on its own in whatever way we deem "best" / "most modular"/ whatever we choose to optimize for.
Give up on reading swap chain outputs and instead read ViewTarget::maint_target_textures outputs in render graph nodes. Seems like we could make those COPY_SRC (maybe in an opt-in fashion when screenshots are enabled). This does ignore the upscaling step though (which does seem like a problem).

That being said, creating a generic abstraction is not a blocker (and I understand there is a fair amount of implementation nuance here when it comes to swapchain access). This is totally fine for now.

# Objective - Enable taking a screenshot in wasm - Followup on #7163 ## Solution - Create a blob from the image data, generate a url to that blob, add an `a` element to the document linking to that url, click on that element, then revoke the url - This will automatically trigger a download of the screenshot file in the browser

zimond · 2023-06-04T17:15:38Z

Still could this work without a window? It seems some questions in the comments are unanswered.

Add screenshot api

7311b14

TheRawMeatball force-pushed the screenshot-api branch from fddfbcf to 7311b14 Compare January 11, 2023 19:26

IceSentry added A-Rendering Drawing game state to the screen C-Testing A change that impacts how we test Bevy or how users test their apps labels Jan 11, 2023

StarArawn suggested changes Jan 11, 2023

View reviewed changes

IceSentry reviewed Jan 11, 2023

View reviewed changes

crates/bevy_render/src/lib.rs Outdated Show resolved Hide resolved

fix clippy

539370c

DevinLeamy reviewed Jan 11, 2023

View reviewed changes

alice-i-cecile requested changes Jan 11, 2023

View reviewed changes

TheRawMeatball added 3 commits January 12, 2023 20:15

Comment updates

ed06f12

Switch to using custom texture for screenshots

a3208de

appease clippy

6707d19

TheRawMeatball requested review from StarArawn and IceSentry January 12, 2023 19:37

alice-i-cecile added this to the 0.10 milestone Jan 12, 2023

IceSentry reviewed Jan 12, 2023

View reviewed changes

crates/bevy_render/src/view/window.rs Outdated Show resolved Hide resolved

IceSentry reviewed Jan 12, 2023

View reviewed changes

crates/bevy_render/src/view/window.rs Outdated Show resolved Hide resolved

IceSentry reviewed Jan 12, 2023

View reviewed changes

crates/bevy_render/src/view/window/screenshot.rs Outdated Show resolved Hide resolved

IceSentry reviewed Jan 12, 2023

View reviewed changes

crates/bevy_render/src/view/window/screenshot.wgsl Show resolved Hide resolved

alice-i-cecile added the D-Complex Quite challenging from either a design or technical perspective. Ask for help! label Jan 13, 2023

TheRawMeatball added 3 commits January 13, 2023 17:08

Adress more feedback

6487690

Adress WASM panic

bfcd473

clippy

6ba0228

TheRawMeatball added 2 commits February 24, 2023 08:27

Merge remote-tracking branch 'upstream/main' into screenshot-api

d2d72cb

fix things up

cf5fd04

james7132 added this to the 0.11 milestone Mar 1, 2023

alice-i-cecile mentioned this pull request Mar 6, 2023

Show an example of rendering to a texture without a window? #7933

Closed

StaffEngineer mentioned this pull request Apr 1, 2023

Add button to make a screenshot of main area and copy it to clipboard StaffEngineer/velo#1

Closed

Merge commit 'cf5fd04' into HEAD

1da0a8a

TheRawMeatball force-pushed the screenshot-api branch from 2b5edfa to 1da0a8a Compare April 18, 2023 17:35

IceSentry self-requested a review April 18, 2023 20:12

IceSentry reviewed Apr 18, 2023

View reviewed changes

crates/bevy_render/src/view/window/screenshot.rs Outdated Show resolved Hide resolved

IceSentry approved these changes Apr 18, 2023

View reviewed changes

print to log when a screenshot is taken

23bad7a

mockersf approved these changes Apr 19, 2023

View reviewed changes

cart approved these changes Apr 19, 2023

View reviewed changes

cart added this pull request to the merge queue Apr 19, 2023

Merged via the queue into bevyengine:main with commit 9db70da Apr 19, 2023
23 of 24 checks passed

mockersf mentioned this pull request Apr 22, 2023

Screenshots in wasm #8455

Merged

This was referenced Jun 28, 2023

News: Release 0.11 (wrong branch ... see #657) bevyengine/bevy-website#656

Closed

News: Release 0.11 bevyengine/bevy-website#657

Merged

hymm mentioned this pull request Jul 24, 2023

Headless rendering to images #22

Closed

carrascomj mentioned this pull request Oct 18, 2023

Export functionality v2 biosustain/shu#32

Closed

banool mentioned this pull request Jan 23, 2024

Bevy lets you use ScreenshotManager even if it won't work #11493

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add screenshot api #7163

Add screenshot api #7163

TheRawMeatball commented Jan 11, 2023 •

edited

StarArawn left a comment

StarArawn Jan 11, 2023

TheRawMeatball Jan 11, 2023

alice-i-cecile Jan 11, 2023

DevinLeamy Jan 11, 2023

alice-i-cecile Jan 11, 2023

TheRawMeatball Jan 11, 2023

alice-i-cecile Jan 11, 2023

alice-i-cecile left a comment

mockersf commented Jan 11, 2023

TheRawMeatball commented Jan 12, 2023

IceSentry Jan 12, 2023

TheRawMeatball Jan 13, 2023

IceSentry Jan 13, 2023

mockersf commented Jan 12, 2023

mockersf commented Jan 12, 2023 •

edited

mockersf commented Jan 12, 2023

TheRawMeatball commented Jan 13, 2023

alice-i-cecile commented Feb 23, 2023

mockersf commented Feb 23, 2023

hymm commented Apr 16, 2023

WannaBeKratos commented Apr 16, 2023

mockersf commented Apr 18, 2023

DGriffin91 commented Apr 18, 2023

IceSentry left a comment

cart left a comment

zimond commented Jun 4, 2023

Add screenshot api #7163

Add screenshot api #7163

Conversation

TheRawMeatball commented Jan 11, 2023 • edited

Objective

Solution

Changelog

StarArawn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alice-i-cecile left a comment

Choose a reason for hiding this comment

mockersf commented Jan 11, 2023

TheRawMeatball commented Jan 12, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mockersf commented Jan 12, 2023

mockersf commented Jan 12, 2023 • edited

OS Screenshot

This PR Screenshot

mockersf commented Jan 12, 2023

TheRawMeatball commented Jan 13, 2023

alice-i-cecile commented Feb 23, 2023

mockersf commented Feb 23, 2023

hymm commented Apr 16, 2023

WannaBeKratos commented Apr 16, 2023

mockersf commented Apr 18, 2023

DGriffin91 commented Apr 18, 2023

IceSentry left a comment

Choose a reason for hiding this comment

cart left a comment

Choose a reason for hiding this comment

zimond commented Jun 4, 2023

TheRawMeatball commented Jan 11, 2023 •

edited

mockersf commented Jan 12, 2023 •

edited