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

Rewrite screenshots. #14833

Merged
merged 21 commits into from
Aug 25, 2024
Merged

Conversation

tychedelia
Copy link
Contributor

@tychedelia tychedelia commented Aug 20, 2024

Objective

Rewrite screenshotting to be able to accept any RenderTarget.

Closes #12478

Solution

Previously, screenshotting relied on setting a variety of state on the requested window. When extracted, the window's swap_chain_texture_view property would be swapped out with a texture_view created that frame for the screenshot pipeline to write back to the cpu.

Besides being tightly coupled to window in a way that prevented screenshotting other render targets, this approach had the drawback of relying on the implicit state of swap_chain_texture_view being returned from a NormalizedRenderTarget when view targets were prepared. Because property is set every frame for windows, that wasn't a problem, but poses a problem for render target images. Namely, to do the equivalent trick, we'd have to replace the GpuImage's texture view, and somehow restore it later.

As such, this PR creates a new prepare_view_textures system which runs before prepare_view_targets that allows a new prepare_screenshots system to be sandwiched between and overwrite the render targets texture view if a screenshot has been requested that frame for the given target.

Additionally, screenshotting itself has been changed to use a component + observer pattern. We now spawn a Screenshot component into the world, whose lifetime is tracked with a series of marker components. When the screenshot is read back to the CPU, we send the image over a channel back to the main world where an observer fires on the screenshot entity before being despawned the next frame. This allows the user to access resources in their save callback that might be useful (e.g. uploading the screenshot over the network, etc.).

Testing

image

TODO:

  • Web
  • Manual texture view

Showcase

render to texture example:

web saving still works:

Migration Guide

ScreenshotManager has been removed. To take a screenshot, spawn a Screenshot entity with the specified render target and provide an observer targeting the ScreenshotCaptured event. See the window/screenshot example to see an example.

@tychedelia tychedelia added A-Rendering Drawing game state to the screen S-Needs-Review Needs reviewer attention (from anyone!) to move forward C-Feature A new feature, making something new possible labels Aug 20, 2024
Copy link
Contributor

@killercup killercup left a comment

Choose a reason for hiding this comment

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

I've tried to follow the reasoning and the change and they make sense to me. I'm not confident in my skills to review the render changes.

crates/bevy_render/src/view/window/screenshot.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/view/window/screenshot.rs Outdated Show resolved Hide resolved
examples/window/screenshot.rs Show resolved Hide resolved
@tychedelia
Copy link
Contributor Author

I'm not totally sure if we should rewrite the headless renderer example to use this. It certainly would reduce a lot of boilerplate in that example but the example is also more efficient since it doesn't recreate textures and buffers for every screenshot like we do here. We could make this more efficient in engine by keeping those resources around rather than creating them for every new screenshot, but I'm not totally sure whether that's a use case we actually want to support in engine...

@tychedelia tychedelia requested a review from killercup August 20, 2024 21:27
Co-authored-by: Kristoffer Søholm <k.soeholm@gmail.com>
Copy link
Contributor

@killercup killercup left a comment

Choose a reason for hiding this comment

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

Example works, and code makes sense. Left a couple inline comments but consider this approved :)

crates/bevy_render/src/view/window/screenshot.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/view/window/screenshot.rs Outdated Show resolved Hide resolved
examples/window/screenshot.rs Show resolved Hide resolved
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

This PR is well motivated and I much prefer the new component/observer api. I am a little wary of the changes to prepare_view_textures/targets, they seem generally like an improvement but are not quite 1-to-1 with the behavior of the original.

In terms of reusing textures and porting over the headless example, I think that's something we can investigate as follow up. The perf and ergonomics are fine for now, and the screenshot example demonstrates use well enough.

crates/bevy_render/src/view/mod.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/view/mod.rs Outdated Show resolved Hide resolved
crates/bevy_render/src/view/mod.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Aug 23, 2024
@tychedelia tychedelia requested a review from NthTensor August 23, 2024 17:41
Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

Yep, looks like this preserves the old behavior now. Haven't run this code yet, but approving anyway.

@NthTensor NthTensor added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 23, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 25, 2024
Merged via the queue into bevyengine:main with commit d9527c1 Aug 25, 2024
26 checks passed
github-merge-queue bot pushed a commit that referenced this pull request Aug 31, 2024
Fixes #14993 (maybe). Adds a system ordering constraint that was missed
in the refactor in #14833. The theory here is that the single threaded
forces a topology that causes the prepare system to run before
`prepare_windows` in a way that causes issues. For whatever reason, this
appears to be unlikely when multi-threading is enabled.
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1678 if you'd like to help out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

Screenshots from any render target not just windows
5 participants