Skip to content

AppExit should be an observer event rather than a BufferedEvent #20408 #20442

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

Closed
wants to merge 11 commits into from
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
59 changes: 32 additions & 27 deletions crates/bevy_app/src/app.rs
Original file line number Diff line number Diff line change
Expand Up @@ -11,7 +11,7 @@ pub use bevy_derive::AppLabel;
use bevy_ecs::{
component::RequiredComponentsError,
error::{DefaultErrorHandler, ErrorHandler},
event::{event_update_system, EventCursor},
event::event_update_system,
intern::Interned,
prelude::*,
schedule::{InternedSystemSet, ScheduleBuildSettings, ScheduleLabel},
Expand Down Expand Up @@ -123,7 +123,6 @@ impl Default for App {
.in_set(bevy_ecs::event::EventUpdateSystems)
.run_if(bevy_ecs::event::event_update_condition),
);
app.add_event::<AppExit>();

app
}
Expand Down Expand Up @@ -1282,27 +1281,27 @@ impl App {
self
}

/// Register the first [`Error`](AppExit::Error) seen.
pub fn register_app_exit_error(
on_app_exit: On<AppExit>,
mut app_event_res: ResMut<AppExitRes>,
) {
if !app_event_res.state.is_error() {
let event_in = on_app_exit.event();
if event_in.is_error() {
app_event_res.state = event_in.clone();
}
}
}

