From 3b0ad4cac76b3b8c3e1965cdb39cc938b1f27296 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 11:23:59 -0400 Subject: [PATCH 01/19] Add unit test --- crates/bevy_ecs/src/event/collections.rs | 38 ++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/crates/bevy_ecs/src/event/collections.rs b/crates/bevy_ecs/src/event/collections.rs index 4370119c065db..79f7b85668096 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); + } +} From c813e66c66f00f74e13e12aafd22e45fd3d1f557 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 11:29:49 -0400 Subject: [PATCH 02/19] Add bevy_app equivalent test --- crates/bevy_app/src/app.rs | 39 +++++++++++++++++++++++++++++++++++++- 1 file changed, 38 insertions(+), 1 deletion(-) 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); + } } From 806b615e2bdefe93d5dc9f0eafe6ab2a4641d136 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 12:14:03 -0400 Subject: [PATCH 03/19] println debugging --- crates/bevy_app/src/app.rs | 28 +++++++++++++----------- crates/bevy_app/src/sub_app.rs | 1 + crates/bevy_ecs/src/event/collections.rs | 2 ++ crates/bevy_ecs/src/event/registry.rs | 7 +++++- crates/bevy_ecs/src/event/update.rs | 5 +++++ 5 files changed, 29 insertions(+), 14 deletions(-) diff --git a/crates/bevy_app/src/app.rs b/crates/bevy_app/src/app.rs index 1391250ac9b4f..c93d4e099b8e7 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::{Event, EventWriter, Events}, + event::{Event, EventWriter}, query::With, removal_detection::RemovedComponents, schedule::{IntoSystemConfigs, ScheduleLabel}, @@ -1280,35 +1280,37 @@ mod tests { #[derive(Event, Clone)] struct TestEvent; + println!("Starting test"); + 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); + //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); + //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); + //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); + //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_app/src/sub_app.rs b/crates/bevy_app/src/sub_app.rs index 59ad4eca8b105..e60029323e165 100644 --- a/crates/bevy_app/src/sub_app.rs +++ b/crates/bevy_app/src/sub_app.rs @@ -307,6 +307,7 @@ impl SubApp { { if !self.world.contains_resource::>() { EventRegistry::register_event::(self.world_mut()); + println!("Registered event: {:?}", std::any::type_name::()); } self diff --git a/crates/bevy_ecs/src/event/collections.rs b/crates/bevy_ecs/src/event/collections.rs index 79f7b85668096..37e4f5c295d42 100644 --- a/crates/bevy_ecs/src/event/collections.rs +++ b/crates/bevy_ecs/src/event/collections.rs @@ -172,6 +172,8 @@ impl Events { /// /// If you need access to the events that were removed, consider using [`Events::update_drain`]. pub fn update(&mut self) { + println!("Updating events!"); + std::mem::swap(&mut self.events_a, &mut self.events_b); self.events_b.clear(); self.events_b.start_event_count = self.event_count; diff --git a/crates/bevy_ecs/src/event/registry.rs b/crates/bevy_ecs/src/event/registry.rs index 06587ddc35aa7..472e3080664d4 100644 --- a/crates/bevy_ecs/src/event/registry.rs +++ b/crates/bevy_ecs/src/event/registry.rs @@ -8,6 +8,7 @@ use bevy_ecs::{ }; #[doc(hidden)] +#[derive(Debug)] struct RegisteredEvent { component_id: ComponentId, // Required to flush the secondary buffer and drop events even if left unchanged. @@ -19,7 +20,7 @@ struct RegisteredEvent { /// A registry of all of the [`Events`] in the [`World`], used by [`event_update_system`] /// to update all of the events. -#[derive(Resource, Default)] +#[derive(Resource, Default, Debug)] pub struct EventRegistry { pub(super) needs_update: bool, event_updates: Vec, @@ -46,7 +47,11 @@ impl EventRegistry { /// Updates all of the registered events in the World. pub fn run_updates(&mut self, world: &mut World, last_change_tick: Tick) { + println!("Running event updates"); + for registered_event in &mut self.event_updates { + println!("Checking if we should update an event"); + // Bypass the type ID -> Component ID lookup with the cached component ID. if let Some(events) = world.get_resource_mut_by_id(registered_event.component_id) { let has_changed = events.has_changed_since(last_change_tick); diff --git a/crates/bevy_ecs/src/event/update.rs b/crates/bevy_ecs/src/event/update.rs index aa2d62bedc46b..ca3beb5a9b7f1 100644 --- a/crates/bevy_ecs/src/event/update.rs +++ b/crates/bevy_ecs/src/event/update.rs @@ -23,6 +23,7 @@ pub fn signal_event_update_system(signal: Option>) { /// A system that calls [`Events::update`] on all registered [`Events`] in the world. pub fn event_update_system(world: &mut World, mut last_change_tick: Local) { + println!("Running event_update_system"); if world.contains_resource::() { world.resource_scope(|world, mut registry: Mut| { registry.run_updates(world, *last_change_tick); @@ -35,6 +36,10 @@ pub fn event_update_system(world: &mut World, mut last_change_tick: Local) /// A run condition for [`event_update_system`]. pub fn event_update_condition(signal: Option>) -> bool { + println!("Checking if we should run event_update_system"); + + println!("signal: {:?}", signal); + // 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) From d97bc72e3eb3b1cd8a85b48a93f4e433ae375e73 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 12:42:08 -0400 Subject: [PATCH 04/19] Add signal_event_update_system to test --- crates/bevy_app/src/app.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/crates/bevy_app/src/app.rs b/crates/bevy_app/src/app.rs index c93d4e099b8e7..44a4622222618 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::{Event, EventWriter}, + event::{signal_event_update_system, Event, EventWriter}, query::With, removal_detection::RemovedComponents, schedule::{IntoSystemConfigs, ScheduleLabel}, @@ -933,7 +933,7 @@ mod tests { world::{FromWorld, World}, }; - use crate::{App, AppExit, Plugin, SubApp, Update}; + use crate::{App, AppExit, Last, Plugin, SubApp, Update}; struct PluginA; impl Plugin for PluginA { @@ -1283,6 +1283,8 @@ mod tests { println!("Starting test"); let mut app = App::new(); + // Without minimal plugins, we need to make sure that this system is added + app.add_systems(Last, signal_event_update_system); app.add_event::(); // Starts empty From e8b5384fbd8503541b7d3374044483d8614f2d55 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 12:43:29 -0400 Subject: [PATCH 05/19] Revert "println debugging" This reverts commit 806b615e2bdefe93d5dc9f0eafe6ab2a4641d136. --- crates/bevy_app/src/app.rs | 28 +++++++++++------------- crates/bevy_app/src/sub_app.rs | 1 - crates/bevy_ecs/src/event/collections.rs | 2 -- crates/bevy_ecs/src/event/registry.rs | 7 +----- crates/bevy_ecs/src/event/update.rs | 5 ----- 5 files changed, 14 insertions(+), 29 deletions(-) diff --git a/crates/bevy_app/src/app.rs b/crates/bevy_app/src/app.rs index 44a4622222618..3575f6b734350 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::{signal_event_update_system, Event, EventWriter}, + event::{signal_event_update_system, Event, EventWriter, Events}, query::With, removal_detection::RemovedComponents, schedule::{IntoSystemConfigs, ScheduleLabel}, @@ -1280,39 +1280,37 @@ mod tests { #[derive(Event, Clone)] struct TestEvent; - println!("Starting test"); - let mut app = App::new(); // Without minimal plugins, we need to make sure that this system is added app.add_systems(Last, signal_event_update_system); 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); + 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); + 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); + 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); + 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_app/src/sub_app.rs b/crates/bevy_app/src/sub_app.rs index e60029323e165..59ad4eca8b105 100644 --- a/crates/bevy_app/src/sub_app.rs +++ b/crates/bevy_app/src/sub_app.rs @@ -307,7 +307,6 @@ impl SubApp { { if !self.world.contains_resource::>() { EventRegistry::register_event::(self.world_mut()); - println!("Registered event: {:?}", std::any::type_name::()); } self diff --git a/crates/bevy_ecs/src/event/collections.rs b/crates/bevy_ecs/src/event/collections.rs index 37e4f5c295d42..79f7b85668096 100644 --- a/crates/bevy_ecs/src/event/collections.rs +++ b/crates/bevy_ecs/src/event/collections.rs @@ -172,8 +172,6 @@ impl Events { /// /// If you need access to the events that were removed, consider using [`Events::update_drain`]. pub fn update(&mut self) { - println!("Updating events!"); - std::mem::swap(&mut self.events_a, &mut self.events_b); self.events_b.clear(); self.events_b.start_event_count = self.event_count; diff --git a/crates/bevy_ecs/src/event/registry.rs b/crates/bevy_ecs/src/event/registry.rs index 472e3080664d4..06587ddc35aa7 100644 --- a/crates/bevy_ecs/src/event/registry.rs +++ b/crates/bevy_ecs/src/event/registry.rs @@ -8,7 +8,6 @@ use bevy_ecs::{ }; #[doc(hidden)] -#[derive(Debug)] struct RegisteredEvent { component_id: ComponentId, // Required to flush the secondary buffer and drop events even if left unchanged. @@ -20,7 +19,7 @@ struct RegisteredEvent { /// A registry of all of the [`Events`] in the [`World`], used by [`event_update_system`] /// to update all of the events. -#[derive(Resource, Default, Debug)] +#[derive(Resource, Default)] pub struct EventRegistry { pub(super) needs_update: bool, event_updates: Vec, @@ -47,11 +46,7 @@ impl EventRegistry { /// Updates all of the registered events in the World. pub fn run_updates(&mut self, world: &mut World, last_change_tick: Tick) { - println!("Running event updates"); - for registered_event in &mut self.event_updates { - println!("Checking if we should update an event"); - // Bypass the type ID -> Component ID lookup with the cached component ID. if let Some(events) = world.get_resource_mut_by_id(registered_event.component_id) { let has_changed = events.has_changed_since(last_change_tick); diff --git a/crates/bevy_ecs/src/event/update.rs b/crates/bevy_ecs/src/event/update.rs index ca3beb5a9b7f1..aa2d62bedc46b 100644 --- a/crates/bevy_ecs/src/event/update.rs +++ b/crates/bevy_ecs/src/event/update.rs @@ -23,7 +23,6 @@ pub fn signal_event_update_system(signal: Option>) { /// A system that calls [`Events::update`] on all registered [`Events`] in the world. pub fn event_update_system(world: &mut World, mut last_change_tick: Local) { - println!("Running event_update_system"); if world.contains_resource::() { world.resource_scope(|world, mut registry: Mut| { registry.run_updates(world, *last_change_tick); @@ -36,10 +35,6 @@ pub fn event_update_system(world: &mut World, mut last_change_tick: Local) /// A run condition for [`event_update_system`]. pub fn event_update_condition(signal: Option>) -> bool { - println!("Checking if we should run event_update_system"); - - println!("signal: {:?}", signal); - // 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) From 1abf336937492ddc9ec7e463ffd84bcbd72463d8 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 13:08:45 -0400 Subject: [PATCH 06/19] Fix behavior when running without FixedUpdate schedules --- crates/bevy_app/src/app.rs | 6 ++--- crates/bevy_ecs/src/event/registry.rs | 14 ++++++++++- crates/bevy_ecs/src/event/update.rs | 35 +++++++++++++++++++++------ 3 files changed, 43 insertions(+), 12 deletions(-) diff --git a/crates/bevy_app/src/app.rs b/crates/bevy_app/src/app.rs index 3575f6b734350..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::{signal_event_update_system, Event, EventWriter, Events}, + event::{Event, EventWriter, Events}, query::With, removal_detection::RemovedComponents, schedule::{IntoSystemConfigs, ScheduleLabel}, @@ -933,7 +933,7 @@ mod tests { world::{FromWorld, World}, }; - use crate::{App, AppExit, Last, Plugin, SubApp, Update}; + use crate::{App, AppExit, Plugin, SubApp, Update}; struct PluginA; impl Plugin for PluginA { @@ -1281,8 +1281,6 @@ mod tests { struct TestEvent; let mut app = App::new(); - // Without minimal plugins, we need to make sure that this system is added - app.add_systems(Last, signal_event_update_system); app.add_event::(); // Starts empty diff --git a/crates/bevy_ecs/src/event/registry.rs b/crates/bevy_ecs/src/event/registry.rs index 06587ddc35aa7..599b94ee33e7f 100644 --- a/crates/bevy_ecs/src/event/registry.rs +++ b/crates/bevy_ecs/src/event/registry.rs @@ -21,10 +21,22 @@ struct RegisteredEvent { /// to update all of the events. #[derive(Resource, Default)] pub struct EventRegistry { - pub(super) needs_update: bool, + pub(super) needs_update: ShouldUpdateEvents, event_updates: Vec, } +/// Controls whether or not the events should be updated. +#[derive(Default)] +pub(crate) enum ShouldUpdateEvents { + /// Without any fixed timestep shenanigans, events should always be updated each frame. + #[default] + AlwaysUpdate, + /// We need to wait until at least one pass of the fixed update schedules to update the events. + WaitingToUpdate, + /// At least one pass of the fixed update schedules has occurred, and the events are ready to be updated. + ReadyToUpdate, +} + 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 aa2d62bedc46b..423d668d7c84c 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.needs_update = ShouldUpdateEvents::ReadyToUpdate; } } @@ -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.needs_update = match registry.needs_update { + // If we're always updating, keep doing so. + ShouldUpdateEvents::AlwaysUpdate => ShouldUpdateEvents::AlwaysUpdate, + // Disable the system until signal_event_update_system runs again. + ShouldUpdateEvents::WaitingToUpdate | ShouldUpdateEvents::ReadyToUpdate => { + ShouldUpdateEvents::WaitingToUpdate + } + }; }); } *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.needs_update { + ShouldUpdateEvents::AlwaysUpdate | ShouldUpdateEvents::ReadyToUpdate => true, + ShouldUpdateEvents::WaitingToUpdate => false, + }, + None => true, + } } From ecf40861709f138b63ad8af52782ca75db1abbc3 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 13:14:38 -0400 Subject: [PATCH 07/19] Clippy style police --- crates/bevy_ecs/src/event/registry.rs | 6 +++--- crates/bevy_ecs/src/event/update.rs | 12 ++++++------ 2 files changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/src/event/registry.rs b/crates/bevy_ecs/src/event/registry.rs index 599b94ee33e7f..dfd7690f4ebd8 100644 --- a/crates/bevy_ecs/src/event/registry.rs +++ b/crates/bevy_ecs/src/event/registry.rs @@ -30,11 +30,11 @@ pub struct EventRegistry { pub(crate) enum ShouldUpdateEvents { /// Without any fixed timestep shenanigans, events should always be updated each frame. #[default] - AlwaysUpdate, + Always, /// We need to wait until at least one pass of the fixed update schedules to update the events. - WaitingToUpdate, + Waiting, /// At least one pass of the fixed update schedules has occurred, and the events are ready to be updated. - ReadyToUpdate, + Ready, } impl EventRegistry { diff --git a/crates/bevy_ecs/src/event/update.rs b/crates/bevy_ecs/src/event/update.rs index 423d668d7c84c..ecd3e2934dd4a 100644 --- a/crates/bevy_ecs/src/event/update.rs +++ b/crates/bevy_ecs/src/event/update.rs @@ -22,7 +22,7 @@ pub struct EventUpdates; /// Normally, this will simply run every frame. pub fn signal_event_update_system(signal: Option>) { if let Some(mut registry) = signal { - registry.needs_update = ShouldUpdateEvents::ReadyToUpdate; + registry.needs_update = ShouldUpdateEvents::Ready; } } @@ -34,10 +34,10 @@ pub fn event_update_system(world: &mut World, mut last_change_tick: Local) registry.needs_update = match registry.needs_update { // If we're always updating, keep doing so. - ShouldUpdateEvents::AlwaysUpdate => ShouldUpdateEvents::AlwaysUpdate, + ShouldUpdateEvents::Always => ShouldUpdateEvents::Always, // Disable the system until signal_event_update_system runs again. - ShouldUpdateEvents::WaitingToUpdate | ShouldUpdateEvents::ReadyToUpdate => { - ShouldUpdateEvents::WaitingToUpdate + ShouldUpdateEvents::Waiting | ShouldUpdateEvents::Ready => { + ShouldUpdateEvents::Waiting } }; }); @@ -54,8 +54,8 @@ pub fn event_update_system(world: &mut World, mut last_change_tick: Local) pub fn event_update_condition(maybe_signal: Option>) -> bool { match maybe_signal { Some(signal) => match signal.needs_update { - ShouldUpdateEvents::AlwaysUpdate | ShouldUpdateEvents::ReadyToUpdate => true, - ShouldUpdateEvents::WaitingToUpdate => false, + ShouldUpdateEvents::Always | ShouldUpdateEvents::Ready => true, + ShouldUpdateEvents::Waiting => false, }, None => true, } From 3f3f3911273913a5ad71282c65f70025533d09a7 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 13:25:47 -0400 Subject: [PATCH 08/19] Reduce invective Co-authored-by: Mike --- crates/bevy_ecs/src/event/registry.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/event/registry.rs b/crates/bevy_ecs/src/event/registry.rs index dfd7690f4ebd8..31ca8ef146b57 100644 --- a/crates/bevy_ecs/src/event/registry.rs +++ b/crates/bevy_ecs/src/event/registry.rs @@ -28,7 +28,7 @@ pub struct EventRegistry { /// Controls whether or not the events should be updated. #[derive(Default)] pub(crate) enum ShouldUpdateEvents { - /// Without any fixed timestep shenanigans, events should always be updated each frame. + /// 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. From 8c850e108868482740ec601024dc422ef486db9a Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 14:11:24 -0400 Subject: [PATCH 09/19] Add test for fixedupdate event logic --- crates/bevy_time/src/lib.rs | 66 ++++++++++++++++++++++++++++++++++++- 1 file changed, 65 insertions(+), 1 deletion(-) diff --git a/crates/bevy_time/src/lib.rs b/crates/bevy_time/src/lib.rs index 9778e1517efc7..844cfab0ab0c6 100644 --- a/crates/bevy_time/src/lib.rs +++ b/crates/bevy_time/src/lib.rs @@ -143,7 +143,8 @@ fn time_system( mod tests { use crate::{Fixed, Time, TimePlugin, TimeUpdateStrategy}; use bevy_app::{App, Startup, Update}; - use bevy_ecs::event::{Event, EventReader, EventWriter}; + use bevy_ecs::event::{Event, EventReader, EventWriter, Events}; + use bevy_utils::Duration; use std::error::Error; #[derive(Event)] @@ -159,6 +160,9 @@ mod tests { } } + #[derive(Event)] + struct DummyEvent; + #[test] fn events_get_dropped_regression_test_11528() -> Result<(), impl Error> { let (tx1, rx1) = std::sync::mpsc::channel(); @@ -199,4 +203,64 @@ mod tests { // Check event type 2 has been dropped rx2.try_recv() } + + #[test] + fn events_should_wait_for_fixed_time_update() { + // 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_event::(); + app.add_plugins(TimePlugin::default()); + app.insert_resource(TimeUpdateStrategy::ManualDuration(time_step)); + + // Start with 0 events + let events = app.world().resource::>(); + assert_eq!(events.len(), 0); + + // Send an event + app.world_mut().send_event(DummyEvent); + let events = app.world().resource::>(); + assert_eq!(events.len(), 1); + assert_eq!(events.iter_current_update_events().count(), 1); + + // Update the app by a single timestep + // Fixed update should not have run yet + app.update(); + assert!(time_step < fixed_update_timestep); + + let events = app.world().resource::>(); + assert_eq!(events.len(), 1); + assert_eq!(events.iter_current_update_events().count(), 1); + + // Update the app by another timestep + // Fixed update should have run now + app.update(); + assert!(2 * time_step > fixed_update_timestep); + + let events = app.world().resource::>(); + assert_eq!(events.len(), 1); + assert_eq!(events.iter_current_update_events().count(), 0); + + // Update the app by another timestep + // Fixed update should have run exactly once still + app.update(); + assert!(3 * time_step < 2 * fixed_update_timestep); + + let events = app.world().resource::>(); + assert_eq!(events.len(), 1); + assert_eq!(events.iter_current_update_events().count(), 0); + + // Update the app by another timestep + // Fixed update should have run twice now + app.update(); + assert!(4 * time_step > 2 * fixed_update_timestep); + + let events = app.world().resource::>(); + assert_eq!(events.len(), 0); + assert_eq!(events.iter_current_update_events().count(), 0); + } } From e0dea5778c5de92a781ab8e3a662d4da10ee84d9 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 14:26:43 -0400 Subject: [PATCH 10/19] Verify that FixedUpdate is even running in test --- crates/bevy_time/src/lib.rs | 26 ++++++++++++++++++++++++-- 1 file changed, 24 insertions(+), 2 deletions(-) diff --git a/crates/bevy_time/src/lib.rs b/crates/bevy_time/src/lib.rs index 844cfab0ab0c6..e9fa98c7c585f 100644 --- a/crates/bevy_time/src/lib.rs +++ b/crates/bevy_time/src/lib.rs @@ -142,8 +142,11 @@ 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, Events}; + use bevy_app::{App, FixedUpdate, Startup, Update}; + use bevy_ecs::{ + event::{Event, EventReader, EventWriter, Events}, + system::{ResMut, Resource}, + }; use bevy_utils::Duration; use std::error::Error; @@ -163,6 +166,9 @@ mod tests { #[derive(Event)] struct DummyEvent; + #[derive(Resource, Default)] + struct FixedUpdateCounter(u8); + #[test] fn events_get_dropped_regression_test_11528() -> Result<(), impl Error> { let (tx1, rx1) = std::sync::mpsc::channel(); @@ -212,14 +218,22 @@ mod tests { let fixed_update_timestep = Time::::default().timestep(); let time_step = fixed_update_timestep / 2 + Duration::from_millis(1); + fn count_fixed_updates(mut counter: ResMut) { + counter.0 += 1; + } + let mut app = App::new(); app.add_event::(); + app.init_resource::(); + app.add_systems(FixedUpdate, count_fixed_updates); app.add_plugins(TimePlugin::default()); app.insert_resource(TimeUpdateStrategy::ManualDuration(time_step)); // Start with 0 events let events = app.world().resource::>(); assert_eq!(events.len(), 0); + let counter = app.world().resource::(); + assert_eq!(counter.0, 0); // Send an event app.world_mut().send_event(DummyEvent); @@ -231,6 +245,8 @@ mod tests { // 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"); let events = app.world().resource::>(); assert_eq!(events.len(), 1); @@ -240,6 +256,8 @@ mod tests { // 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"); let events = app.world().resource::>(); assert_eq!(events.len(), 1); @@ -249,6 +267,8 @@ mod tests { // 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"); let events = app.world().resource::>(); assert_eq!(events.len(), 1); @@ -258,6 +278,8 @@ mod tests { // 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"); let events = app.world().resource::>(); assert_eq!(events.len(), 0); From 0655dbe1f14dbbecd29118a854b084ad9e0e3112 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 14:30:26 -0400 Subject: [PATCH 11/19] Initialize to ShouldUpdateEvents::Waiting in TimePlugin exists --- crates/bevy_ecs/src/event/mod.rs | 2 +- crates/bevy_ecs/src/event/registry.rs | 11 +++++++---- crates/bevy_ecs/src/event/update.rs | 6 +++--- crates/bevy_time/src/lib.rs | 7 +++++-- 4 files changed, 16 insertions(+), 10 deletions(-) diff --git a/crates/bevy_ecs/src/event/mod.rs b/crates/bevy_ecs/src/event/mod.rs index 4aae28587d6eb..1e8f89f28b8eb 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 31ca8ef146b57..fbfb53d7403c7 100644 --- a/crates/bevy_ecs/src/event/registry.rs +++ b/crates/bevy_ecs/src/event/registry.rs @@ -17,17 +17,20 @@ 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: ShouldUpdateEvents, + /// 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 should be updated. +/// Controls whether or not the events in an [`EventRegistry`] should be updated. #[derive(Default)] -pub(crate) enum ShouldUpdateEvents { +pub enum ShouldUpdateEvents { /// Without any fixed timestep, events should always be updated each frame. #[default] Always, diff --git a/crates/bevy_ecs/src/event/update.rs b/crates/bevy_ecs/src/event/update.rs index ecd3e2934dd4a..535c3092445a7 100644 --- a/crates/bevy_ecs/src/event/update.rs +++ b/crates/bevy_ecs/src/event/update.rs @@ -22,7 +22,7 @@ pub struct EventUpdates; /// Normally, this will simply run every frame. pub fn signal_event_update_system(signal: Option>) { if let Some(mut registry) = signal { - registry.needs_update = ShouldUpdateEvents::Ready; + registry.should_update = ShouldUpdateEvents::Ready; } } @@ -32,7 +32,7 @@ pub fn event_update_system(world: &mut World, mut last_change_tick: Local) world.resource_scope(|world, mut registry: Mut| { registry.run_updates(world, *last_change_tick); - registry.needs_update = match registry.needs_update { + 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. @@ -53,7 +53,7 @@ pub fn event_update_system(world: &mut World, mut last_change_tick: Local) /// Otherwise, we will always update the events. pub fn event_update_condition(maybe_signal: Option>) -> bool { match maybe_signal { - Some(signal) => match signal.needs_update { + Some(signal) => match signal.should_update { ShouldUpdateEvents::Always | ShouldUpdateEvents::Ready => true, ShouldUpdateEvents::Waiting => false, }, diff --git a/crates/bevy_time/src/lib.rs b/crates/bevy_time/src/lib.rs index e9fa98c7c585f..ddda8993bd905 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; } } From d2ea11e660b2ac9220dd6baae98175d2d37f159b Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 14:55:53 -0400 Subject: [PATCH 12/19] Create a test just to verify that FixedMain schedule is running as expected --- crates/bevy_time/src/lib.rs | 67 ++++++++++++++++++++++++++----------- 1 file changed, 47 insertions(+), 20 deletions(-) diff --git a/crates/bevy_time/src/lib.rs b/crates/bevy_time/src/lib.rs index ddda8993bd905..54af18bc4aa59 100644 --- a/crates/bevy_time/src/lib.rs +++ b/crates/bevy_time/src/lib.rs @@ -172,6 +172,53 @@ mod tests { #[derive(Resource, Default)] struct FixedUpdateCounter(u8); + fn count_fixed_updates(mut counter: ResMut) { + counter.0 += 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) + .init_resource::() + .insert_resource(TimeUpdateStrategy::ManualDuration(time_step)); + + // Update the app by a single timestep + // 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"); + + // Update the app by another timestep + // 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"); + + // Update the app by another timestep + // 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"); + + // Update the app by another timestep + // 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(); @@ -221,22 +268,14 @@ mod tests { let fixed_update_timestep = Time::::default().timestep(); let time_step = fixed_update_timestep / 2 + Duration::from_millis(1); - fn count_fixed_updates(mut counter: ResMut) { - counter.0 += 1; - } - let mut app = App::new(); app.add_event::(); - app.init_resource::(); - app.add_systems(FixedUpdate, count_fixed_updates); app.add_plugins(TimePlugin::default()); app.insert_resource(TimeUpdateStrategy::ManualDuration(time_step)); // Start with 0 events let events = app.world().resource::>(); assert_eq!(events.len(), 0); - let counter = app.world().resource::(); - assert_eq!(counter.0, 0); // Send an event app.world_mut().send_event(DummyEvent); @@ -247,9 +286,6 @@ mod tests { // Update the app by a single timestep // 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"); let events = app.world().resource::>(); assert_eq!(events.len(), 1); @@ -258,9 +294,6 @@ mod tests { // Update the app by another timestep // 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"); let events = app.world().resource::>(); assert_eq!(events.len(), 1); @@ -269,9 +302,6 @@ mod tests { // Update the app by another timestep // 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"); let events = app.world().resource::>(); assert_eq!(events.len(), 1); @@ -280,9 +310,6 @@ mod tests { // Update the app by another timestep // 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"); let events = app.world().resource::>(); assert_eq!(events.len(), 0); From ffe07987eb040748b4a19f11533ec572a3062542 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 14:58:49 -0400 Subject: [PATCH 13/19] Add logging to test --- crates/bevy_time/src/lib.rs | 15 +++++++++++++++ 1 file changed, 15 insertions(+) diff --git a/crates/bevy_time/src/lib.rs b/crates/bevy_time/src/lib.rs index 54af18bc4aa59..f0963c700a1d5 100644 --- a/crates/bevy_time/src/lib.rs +++ b/crates/bevy_time/src/lib.rs @@ -190,9 +190,15 @@ mod tests { .init_resource::() .insert_resource(TimeUpdateStrategy::ManualDuration(time_step)); + let fixed_time = app.world().resource::>(); + println!("Fixed time on frame 0: {:?}", fixed_time); + // Update the app by a single timestep // Fixed update should not have run yet app.update(); + let fixed_time = app.world().resource::>(); + println!("Fixed time on frame 1: {:?}", fixed_time); + assert!(time_step < fixed_update_timestep); let counter = app.world().resource::(); assert_eq!(counter.0, 0, "Fixed update should not have run yet"); @@ -200,6 +206,9 @@ mod tests { // Update the app by another timestep // Fixed update should have run now app.update(); + let fixed_time = app.world().resource::>(); + println!("Fixed time on frame 2: {:?}", fixed_time); + assert!(2 * time_step > fixed_update_timestep); let counter = app.world().resource::(); assert_eq!(counter.0, 1, "Fixed update should have run once"); @@ -207,6 +216,9 @@ mod tests { // Update the app by another timestep // Fixed update should have run exactly once still app.update(); + let fixed_time = app.world().resource::>(); + println!("Fixed time on frame 3: {:?}", fixed_time); + assert!(3 * time_step < 2 * fixed_update_timestep); let counter = app.world().resource::(); assert_eq!(counter.0, 1, "Fixed update should have run once"); @@ -214,6 +226,9 @@ mod tests { // Update the app by another timestep // Fixed update should have run twice now app.update(); + let fixed_time = app.world().resource::>(); + println!("Fixed time on frame 4: {:?}", fixed_time); + assert!(4 * time_step > 2 * fixed_update_timestep); let counter = app.world().resource::(); assert_eq!(counter.0, 2, "Fixed update should have run twice"); From 5fef8cfa981cfc5b2d099f07bb1dc53aa3f090d1 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 15:14:18 -0400 Subject: [PATCH 14/19] Add better logging for tests --- crates/bevy_time/src/lib.rs | 36 +++++++++++++++++++++++------------- 1 file changed, 23 insertions(+), 13 deletions(-) diff --git a/crates/bevy_time/src/lib.rs b/crates/bevy_time/src/lib.rs index f0963c700a1d5..bfef80ae2fdb2 100644 --- a/crates/bevy_time/src/lib.rs +++ b/crates/bevy_time/src/lib.rs @@ -144,11 +144,11 @@ fn time_system( #[cfg(test)] mod tests { - use crate::{Fixed, Time, TimePlugin, TimeUpdateStrategy}; + use crate::{Fixed, Time, TimePlugin, TimeUpdateStrategy, Virtual}; use bevy_app::{App, FixedUpdate, Startup, Update}; use bevy_ecs::{ event::{Event, EventReader, EventWriter, Events}, - system::{ResMut, Resource}, + system::{Local, Res, ResMut, Resource}, }; use bevy_utils::Duration; use std::error::Error; @@ -176,6 +176,25 @@ mod tests { 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 @@ -187,17 +206,13 @@ mod tests { 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)); - let fixed_time = app.world().resource::>(); - println!("Fixed time on frame 0: {:?}", fixed_time); - // Update the app by a single timestep // Fixed update should not have run yet app.update(); - let fixed_time = app.world().resource::>(); - println!("Fixed time on frame 1: {:?}", fixed_time); assert!(time_step < fixed_update_timestep); let counter = app.world().resource::(); @@ -206,8 +221,6 @@ mod tests { // Update the app by another timestep // Fixed update should have run now app.update(); - let fixed_time = app.world().resource::>(); - println!("Fixed time on frame 2: {:?}", fixed_time); assert!(2 * time_step > fixed_update_timestep); let counter = app.world().resource::(); @@ -216,8 +229,6 @@ mod tests { // Update the app by another timestep // Fixed update should have run exactly once still app.update(); - let fixed_time = app.world().resource::>(); - println!("Fixed time on frame 3: {:?}", fixed_time); assert!(3 * time_step < 2 * fixed_update_timestep); let counter = app.world().resource::(); @@ -226,8 +237,6 @@ mod tests { // Update the app by another timestep // Fixed update should have run twice now app.update(); - let fixed_time = app.world().resource::>(); - println!("Fixed time on frame 4: {:?}", fixed_time); assert!(4 * time_step > 2 * fixed_update_timestep); let counter = app.world().resource::(); @@ -286,6 +295,7 @@ mod tests { let mut app = App::new(); app.add_event::(); app.add_plugins(TimePlugin::default()); + app.add_systems(Update, report_time); app.insert_resource(TimeUpdateStrategy::ManualDuration(time_step)); // Start with 0 events From 985f5999b10ba22b914f6fbed3f915e5c016384b Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 15:16:40 -0400 Subject: [PATCH 15/19] Fix fixed_main_schedule test --- crates/bevy_time/src/lib.rs | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/crates/bevy_time/src/lib.rs b/crates/bevy_time/src/lib.rs index bfef80ae2fdb2..cdfcec97dcfec 100644 --- a/crates/bevy_time/src/lib.rs +++ b/crates/bevy_time/src/lib.rs @@ -210,7 +210,15 @@ mod tests { .init_resource::() .insert_resource(TimeUpdateStrategy::ManualDuration(time_step)); - // Update the app by a single timestep + // 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(); @@ -218,7 +226,7 @@ mod tests { let counter = app.world().resource::(); assert_eq!(counter.0, 0, "Fixed update should not have run yet"); - // Update the app by another timestep + // Frame 2 // Fixed update should have run now app.update(); @@ -226,7 +234,7 @@ mod tests { let counter = app.world().resource::(); assert_eq!(counter.0, 1, "Fixed update should have run once"); - // Update the app by another timestep + // Frame 3 // Fixed update should have run exactly once still app.update(); @@ -234,7 +242,7 @@ mod tests { let counter = app.world().resource::(); assert_eq!(counter.0, 1, "Fixed update should have run once"); - // Update the app by another timestep + // Frame 4 // Fixed update should have run twice now app.update(); From f48162ab15407d1f8a9144606aebd2998142fd75 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 15:30:55 -0400 Subject: [PATCH 16/19] Rewrite test for clarity --- crates/bevy_time/src/lib.rs | 47 ++++++++++++++++++++----------------- 1 file changed, 25 insertions(+), 22 deletions(-) diff --git a/crates/bevy_time/src/lib.rs b/crates/bevy_time/src/lib.rs index cdfcec97dcfec..6aa472e3b8d5f 100644 --- a/crates/bevy_time/src/lib.rs +++ b/crates/bevy_time/src/lib.rs @@ -293,59 +293,62 @@ mod tests { } #[test] - fn events_should_wait_for_fixed_time_update() { + 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_event::(); - app.add_plugins(TimePlugin::default()); - app.add_systems(Update, report_time); - app.insert_resource(TimeUpdateStrategy::ManualDuration(time_step)); + app.add_plugins(TimePlugin) + .add_systems(Startup, send_event) + .add_systems(Update, report_time) + .add_event::() + .insert_resource(TimeUpdateStrategy::ManualDuration(time_step)); - // Start with 0 events - let events = app.world().resource::>(); - assert_eq!(events.len(), 0); + // Frame 0 + // Fixed update should not have run yet + app.update(); - // Send an event - app.world_mut().send_event(DummyEvent); let events = app.world().resource::>(); assert_eq!(events.len(), 1); - assert_eq!(events.iter_current_update_events().count(), 1); + assert_eq!(events.iter_current_update_events().len(), 1); - // Update the app by a single timestep + // Frame 1 // Fixed update should not have run yet app.update(); let events = app.world().resource::>(); assert_eq!(events.len(), 1); - assert_eq!(events.iter_current_update_events().count(), 1); + assert_eq!(events.iter_current_update_events().len(), 1); - // Update the app by another timestep + // Frame 2 // Fixed update should have run now app.update(); - let events = app.world().resource::>(); + let events: &Events = app.world().resource::>(); assert_eq!(events.len(), 1); - assert_eq!(events.iter_current_update_events().count(), 0); + assert_eq!(events.iter_current_update_events().len(), 0); - // Update the app by another timestep + // Frame 3 // Fixed update should have run exactly once still app.update(); - let events = app.world().resource::>(); + let events: &Events = app.world().resource::>(); assert_eq!(events.len(), 1); - assert_eq!(events.iter_current_update_events().count(), 0); + assert_eq!(events.iter_current_update_events().len(), 0); - // Update the app by another timestep + // Frame 4 // Fixed update should have run twice now app.update(); - let events = app.world().resource::>(); + let events: &Events = app.world().resource::>(); assert_eq!(events.len(), 0); - assert_eq!(events.iter_current_update_events().count(), 0); + assert_eq!(events.iter_current_update_events().len(), 0); } } From f34e68e3058c7b3dfc725a18d2d4876ba0a858f5 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 15:38:51 -0400 Subject: [PATCH 17/19] Improve test to add debugging --- crates/bevy_ecs/src/event/registry.rs | 2 +- crates/bevy_time/src/lib.rs | 58 ++++++++------------------- 2 files changed, 17 insertions(+), 43 deletions(-) diff --git a/crates/bevy_ecs/src/event/registry.rs b/crates/bevy_ecs/src/event/registry.rs index fbfb53d7403c7..2904214bdee3d 100644 --- a/crates/bevy_ecs/src/event/registry.rs +++ b/crates/bevy_ecs/src/event/registry.rs @@ -29,7 +29,7 @@ pub struct EventRegistry { } /// Controls whether or not the events in an [`EventRegistry`] should be updated. -#[derive(Default)] +#[derive(Default, Debug, Clone, Copy, PartialEq, Eq)] pub enum ShouldUpdateEvents { /// Without any fixed timestep, events should always be updated each frame. #[default] diff --git a/crates/bevy_time/src/lib.rs b/crates/bevy_time/src/lib.rs index 6aa472e3b8d5f..cca0b5c10aa09 100644 --- a/crates/bevy_time/src/lib.rs +++ b/crates/bevy_time/src/lib.rs @@ -147,7 +147,7 @@ mod tests { use crate::{Fixed, Time, TimePlugin, TimeUpdateStrategy, Virtual}; use bevy_app::{App, FixedUpdate, Startup, Update}; use bevy_ecs::{ - event::{Event, EventReader, EventWriter, Events}, + event::{Event, EventReader, EventRegistry, EventWriter, Events}, system::{Local, Res, ResMut, Resource}, }; use bevy_utils::Duration; @@ -306,49 +306,23 @@ mod tests { let mut app = App::new(); app.add_plugins(TimePlugin) - .add_systems(Startup, send_event) - .add_systems(Update, report_time) .add_event::() + .init_resource::() + .add_systems(Startup, send_event) + .add_systems(FixedUpdate, count_fixed_updates) .insert_resource(TimeUpdateStrategy::ManualDuration(time_step)); - // Frame 0 - // Fixed update should not have run yet - app.update(); - - let events = app.world().resource::>(); - assert_eq!(events.len(), 1); - assert_eq!(events.iter_current_update_events().len(), 1); - - // Frame 1 - // Fixed update should not have run yet - app.update(); - - let events = app.world().resource::>(); - assert_eq!(events.len(), 1); - assert_eq!(events.iter_current_update_events().len(), 1); - - // Frame 2 - // Fixed update should have run now - app.update(); - - let events: &Events = app.world().resource::>(); - assert_eq!(events.len(), 1); - assert_eq!(events.iter_current_update_events().len(), 0); - - // Frame 3 - // Fixed update should have run exactly once still - app.update(); - - let events: &Events = app.world().resource::>(); - assert_eq!(events.len(), 1); - assert_eq!(events.iter_current_update_events().len(), 0); - - // Frame 4 - // Fixed update should have run twice now - app.update(); - - let events: &Events = app.world().resource::>(); - assert_eq!(events.len(), 0); - assert_eq!(events.iter_current_update_events().len(), 0); + for i 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 {i}, {fixed_updates_seen} fixed updates seen. Should update: {should_update:?}"); + println!("Total events: {n_total_events} | Current events: {n_current_events}",); + } } } From 1d8a8ab568a8cbb5ee18e8c7bf37f01b389ca276 Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 15:50:52 -0400 Subject: [PATCH 18/19] Assert correct behavior --- crates/bevy_time/src/lib.rs | 49 ++++++++++++++++++++++++++++++++++--- 1 file changed, 46 insertions(+), 3 deletions(-) diff --git a/crates/bevy_time/src/lib.rs b/crates/bevy_time/src/lib.rs index cca0b5c10aa09..18347a81a1e9e 100644 --- a/crates/bevy_time/src/lib.rs +++ b/crates/bevy_time/src/lib.rs @@ -147,7 +147,7 @@ mod tests { use crate::{Fixed, Time, TimePlugin, TimeUpdateStrategy, Virtual}; use bevy_app::{App, FixedUpdate, Startup, Update}; use bevy_ecs::{ - event::{Event, EventReader, EventRegistry, EventWriter, Events}, + event::{Event, EventReader, EventRegistry, EventWriter, Events, ShouldUpdateEvents}, system::{Local, Res, ResMut, Resource}, }; use bevy_utils::Duration; @@ -312,7 +312,7 @@ mod tests { .add_systems(FixedUpdate, count_fixed_updates) .insert_resource(TimeUpdateStrategy::ManualDuration(time_step)); - for i in 0..10 { + for frame in 0..10 { app.update(); let fixed_updates_seen = app.world().resource::().0; let events = app.world().resource::>(); @@ -321,8 +321,51 @@ mod tests { let event_registry = app.world().resource::(); let should_update = event_registry.should_update; - println!("Frame {i}, {fixed_updates_seen} fixed updates seen. Should update: {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 => { + assert_eq!(fixed_updates_seen, 0); + assert_eq!(n_total_events, 1); + assert_eq!(n_current_events, 1); + assert_eq!(should_update, ShouldUpdateEvents::Waiting); + } + 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); + } + } } } } From a2bb9bc61dbd1dd158353d31790a753453bbf1ee Mon Sep 17 00:00:00 2001 From: Alice Cecile Date: Tue, 11 Jun 2024 15:59:21 -0400 Subject: [PATCH 19/19] Clippy --- crates/bevy_time/src/lib.rs | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/crates/bevy_time/src/lib.rs b/crates/bevy_time/src/lib.rs index 18347a81a1e9e..9b1bfc2e7635c 100644 --- a/crates/bevy_time/src/lib.rs +++ b/crates/bevy_time/src/lib.rs @@ -325,13 +325,7 @@ mod tests { println!("Total events: {n_total_events} | Current events: {n_current_events}",); match frame { - 0 => { - assert_eq!(fixed_updates_seen, 0); - assert_eq!(n_total_events, 1); - assert_eq!(n_current_events, 1); - assert_eq!(should_update, ShouldUpdateEvents::Waiting); - } - 1 => { + 0 | 1 => { assert_eq!(fixed_updates_seen, 0); assert_eq!(n_total_events, 1); assert_eq!(n_current_events, 1);