From a8acdb38a7f183ad54ecace1d0368a0fdf31275f Mon Sep 17 00:00:00 2001 From: Charlotte McElwain Date: Sat, 4 May 2024 12:02:49 -0700 Subject: [PATCH] Ensure clean exit 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. - 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. --- crates/bevy_render/src/view/window/mod.rs | 10 +++++----- crates/bevy_window/src/event.rs | 14 ++++++++++++++ crates/bevy_window/src/lib.rs | 2 ++ crates/bevy_window/src/system.rs | 15 ++++++++++++--- crates/bevy_window/src/window.rs | 5 +++++ crates/bevy_winit/src/lib.rs | 5 +++++ crates/bevy_winit/src/system.rs | 16 ++++++++++++---- 7 files changed, 55 insertions(+), 12 deletions(-) diff --git a/crates/bevy_render/src/view/window/mod.rs b/crates/bevy_render/src/view/window/mod.rs index 762771a41edd5..cf347586e6429 100644 --- a/crates/bevy_render/src/view/window/mod.rs +++ b/crates/bevy_render/src/view/window/mod.rs @@ -12,7 +12,7 @@ use bevy_ecs::{entity::EntityHashMap, prelude::*}; use bevy_utils::warn_once; use bevy_utils::{default, tracing::debug, HashSet}; use bevy_window::{ - CompositeAlphaMode, PresentMode, PrimaryWindow, RawHandleWrapper, Window, WindowClosed, + CompositeAlphaMode, PresentMode, PrimaryWindow, RawHandleWrapper, Window, WindowClosing, }; use std::{ num::NonZeroU32, @@ -117,7 +117,7 @@ impl DerefMut for ExtractedWindows { fn extract_windows( mut extracted_windows: ResMut, screenshot_manager: Extract>, - mut closed: Extract>, + mut closing: Extract>, windows: Extract)>>, mut removed: Extract>, mut window_surfaces: ResMut, @@ -177,9 +177,9 @@ fn extract_windows( } } - for closed_window in closed.read() { - extracted_windows.remove(&closed_window.window); - window_surfaces.remove(&closed_window.window); + for closing_window in closing.read() { + extracted_windows.remove(&closing_window.window); + window_surfaces.remove(&closing_window.window); } for removed_window in removed.read() { extracted_windows.remove(&removed_window); diff --git a/crates/bevy_window/src/event.rs b/crates/bevy_window/src/event.rs index 039b646b8f94a..9bc698acaed72 100644 --- a/crates/bevy_window/src/event.rs +++ b/crates/bevy_window/src/event.rs @@ -94,6 +94,20 @@ pub struct WindowClosed { pub window: Entity, } +/// An event that is sent whenever a window is closing. This will be sent when +/// after a [`WindowCloseRequested`] event is received and the window is in the process of closing. +#[derive(Event, Debug, Clone, PartialEq, Eq, Reflect)] +#[reflect(Debug, PartialEq)] +#[cfg_attr( + feature = "serialize", + derive(serde::Serialize, serde::Deserialize), + reflect(Serialize, Deserialize) +)] +pub struct WindowClosing { + /// Window that has been requested to close and is the process of closing. + pub window: Entity, +} + /// An event that is sent whenever a window is destroyed by the underlying window system. /// /// Note that if your application only has a single window, this event may be your last chance to diff --git a/crates/bevy_window/src/lib.rs b/crates/bevy_window/src/lib.rs index cabdb04d1818d..7ecf8d21c0e02 100644 --- a/crates/bevy_window/src/lib.rs +++ b/crates/bevy_window/src/lib.rs @@ -89,6 +89,7 @@ impl Plugin for WindowPlugin { #[allow(deprecated)] app.add_event::() .add_event::() + .add_event::() .add_event::() .add_event::() .add_event::() @@ -139,6 +140,7 @@ impl Plugin for WindowPlugin { .register_type::() .register_type::() .register_type::() + .register_type::() .register_type::() .register_type::() .register_type::() diff --git a/crates/bevy_window/src/system.rs b/crates/bevy_window/src/system.rs index ac92c77408e71..d6bb40bfefdc6 100644 --- a/crates/bevy_window/src/system.rs +++ b/crates/bevy_window/src/system.rs @@ -1,4 +1,4 @@ -use crate::{PrimaryWindow, Window, WindowCloseRequested}; +use crate::{ClosingWindow, PrimaryWindow, Window, WindowCloseRequested}; use bevy_app::AppExit; use bevy_ecs::prelude::*; @@ -39,8 +39,17 @@ pub fn exit_on_primary_closed( /// Ensure that you read the caveats documented on that field if doing so. /// /// [`WindowPlugin`]: crate::WindowPlugin -pub fn close_when_requested(mut commands: Commands, mut closed: EventReader) { +pub fn close_when_requested( + mut commands: Commands, + mut closed: EventReader, + closing: Query>, +) { + // This was inserted by us on the last frame so now we can despawn the window + for window in closing.iter() { + commands.entity(window).despawn(); + } + // Mark the window as closing so we can despawn it on the next frame for event in closed.read() { - commands.entity(event.window).despawn(); + commands.entity(event.window).insert(ClosingWindow); } } diff --git a/crates/bevy_window/src/window.rs b/crates/bevy_window/src/window.rs index d80f070a1657b..453c9245b58d8 100644 --- a/crates/bevy_window/src/window.rs +++ b/crates/bevy_window/src/window.rs @@ -1171,6 +1171,11 @@ impl Default for EnabledButtons { } } +/// Marker component for a [`Window`] that has been requested to close and +/// is in the process of closing (on the next frame). +#[derive(Component)] +pub struct ClosingWindow; + #[cfg(test)] mod tests { use super::*; diff --git a/crates/bevy_winit/src/lib.rs b/crates/bevy_winit/src/lib.rs index 69008613259d7..558b2e836a5a8 100644 --- a/crates/bevy_winit/src/lib.rs +++ b/crates/bevy_winit/src/lib.rs @@ -776,6 +776,11 @@ fn handle_winit_event( } if let Some(app_exit) = app.should_exit() { + // The event loop is in the process of exiting, but we might still be processing events. + if event_loop.exiting() { + return; + } + if let Err(err) = exit_notify.try_send(app_exit) { error!("Failed to send a app exit notification! This is a bug. Reason: {err}"); }; diff --git a/crates/bevy_winit/src/system.rs b/crates/bevy_winit/src/system.rs index 82327aaf6a60f..d871c0fb6e207 100644 --- a/crates/bevy_winit/src/system.rs +++ b/crates/bevy_winit/src/system.rs @@ -8,7 +8,8 @@ use bevy_ecs::{ }; use bevy_utils::tracing::{error, info, warn}; use bevy_window::{ - RawHandleWrapper, Window, WindowClosed, WindowCreated, WindowMode, WindowResized, + ClosingWindow, RawHandleWrapper, Window, WindowClosed, WindowClosing, WindowCreated, + WindowMode, WindowResized, }; use winit::{ @@ -16,6 +17,7 @@ use winit::{ event_loop::EventLoopWindowTarget, }; +use bevy_ecs::query::With; #[cfg(target_arch = "wasm32")] use winit::platform::web::WindowExtWebSys; @@ -97,18 +99,24 @@ pub fn create_windows( } pub(crate) fn despawn_windows( + closing: Query>, mut closed: RemovedComponents, window_entities: Query<&Window>, - mut close_events: EventWriter, + mut closing_events: EventWriter, + mut closed_events: EventWriter, mut winit_windows: NonSendMut, ) { + for window in closing.iter() { + closing_events.send(WindowClosing { window }); + } for window in closed.read() { info!("Closing window {:?}", window); // Guard to verify that the window is in fact actually gone, - // rather than having the component added and removed in the same frame. + // rather than having the component added + // and removed in the same frame. if !window_entities.contains(window) { winit_windows.remove_window(window); - close_events.send(WindowClosed { window }); + closed_events.send(WindowClosed { window }); } } }