/// Attempts to determine if an [`AppExit`] was raised since the last update.
///
/// Will attempt to return the first [`Error`](AppExit::Error) it encounters.
/// This should be called after every [`update()`](App::update) otherwise you risk
/// dropping possible [`AppExit`] events.
pub fn should_exit(&self) -> Option<AppExit> {
let mut reader = EventCursor::default();

let events = self.world().get_resource::<Events<AppExit>>()?;
let mut events = reader.read(events);

if events.len() != 0 {
return Some(
events
.find(|exit| exit.is_error())
.cloned()
.unwrap_or(AppExit::Success),
);
let stored_event = &self.world().get_resource::<AppExitRes>()?.state;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think something is going wrong here, if I understand correctly you will only return a Some if any non AppExit::Success has been triggered (as a result of register_app_exit_error). However in the old code, a Some(AppExit::Success would still be emitted if there is at least 1 event and none of them are errors.

In other words, I believe your version would simply not exit upon AppExit::Success

if stored_event.is_error() {
Some(stored_event.clone())
} else {
None
}

None
}

/// Spawns an [`Observer`] entity, which will watch for and respond to the given event.
Expand Down Expand Up @@ -1405,7 +1404,7 @@ fn run_once(mut app: App) -> AppExit {
app.should_exit().unwrap_or(AppExit::Success)
}

/// A [`BufferedEvent`] that indicates the [`App`] should exit. If one or more of these are present at the end of an update,
/// A [`Event`] that indicates the [`App`] should exit.
/// the [runner](App::set_runner) will end and ([maybe](App::run)) return control to the caller.
///
/// This event can be used to detect when an exit is requested. Make sure that systems listening
Expand All @@ -1415,7 +1414,7 @@ fn run_once(mut app: App) -> AppExit {
/// This type is roughly meant to map to a standard definition of a process exit code (0 means success, not 0 means error). Due to portability concerns
/// (see [`ExitCode`](https://doc.rust-lang.org/std/process/struct.ExitCode.html) and [`process::exit`](https://doc.rust-lang.org/std/process/fn.exit.html#))
/// we only allow error codes between 1 and [255](u8::MAX).
#[derive(BufferedEvent, Debug, Clone, Default, PartialEq, Eq)]
#[derive(Event, Debug, Clone, Default, PartialEq, Eq)]
pub enum AppExit {
/// [`App`] exited without any problems.
#[default]
Expand Down Expand Up @@ -1474,6 +1473,12 @@ impl Termination for AppExit {
}
}

/// Resource that latches the first [`AppExit::Error`] seen.
#[derive(Resource)]
pub struct AppExitRes {
state: AppExit,
}

#[cfg(test)]
mod tests {
use core::marker::PhantomData;
Expand All @@ -1483,7 +1488,7 @@ mod tests {
change_detection::{DetectChanges, ResMut},
component::Component,
entity::Entity,
event::{BufferedEvent, EventWriter, Events},
event::{BufferedEvent, Events},
lifecycle::RemovedComponents,
query::With,
resource::Resource,
Expand Down Expand Up @@ -1754,12 +1759,12 @@ mod tests {

#[test]
fn runner_returns_correct_exit_code() {
fn raise_exits(mut exits: EventWriter<AppExit>) {
fn raise_exits(mut commands: Commands) {
// Exit codes chosen by a fair dice roll.
// Unlikely to overlap with default values.
exits.write(AppExit::Success);
exits.write(AppExit::from_code(4));
exits.write(AppExit::from_code(73));
commands.trigger(AppExit::Success);
commands.trigger(AppExit::from_code(4));
commands.trigger(AppExit::from_code(73));
}

let exit = App::new().add_systems(Update, raise_exits).run();
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_app/src/terminal_ctrl_c_handler.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use core::sync::atomic::{AtomicBool, Ordering};

use bevy_ecs::event::EventWriter;
use bevy_ecs::system::Commands;

use crate::{App, AppExit, Plugin, Update};

Expand Down Expand Up @@ -48,9 +48,9 @@ impl TerminalCtrlCHandlerPlugin {
}

/// Sends a [`AppExit`] event when the user presses `Ctrl+C` on the terminal.
pub fn exit_on_flag(mut events: EventWriter<AppExit>) {
pub fn exit_on_flag(mut commands: Commands) {
if SHOULD_EXIT.load(Ordering::Relaxed) {
events.write(AppExit::from_code(130));
commands.trigger(AppExit::from_code(130));
}
}
}
Expand Down
5 changes: 2 additions & 3 deletions crates/bevy_asset/src/asset_changed.rs
Original file line number Diff line number Diff line change
Expand Up @@ -297,7 +297,6 @@ mod tests {
use bevy_ecs::schedule::IntoScheduleConfigs;
use bevy_ecs::{
component::Component,
event::EventWriter,
resource::Resource,
system::{Commands, IntoSystem, Local, Query, Res, ResMut},
};
Expand Down Expand Up @@ -333,9 +332,9 @@ mod tests {
fn handle_filter_pos_ok() {
fn compatible_filter(
_query: Query<&mut MyComponent, AssetChanged<MyComponent>>,
mut exit: EventWriter<AppExit>,
mut commands: Commands,
) {
exit.write(AppExit::Error(NonZero::<u8>::MIN));
commands.trigger(AppExit::Error(NonZero::<u8>::MIN));
}
run_app(compatible_filter);
}
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_dev_tools/src/ci_testing/systems.rs
Original file line number Diff line number Diff line change
Expand Up @@ -18,18 +18,18 @@ pub(crate) fn send_events(world: &mut World, mut current_frame: Local<u32>) {
debug!("Handling event: {:?}", event);
match event {
CiTestingEvent::AppExit => {
world.write_event(AppExit::Success);
world.trigger(AppExit::Success);
info!("Exiting after {} frames. Test successful!", *current_frame);
}
CiTestingEvent::ScreenshotAndExit => {
let this_frame = *current_frame;
world.spawn(Screenshot::primary_window()).observe(
move |captured: On<bevy_render::view::screenshot::ScreenshotCaptured>,
mut exit_event: EventWriter<AppExit>| {
mut commands: Commands| {
let path = format!("./screenshot-{this_frame}.png");
save_to_disk(path)(captured);
info!("Exiting. Test successful!");
exit_event.write(AppExit::Success);
commands.trigger(AppExit::Success);
},
);
info!("Took a screenshot at frame {}.", *current_frame);
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_render/src/pipelined_rendering.rs
Original file line number Diff line number Diff line change
Expand Up @@ -197,7 +197,7 @@ fn renderer_extract(app_world: &mut World, _world: &mut World) {
render_channels.send_blocking(render_app);
} else {
// Renderer thread panicked
world.write_event(AppExit::error());
world.trigger(AppExit::error());
}
});
});
Expand Down
7 changes: 4 additions & 3 deletions crates/bevy_time/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,9 +92,10 @@ impl Plugin for TimePlugin {

// Ensure the events are not dropped until `FixedMain` systems can observe them
app.add_systems(FixedPostUpdate, signal_event_update_system);
let mut event_registry = app.world_mut().resource_mut::<EventRegistry>();
// We need to start in a waiting state so that the events are not updated until the first fixed update
event_registry.should_update = ShouldUpdateEvents::Waiting;
if let Some(mut event_registry) = app.world_mut().get_resource_mut::<EventRegistry>() {
// We need to start in a waiting state so that the events are not updated until the first fixed update
event_registry.should_update = ShouldUpdateEvents::Waiting;
}
}
}

Expand Down
8 changes: 4 additions & 4 deletions crates/bevy_window/src/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -10,10 +10,10 @@ use bevy_ecs::prelude::*;
/// Ensure that you read the caveats documented on that field if doing so.
///
/// [`WindowPlugin`]: crate::WindowPlugin
pub fn exit_on_all_closed(mut app_exit_events: EventWriter<AppExit>, windows: Query<&Window>) {
pub fn exit_on_all_closed(mut commands: Commands, windows: Query<&Window>) {
if windows.is_empty() {
log::info!("No windows are open, exiting");
app_exit_events.write(AppExit::Success);
commands.trigger(AppExit::Success);
}
}

Expand All @@ -23,12 +23,12 @@ pub fn exit_on_all_closed(mut app_exit_events: EventWriter<AppExit>, windows: Qu
///
/// [`WindowPlugin`]: crate::WindowPlugin
pub fn exit_on_primary_closed(
mut app_exit_events: EventWriter<AppExit>,
mut commands: Commands,
windows: Query<(), (With<Window>, With<PrimaryWindow>)>,
) {
if windows.is_empty() {
log::info!("Primary window was closed, exiting");
app_exit_events.write(AppExit::Success);
commands.trigger(AppExit::Success);
}
}

Expand Down
7 changes: 4 additions & 3 deletions crates/bevy_winit/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ use bevy_a11y::AccessibilityRequested;
use bevy_app::{App, Last, Plugin};
use bevy_ecs::prelude::*;
use bevy_window::{exit_on_all_closed, CursorOptions, Window, WindowCreated};
use system::{changed_cursor_options, changed_windows, check_keyboard_focus_lost, despawn_windows};
use system::{changed_cursor_options, changed_windows, check_keyboard_focus_lost, on_app_exit};
pub use system::{create_monitors, create_windows};
#[cfg(all(target_family = "wasm", target_os = "unknown"))]
pub use winit::platform::web::CustomCursorExtWebSys;
Expand Down Expand Up @@ -132,7 +132,9 @@ impl<T: BufferedEvent> Plugin for WinitPlugin<T> {

app.init_resource::<WinitMonitors>()
.init_resource::<WinitSettings>()
.insert_resource(DisplayHandleWrapper(event_loop.owned_display_handle()))
.insert_resource(DisplayHandleWrapper(event_loop.owned_display_handle()));

app.add_observer(on_app_exit)
.add_event::<RawWinitWindowEvent>()
Copy link
Contributor

Choose a reason for hiding this comment

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

Why split up this chain? Also .add_event::<RawWinitWindowEvent>() belongs close to .insert_resource(DisplayHandleWrapper(event_loop.owned_display_handle())), they serve a complimentary purpose.

.set_runner(|app| winit_runner(app, event_loop))
.add_systems(
Expand All @@ -142,7 +144,6 @@ impl<T: BufferedEvent> Plugin for WinitPlugin<T> {
// so we don't need to care about its ordering relative to `changed_windows`
changed_windows.ambiguous_with(exit_on_all_closed),
changed_cursor_options,
despawn_windows,
check_keyboard_focus_lost,
)
.chain(),
Expand Down
14 changes: 7 additions & 7 deletions crates/bevy_winit/src/system.rs
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@ use bevy_ecs::{
entity::Entity,
event::EventWriter,
lifecycle::RemovedComponents,
observer::On,
prelude::{Changed, Component},
query::QueryFilter,
system::{Local, NonSendMarker, Query, SystemParamItem},
Expand Down Expand Up @@ -238,14 +239,15 @@ pub fn create_monitors(
});
}

pub(crate) fn despawn_windows(
/// Observer that handles [`AppExit`] events.
pub(crate) fn on_app_exit(
_exit_event: On<AppExit>,
closing: Query<Entity, With<ClosingWindow>>,
mut closed: RemovedComponents<Window>,
window_entities: Query<Entity, With<Window>>,
mut closing_events: EventWriter<WindowClosing>,
mut closed_events: EventWriter<WindowClosed>,
mut windows_to_drop: Local<Vec<WindowWrapper<winit::window::Window>>>,
mut exit_events: EventReader<AppExit>,
_non_send_marker: NonSendMarker,
) {
// Drop all the windows that are waiting to be closed
Expand Down Expand Up @@ -274,11 +276,9 @@ pub(crate) fn despawn_windows(

// On macOS, when exiting, we need to tell the rendering thread the windows are about to
// close to ensure that they are dropped on the main thread. Otherwise, the app will hang.
if !exit_events.is_empty() {
exit_events.clear();
for window in window_entities.iter() {
closing_events.write(WindowClosing { window });
}

for window in window_entities.iter() {
closing_events.write(WindowClosing { window });
}
}

Expand Down
2 changes: 1 addition & 1 deletion examples/3d/clustered_decals.rs
Original file line number Diff line number Diff line change
Expand Up @@ -162,7 +162,7 @@ fn setup(
// Error out if clustered decals aren't supported on the current platform.
if !decal::clustered::clustered_decals_are_usable(&render_device, &render_adapter) {
error!("Clustered decals aren't usable on this platform.");
commands.write_event(AppExit::error());
commands.trigger(AppExit::error());
}

spawn_cube(&mut commands, &mut meshes, &mut materials);
Expand Down
2 changes: 1 addition & 1 deletion examples/3d/light_textures.rs
Original file line number Diff line number Diff line change
Expand Up @@ -152,7 +152,7 @@ fn setup(
// Error out if clustered decals (and so light textures) aren't supported on the current platform.
if !decal::clustered::clustered_decals_are_usable(&render_device, &render_adapter) {
error!("Light textures aren't usable on this platform.");
commands.write_event(AppExit::error());
commands.trigger(AppExit::error());
}

spawn_cubes(&mut commands, &mut meshes, &mut materials);
Expand Down
4 changes: 2 additions & 2 deletions examples/app/custom_loop.rs
Original file line number Diff line number Diff line change
Expand Up @@ -33,9 +33,9 @@ fn print_system(input: Res<Input>) {
println!("You typed: {}", input.0);
}

fn exit_system(input: Res<Input>, mut exit_event: EventWriter<AppExit>) {
fn exit_system(input: Res<Input>, mut commands: Commands) {
if input.0 == "exit" {
exit_event.write(AppExit::Success);
commands.trigger(AppExit::Success);
}
}

Expand Down
4 changes: 2 additions & 2 deletions examples/app/headless_renderer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -475,7 +475,7 @@ fn update(
receiver: Res<MainWorldReceiver>,
mut images: ResMut<Assets<Image>>,
mut scene_controller: ResMut<SceneController>,
mut app_exit_writer: EventWriter<AppExit>,
mut commands: Commands,
mut file_number: Local<u32>,
) {
if let SceneState::Render(n) = scene_controller.state {
Expand Down Expand Up @@ -536,7 +536,7 @@ fn update(
};
}
if scene_controller.single_image {
app_exit_writer.write(AppExit::Success);
commands.trigger(AppExit::Success);
}
}
} else {
Expand Down
6 changes: 3 additions & 3 deletions examples/ecs/ecs_guide.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,14 +164,14 @@ fn score_check_system(
fn game_over_system(
game_rules: Res<GameRules>,
game_state: Res<GameState>,
mut app_exit_events: EventWriter<AppExit>,
mut commands: Commands,
) {
if let Some(ref player) = game_state.winning_player {
println!("{player} won the game!");
app_exit_events.write(AppExit::Success);
commands.trigger(AppExit::Success);
} else if game_state.current_round == game_rules.max_rounds {
println!("Ran out of rounds. Nobody wins!");
app_exit_events.write(AppExit::Success);
commands.trigger(AppExit::Success);
}
}

Expand Down
3 changes: 1 addition & 2 deletions examples/ecs/observer_propagation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -107,7 +107,6 @@ fn take_damage(
trigger: On<Attack>,
mut hp: Query<(&mut HitPoints, &Name)>,
mut commands: Commands,
mut app_exit: EventWriter<AppExit>,
) {
let attack = trigger.event();
let (mut hp, name) = hp.get_mut(trigger.target()).unwrap();
Expand All @@ -118,7 +117,7 @@ fn take_damage(
} else {
warn!("💀 {} has died a gruesome death", name);
commands.entity(trigger.target()).despawn();
app_exit.write(AppExit::Success);
commands.trigger(AppExit::Success);
}

info!("(propagation reached root)\n");
Expand Down
Loading
Loading