Introduce a `WindowWrapper` to extend the lifetime of the window when using pipelined rendering #12978

Friz64 · 2024-04-15T15:44:39Z

Objective

A RawWindowHandle is only valid as long as the window it was retrieved from is alive. Extend the lifetime of the window, so that the RawWindowHandle doesn't outlive it, and bevy doesn't crash when closing a window a pipelined renderer is drawing to.

Fix Multiple windows example panics when closing one of the windows #11236
Fix XError BadWindow (invalid Window parameter) when closing windows at seemingly random #11150
Fix 2D examples crash on exit with "Couldn't get swap chain texture" #11734
Alternative to / Closes Arc WinitWindow and extract to render world #12524

Solution

Introduce a WindowWrapper that takes ownership of the window. Require it to be used when constructing a RawHandleWrapper. This forces windowing backends to store their window in this wrapper.

The WindowWrapper is implemented by storing the window in an Arc<dyn Any + Send + Sync>.

We use dynamic dispatch here because we later want the RawHandleWrapper to be able dynamically hold a reference to any windowing backend's window.

But alas, the WindowWrapper itself is still practically invisible to windowing backends, because it implements Deref to the underlying window, by storing its type in a PhantomData.

Changelog

Added

Added WindowWrapper, which windowing backends are now required to use to store their underlying window.

Fixed

Fixed a safety problem which caused crashes when closing bevy windows when using pipelined rendering.

Migration Guide

Windowing backends now need to store their window in the new WindowWrapper.

… using pipelined rendering

hymm · 2024-04-16T02:38:14Z

crates/bevy_window/src/raw_handle.rs

