diff --git a/crates/bevy_app/src/app.rs b/crates/bevy_app/src/app.rs index ccee94ec54..1391250ac9 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/collections.rs b/crates/bevy_ecs/src/event/collections.rs index 4370119c06..79f7b85668 100644 --- a/crates/bevy_ecs/src/event/collections.rs +++ b/crates/bevy_ecs/src/event/collections.rs @@ -366,3 +366,41 @@ impl ExactSizeIterator for SendBatchIds { self.event_count.saturating_sub(self.last_count) } } + +#[cfg(test)] +mod tests { + use crate::{self as bevy_ecs, event::Events}; + use bevy_ecs_macros::Event; + + #[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/event/mod.rs b/crates/bevy_ecs/src/event/mod.rs index 4aae28587d..1e8f89f28b 100644 --- a/crates/bevy_ecs/src/event/mod.rs +++ b/crates/bevy_ecs/src/event/mod.rs @@ -13,7 +13,7 @@ pub use bevy_ecs_macros::Event; pub use collections::{Events, SendBatchIds}; pub use iterators::{EventIterator, EventIteratorWithId, EventParIter}; pub use reader::{EventReader, ManualEventReader}; -pub use registry::EventRegistry; +pub use registry::{EventRegistry, ShouldUpdateEvents}; pub use update::{ event_update_condition, event_update_system, signal_event_update_system, EventUpdates, }; diff --git a/crates/bevy_ecs/src/event/registry.rs b/crates/bevy_ecs/src/event/registry.rs index 06587ddc35..2904214bde 100644 --- a/crates/bevy_ecs/src/event/registry.rs +++ b/crates/bevy_ecs/src/event/registry.rs @@ -17,14 +17,29 @@ struct RegisteredEvent { update: unsafe fn(MutUntyped), } -/// A registry of all of the [`Events`] in the [`World`], used by [`event_update_system`] +/// A registry of all of the [`Events`] in the [`World`], used by [`event_update_system`](crate::event::update::event_update_system) /// to update all of the events. #[derive(Resource, Default)] pub struct EventRegistry { - pub(super) 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) { diff --git a/crates/bevy_ecs/src/event/update.rs b/crates/bevy_ecs/src/event/update.rs index aa2d62bedc..535c309244 100644 --- a/crates/bevy_ecs/src/event/update.rs +++ b/crates/bevy_ecs/src/event/update.rs @@ -10,14 +10,19 @@ use bevy_ecs_macros::SystemSet; #[cfg(feature = "bevy_reflect")] use std::hash::Hash; +use super::registry::ShouldUpdateEvents; + #[doc(hidden)] #[derive(SystemSet, Clone, Debug, PartialEq, Eq, Hash)] 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; } } @@ -26,16 +31,32 @@ 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, + } } diff --git a/crates/bevy_time/src/lib.rs b/crates/bevy_time/src/lib.rs index 9778e1517e..9b1bfc2e76 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); + } + } + } + } }