From 035d5a85d9d98ebaded42facb19b1a78440d3b9a Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Wed, 12 Jun 2024 10:28:51 -0400 Subject: [PATCH] Ensure that events are updated even when using a bare-bones Bevy App (#13808) As discovered in https://github.com/Leafwing-Studios/leafwing-input-manager/issues/538, there appears to be some real weirdness going on in how event updates are processed between Bevy 0.13 and Bevy 0.14. To identify the cause and prevent regression, I've added tests to validate the intended behavior. My initial suspicion was that this would be fixed by https://github.com/bevyengine/bevy/pull/13762, but that doesn't seem to be the case. Instead, events appear to never be updated at all when using `bevy_app` by itself. This is part of the problem resolved by https://github.com/bevyengine/bevy/pull/11528, and introduced by https://github.com/bevyengine/bevy/pull/10077. After some investigation, it appears that `signal_event_update_system` is never added using a bare-bones `App`, and so event updates are always skipped. This can be worked around by adding your own copy to a later-in-the-frame schedule, but that's not a very good fix. Ensure that if we're not using a `FixedUpdate` schedule, events are always updated every frame. To do this, I've modified the logic of `event_update_condition` and `event_update_system` to clearly and correctly differentiate between the two cases: where we're waiting for a "you should update now" signal and where we simply don't care. To encode this, I've added the `ShouldUpdateEvents` enum, replacing a simple `bool` in `EventRegistry`'s `needs_update` field. Now, both tests pass as expected, without having to manually add a system! I've written two parallel unit tests to cover the intended behavior: 1. Test that `iter_current_update_events` works as expected in `bevy_ecs`. 2. Test that `iter_current_update_events` works as expected in `bevy_app` I've also added a test to verify that event updating works correctly in the presence of a fixed main schedule, and a second test to verify that fixed updating works at all to help future authors narrow down failures. - [x] figure out why the `bevy_app` version of this test fails but the `bevy_ecs` version does not - [x] figure out why `EventRegistry::run_updates` isn't working properly - [x] figure out why `EventRegistry::run_updates` is never getting called - [x] figure out why `event_update_condition` is always returning false - [x] figure out why `EventRegistry::needs_update` is always false - [x] verify that the problem is a missing `signal_events_update_system` --------- Co-authored-by: Mike --- crates/bevy_app/src/app.rs | 39 +++++++- crates/bevy_ecs/src/event.rs | 82 +++++++++++++++-- crates/bevy_ecs/src/lib.rs | 2 +- crates/bevy_time/src/lib.rs | 173 ++++++++++++++++++++++++++++++++++- 4 files changed, 281 insertions(+), 15 deletions(-) diff --git a/crates/bevy_app/src/app.rs b/crates/bevy_app/src/app.rs index ccee94ec544ee..1391250ac9b4f 100644 --- a/crates/bevy_app/src/app.rs +++ b/crates/bevy_app/src/app.rs @@ -925,7 +925,7 @@ mod tests { change_detection::{DetectChanges, ResMut}, component::Component, entity::Entity, - event::EventWriter, + event::{Event, EventWriter, Events}, query::With, removal_detection::RemovedComponents, schedule::{IntoSystemConfigs, ScheduleLabel}, @@ -1274,4 +1274,41 @@ mod tests { .init_non_send_resource::() .init_resource::(); } + + #[test] + fn events_should_be_updated_once_per_update() { + #[derive(Event, Clone)] + struct TestEvent; + + let mut app = App::new(); + app.add_event::(); + + // Starts empty + let test_events = app.world().resource::>(); + assert_eq!(test_events.len(), 0); + assert_eq!(test_events.iter_current_update_events().count(), 0); + app.update(); + + // Sending one event + app.world_mut().send_event(TestEvent); + + let test_events = app.world().resource::>(); + assert_eq!(test_events.len(), 1); + assert_eq!(test_events.iter_current_update_events().count(), 1); + app.update(); + + // Sending two events on the next frame + app.world_mut().send_event(TestEvent); + app.world_mut().send_event(TestEvent); + + let test_events = app.world().resource::>(); + assert_eq!(test_events.len(), 3); // Events are double-buffered, so we see 1 + 2 = 3 + assert_eq!(test_events.iter_current_update_events().count(), 2); + app.update(); + + // Sending zero events + let test_events = app.world().resource::>(); + assert_eq!(test_events.len(), 2); // Events are double-buffered, so we see 2 + 0 = 2 + assert_eq!(test_events.iter_current_update_events().count(), 0); + } } diff --git a/crates/bevy_ecs/src/event.rs b/crates/bevy_ecs/src/event.rs index 18f491a074d4d..25954c44edbdd 100644 --- a/crates/bevy_ecs/src/event.rs +++ b/crates/bevy_ecs/src/event.rs @@ -1010,10 +1010,25 @@ struct RegisteredEvent { /// to update all of the events. #[derive(Resource, Default)] pub struct EventRegistry { - needs_update: bool, + /// Should the events be updated? + /// + /// This field is generally automatically updated by the [`signal_event_update_system`](crate::event::update::signal_event_update_system). + pub should_update: ShouldUpdateEvents, event_updates: Vec, } +/// Controls whether or not the events in an [`EventRegistry`] should be updated. +#[derive(Default, Debug, Clone, Copy, PartialEq, Eq)] +pub enum ShouldUpdateEvents { + /// Without any fixed timestep, events should always be updated each frame. + #[default] + Always, + /// We need to wait until at least one pass of the fixed update schedules to update the events. + Waiting, + /// At least one pass of the fixed update schedules has occurred, and the events are ready to be updated. + Ready, +} + impl EventRegistry { /// Registers an event type to be updated. pub fn register_event(world: &mut World) { @@ -1058,9 +1073,12 @@ impl EventRegistry { pub struct EventUpdates; /// Signals the [`event_update_system`] to run after `FixedUpdate` systems. +/// +/// This will change the behavior of the [`EventRegistry`] to only run after a fixed update cycle has passed. +/// Normally, this will simply run every frame. pub fn signal_event_update_system(signal: Option>) { if let Some(mut registry) = signal { - registry.needs_update = true; + registry.should_update = ShouldUpdateEvents::Ready; } } @@ -1069,18 +1087,34 @@ pub fn event_update_system(world: &mut World, mut last_change_tick: Local) if world.contains_resource::() { world.resource_scope(|world, mut registry: Mut| { registry.run_updates(world, *last_change_tick); - // Disable the system until signal_event_update_system runs again. - registry.needs_update = false; + + registry.should_update = match registry.should_update { + // If we're always updating, keep doing so. + ShouldUpdateEvents::Always => ShouldUpdateEvents::Always, + // Disable the system until signal_event_update_system runs again. + ShouldUpdateEvents::Waiting | ShouldUpdateEvents::Ready => { + ShouldUpdateEvents::Waiting + } + }; }); } *last_change_tick = world.change_tick(); } /// A run condition for [`event_update_system`]. -pub fn event_update_condition(signal: Option>) -> bool { - // If we haven't got a signal to update the events, but we *could* get such a signal - // return early and update the events later. - signal.map_or(false, |signal| signal.needs_update) +/// +/// If [`signal_event_update_system`] has been run at least once, +/// we will wait for it to be run again before updating the events. +/// +/// Otherwise, we will always update the events. +pub fn event_update_condition(maybe_signal: Option>) -> bool { + match maybe_signal { + Some(signal) => match signal.should_update { + ShouldUpdateEvents::Always | ShouldUpdateEvents::Ready => true, + ShouldUpdateEvents::Waiting => false, + }, + None => true, + } } /// [`Iterator`] over sent [`EventIds`](`EventId`) from a batch. @@ -1541,4 +1575,36 @@ mod tests { }); schedule.run(&mut world); } + + #[test] + fn iter_current_update_events_iterates_over_current_events() { + #[derive(Event, Clone)] + struct TestEvent; + + let mut test_events = Events::::default(); + + // Starting empty + assert_eq!(test_events.len(), 0); + assert_eq!(test_events.iter_current_update_events().count(), 0); + test_events.update(); + + // Sending one event + test_events.send(TestEvent); + + assert_eq!(test_events.len(), 1); + assert_eq!(test_events.iter_current_update_events().count(), 1); + test_events.update(); + + // Sending two events on the next frame + test_events.send(TestEvent); + test_events.send(TestEvent); + + assert_eq!(test_events.len(), 3); // Events are double-buffered, so we see 1 + 2 = 3 + assert_eq!(test_events.iter_current_update_events().count(), 2); + test_events.update(); + + // Sending zero events + assert_eq!(test_events.len(), 2); // Events are double-buffered, so we see 2 + 0 = 2 + assert_eq!(test_events.iter_current_update_events().count(), 0); + } } diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index e6cabde398153..689ee8967e5d6 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/crates/bevy_ecs/src/lib.rs @@ -45,7 +45,7 @@ pub mod prelude { change_detection::{DetectChanges, DetectChangesMut, Mut, Ref}, component::Component, entity::{Entity, EntityMapper}, - event::{Event, EventReader, EventWriter, Events}, + event::{Event, EventReader, EventWriter, Events, ShouldUpdateEvents}, query::{Added, AnyOf, Changed, Has, Or, QueryBuilder, QueryState, With, Without}, removal_detection::RemovedComponents, schedule::{ diff --git a/crates/bevy_time/src/lib.rs b/crates/bevy_time/src/lib.rs index 9778e1517efc7..9b1bfc2e7635c 100644 --- a/crates/bevy_time/src/lib.rs +++ b/crates/bevy_time/src/lib.rs @@ -30,7 +30,7 @@ pub mod prelude { } use bevy_app::{prelude::*, RunFixedMainLoop}; -use bevy_ecs::event::signal_event_update_system; +use bevy_ecs::event::{signal_event_update_system, EventRegistry, ShouldUpdateEvents}; use bevy_ecs::prelude::*; use bevy_utils::{tracing::warn, Duration, Instant}; pub use crossbeam_channel::TrySendError; @@ -65,8 +65,11 @@ impl Plugin for TimePlugin { app.add_systems(First, time_system.in_set(TimeSystem)) .add_systems(RunFixedMainLoop, run_fixed_main_schedule); - // ensure the events are not dropped until `FixedMain` systems can observe them + // 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::(); + // 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; } } @@ -141,9 +144,13 @@ fn time_system( #[cfg(test)] mod tests { - use crate::{Fixed, Time, TimePlugin, TimeUpdateStrategy}; - use bevy_app::{App, Startup, Update}; - use bevy_ecs::event::{Event, EventReader, EventWriter}; + use crate::{Fixed, Time, TimePlugin, TimeUpdateStrategy, Virtual}; + use bevy_app::{App, FixedUpdate, Startup, Update}; + use bevy_ecs::{ + event::{Event, EventReader, EventRegistry, EventWriter, Events, ShouldUpdateEvents}, + system::{Local, Res, ResMut, Resource}, + }; + use bevy_utils::Duration; use std::error::Error; #[derive(Event)] @@ -159,6 +166,91 @@ mod tests { } } + #[derive(Event)] + struct DummyEvent; + + #[derive(Resource, Default)] + struct FixedUpdateCounter(u8); + + fn count_fixed_updates(mut counter: ResMut) { + counter.0 += 1; + } + + fn report_time( + mut frame_count: Local, + virtual_time: Res>, + fixed_time: Res>, + ) { + println!( + "Virtual time on frame {}: {:?}", + *frame_count, + virtual_time.elapsed() + ); + println!( + "Fixed time on frame {}: {:?}", + *frame_count, + fixed_time.elapsed() + ); + + *frame_count += 1; + } + + #[test] + fn fixed_main_schedule_should_run_with_time_plugin_enabled() { + // Set the time step to just over half the fixed update timestep + // This way, it will have not accumulated enough time to run the fixed update after one update + // But will definitely have enough time after two updates + let fixed_update_timestep = Time::::default().timestep(); + let time_step = fixed_update_timestep / 2 + Duration::from_millis(1); + + let mut app = App::new(); + app.add_plugins(TimePlugin) + .add_systems(FixedUpdate, count_fixed_updates) + .add_systems(Update, report_time) + .init_resource::() + .insert_resource(TimeUpdateStrategy::ManualDuration(time_step)); + + // Frame 0 + // Fixed update should not have run yet + app.update(); + + assert!(Duration::ZERO < fixed_update_timestep); + let counter = app.world().resource::(); + assert_eq!(counter.0, 0, "Fixed update should not have run yet"); + + // Frame 1 + // Fixed update should not have run yet + app.update(); + + assert!(time_step < fixed_update_timestep); + let counter = app.world().resource::(); + assert_eq!(counter.0, 0, "Fixed update should not have run yet"); + + // Frame 2 + // Fixed update should have run now + app.update(); + + assert!(2 * time_step > fixed_update_timestep); + let counter = app.world().resource::(); + assert_eq!(counter.0, 1, "Fixed update should have run once"); + + // Frame 3 + // Fixed update should have run exactly once still + app.update(); + + assert!(3 * time_step < 2 * fixed_update_timestep); + let counter = app.world().resource::(); + assert_eq!(counter.0, 1, "Fixed update should have run once"); + + // Frame 4 + // Fixed update should have run twice now + app.update(); + + assert!(4 * time_step > 2 * fixed_update_timestep); + let counter = app.world().resource::(); + assert_eq!(counter.0, 2, "Fixed update should have run twice"); + } + #[test] fn events_get_dropped_regression_test_11528() -> Result<(), impl Error> { let (tx1, rx1) = std::sync::mpsc::channel(); @@ -199,4 +291,75 @@ mod tests { // Check event type 2 has been dropped rx2.try_recv() } + + #[test] + fn event_update_should_wait_for_fixed_main() { + // Set the time step to just over half the fixed update timestep + // This way, it will have not accumulated enough time to run the fixed update after one update + // But will definitely have enough time after two updates + let fixed_update_timestep = Time::::default().timestep(); + let time_step = fixed_update_timestep / 2 + Duration::from_millis(1); + + fn send_event(mut events: ResMut>) { + events.send(DummyEvent); + } + + let mut app = App::new(); + app.add_plugins(TimePlugin) + .add_event::() + .init_resource::() + .add_systems(Startup, send_event) + .add_systems(FixedUpdate, count_fixed_updates) + .insert_resource(TimeUpdateStrategy::ManualDuration(time_step)); + + for frame in 0..10 { + app.update(); + let fixed_updates_seen = app.world().resource::().0; + let events = app.world().resource::>(); + let n_total_events = events.len(); + let n_current_events = events.iter_current_update_events().count(); + let event_registry = app.world().resource::(); + let should_update = event_registry.should_update; + + println!("Frame {frame}, {fixed_updates_seen} fixed updates seen. Should update: {should_update:?}"); + println!("Total events: {n_total_events} | Current events: {n_current_events}",); + + match frame { + 0 | 1 => { + assert_eq!(fixed_updates_seen, 0); + assert_eq!(n_total_events, 1); + assert_eq!(n_current_events, 1); + assert_eq!(should_update, ShouldUpdateEvents::Waiting); + } + 2 => { + assert_eq!(fixed_updates_seen, 1); // Time to trigger event updates + assert_eq!(n_total_events, 1); + assert_eq!(n_current_events, 1); + assert_eq!(should_update, ShouldUpdateEvents::Ready); // Prepping first update + } + 3 => { + assert_eq!(fixed_updates_seen, 1); + assert_eq!(n_total_events, 1); + assert_eq!(n_current_events, 0); // First update has occurred + assert_eq!(should_update, ShouldUpdateEvents::Waiting); + } + 4 => { + assert_eq!(fixed_updates_seen, 2); // Time to trigger the second update + assert_eq!(n_total_events, 1); + assert_eq!(n_current_events, 0); + assert_eq!(should_update, ShouldUpdateEvents::Ready); // Prepping second update + } + 5 => { + assert_eq!(fixed_updates_seen, 2); + assert_eq!(n_total_events, 0); // Second update has occurred + assert_eq!(n_current_events, 0); + assert_eq!(should_update, ShouldUpdateEvents::Waiting); + } + _ => { + assert_eq!(n_total_events, 0); // No more events are sent + assert_eq!(n_current_events, 0); + } + } + } + } }