+/// which gets picked up by the renderer during extraction.
+#[derive(Debug)]
+pub struct WindowWrapper<W> {
+    reference: Arc<dyn Any + Send + Sync>,


Why does this need to be type erased instead of storing a Arc<W>?

This is so that we can later store this Arc in the RawHandleWrapper without static typing:

bevy/crates/bevy_window/src/raw_handle.rs

Lines 47 to 48 in 5a01357

pub struct RawHandleWrapper {

_window: Arc<dyn Any + Send + Sync>,

Friz64 · 2024-04-30T02:06:42Z

That Android CI failure is an oversight of mine. I've fixed it, so that it now compiles and runs successfully on my phone.

I noticed that bevyengine#12978 introduces a reference counted wrapper around windows, but still uses `RawWindowHandle` and `RawDisplayHandle`, with `wgpu::Instance::create_surface_unsafe`. This can be changed easily enough to use `wgpu::Instance::create_surface`, and not use the raw handles anywhere. Apparently we can just cast a `Arc<W>` to an `Arc<dyn Trait>`, so that makes `WindowWrapper` a little simpler too.

Fixes two issues related to bevyengine#13208. First, we ensure render resources for a window are always dropped first to ensure that the `winit::Window` always drops on the main thread when it is removed from `WinitWindows`. Previously, changes in bevyengine#12978 caused the window to drop in the render world, causing issues. We accomplish this by delaying despawning the window by a frame by inserting a marker component `ClosingWindow` that indicates the window has been requested to close and is in the process of closing. The render world now responds to the equivalent `WindowClosing` event rather than `WindowCloseed` which now fires after the render resources are guarunteed to be cleaned up. Secondly, fixing the above caused (revealed?) that additional events were being delivered to the the event loop handler after exit had already been requested: in my testing `RedrawRequested` and `LoopExiting`. This caused errors to be reported try to send an exit event on the close channel. There are two options here: - Guard the handler so no additional events are delivered once the app is exiting. I considered this but worried it might be confusing or bug prone if in the future someone wants to handle `LoopExiting` or some other event to clean-up while exiting. - Only send an exit signal if we are not already exiting. It doesn't appear to cause any problems to handle the extra events so this seems safer. Fixing this also appears to have fixed bevyengine#13231.

# Objective Fixes two issues related to #13208. First, we ensure render resources for a window are always dropped first to ensure that the `winit::Window` always drops on the main thread when it is removed from `WinitWindows`. Previously, changes in #12978 caused the window to drop in the render world, causing issues. We accomplish this by delaying despawning the window by a frame by inserting a marker component `ClosingWindow` that indicates the window has been requested to close and is in the process of closing. The render world now responds to the equivalent `WindowClosing` event rather than `WindowCloseed` which now fires after the render resources are guarunteed to be cleaned up. Secondly, fixing the above caused (revealed?) that additional events were being delivered to the the event loop handler after exit had already been requested: in my testing `RedrawRequested` and `LoopExiting`. This caused errors to be reported try to send an exit event on the close channel. There are two options here: - Guard the handler so no additional events are delivered once the app is exiting. I ~considered this but worried it might be confusing or bug prone if in the future someone wants to handle `LoopExiting` or some other event to clean-up while exiting.~ We are now taking this approach. - Only send an exit signal if we are not already exiting. ~It doesn't appear to cause any problems to handle the extra events so this seems safer.~ Fixing this also appears to have fixed #13231. Fixes #10260. ## Testing Tested on mac only. --- ## Changelog ### Added - A `WindowClosing` event has been added that indicates the window will be despawned on the next frame. ### Changed - Windows now close a frame after their exit has been requested. ## Migration Guide - Ensure custom exit logic does not rely on the app exiting the same frame as a window is closed.

Introduce a WindowWrapper to extend the lifetime of the window when…

5a01357

… using pipelined rendering

alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Rendering Drawing game state to the screen A-Windowing Platform-agnostic interface layer to run your app in labels Apr 15, 2024

alice-i-cecile requested review from superdump and james7132 April 15, 2024 19:11

alice-i-cecile added the P-Unsound A bug that results in undefined compiler behavior label Apr 15, 2024

alice-i-cecile added this to the 0.14 milestone Apr 15, 2024

hymm reviewed Apr 16, 2024

View reviewed changes

JMS55 approved these changes Apr 17, 2024

View reviewed changes

james7132 approved these changes Apr 18, 2024

View reviewed changes

james7132 added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Apr 18, 2024

james7132 requested a review from hymm April 18, 2024 03:32

IceSentry added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Apr 25, 2024

alice-i-cecile added this pull request to the merge queue Apr 30, 2024

github-merge-queue bot removed this pull request from the merge queue due to failed status checks Apr 30, 2024

Fix android support

bf14ef1

alice-i-cecile added this pull request to the merge queue Apr 30, 2024

Merged via the queue into bevyengine:main with commit 9973f0c Apr 30, 2024
27 checks passed

ids1024 mentioned this pull request May 2, 2024

Use safe window handles and wgpu::Instance::create_surface #13172

Open

rparrett mentioned this pull request May 3, 2024

macos: app hangs after pressing window close button #13208

Closed

mockersf mentioned this pull request May 3, 2024

ignore the window arc on macos #13221

Closed

tychedelia mentioned this pull request May 4, 2024

Ensure clean exit #13236

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Introduce a `WindowWrapper` to extend the lifetime of the window when using pipelined rendering #12978

Introduce a `WindowWrapper` to extend the lifetime of the window when using pipelined rendering #12978

Friz64 commented Apr 15, 2024 •

edited

Loading

hymm Apr 16, 2024 •

edited

Loading

Friz64 Apr 16, 2024 •

edited

Loading

Friz64 commented Apr 30, 2024

	pub struct RawHandleWrapper {
	_window: Arc<dyn Any + Send + Sync>,

Introduce a WindowWrapper to extend the lifetime of the window when using pipelined rendering #12978

Introduce a WindowWrapper to extend the lifetime of the window when using pipelined rendering #12978

Conversation

Friz64 commented Apr 15, 2024 • edited Loading

Objective

Solution

Changelog

Added

Fixed

Migration Guide

hymm Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Friz64 Apr 16, 2024 • edited Loading

Choose a reason for hiding this comment

Friz64 commented Apr 30, 2024

Introduce a `WindowWrapper` to extend the lifetime of the window when using pipelined rendering #12978

Introduce a `WindowWrapper` to extend the lifetime of the window when using pipelined rendering #12978

Friz64 commented Apr 15, 2024 •

edited

Loading

hymm Apr 16, 2024 •

edited

Loading

Friz64 Apr 16, 2024 •

edited

Loading