diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index e09e4501dd..e3614f47af 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -5,6 +5,21 @@ use crate::{component::ComponentTicks, system::Resource}; use bevy_reflect::Reflect; use std::ops::{Deref, DerefMut}; +/// The (arbitrarily chosen) minimum number of world tick increments between `check_tick` scans. +/// +/// Change ticks can only be scanned when systems aren't running. Thus, if the threshold is `N`, +/// the maximum is `2 * N - 1` (i.e. the world ticks `N - 1` times, then `N` times). +/// +/// If no change is older than `u32::MAX - (2 * N - 1)` following a scan, none of their ages can +/// overflow and cause false positives. +// (518,400,000 = 1000 ticks per frame * 144 frames per second * 3600 seconds per hour) +pub const CHECK_TICK_THRESHOLD: u32 = 518_400_000; + +/// The maximum change tick difference that won't overflow before the next `check_tick` scan. +/// +/// Changes stop being detected once they become this old. +pub const MAX_CHANGE_AGE: u32 = u32::MAX - (2 * CHECK_TICK_THRESHOLD - 1); + /// Types that implement reliable change detection. /// /// ## Example @@ -28,19 +43,18 @@ use std::ops::{Deref, DerefMut}; /// ``` /// pub trait DetectChanges { - /// Returns true if (and only if) this value been added since the last execution of this - /// system. + /// Returns `true` if this value was added after the system last ran. fn is_added(&self) -> bool; - /// Returns true if (and only if) this value been changed since the last execution of this - /// system. + /// Returns `true` if this value was added or mutably dereferenced after the system last ran. fn is_changed(&self) -> bool; - /// Manually flags this value as having been changed. This normally isn't - /// required because accessing this pointer mutably automatically flags this - /// value as "changed". + /// Flags this value as having been changed. /// - /// **Note**: This operation is irreversible. + /// Mutably accessing this smart pointer will automatically flag this value as having been changed. + /// However, mutation through interior mutability requires manual reporting. + /// + /// **Note**: This operation cannot be undone. fn set_changed(&mut self); /// Returns the change tick recording the previous time this component (or resource) was changed. @@ -213,3 +227,105 @@ pub struct ReflectMut<'a> { change_detection_impl!(ReflectMut<'a>, dyn Reflect,); #[cfg(feature = "bevy_reflect")] impl_into_inner!(ReflectMut<'a>, dyn Reflect,); + +#[cfg(test)] +mod tests { + use crate::{ + self as bevy_ecs, + change_detection::{CHECK_TICK_THRESHOLD, MAX_CHANGE_AGE}, + component::Component, + query::ChangeTrackers, + system::{IntoSystem, Query, System}, + world::World, + }; + + #[derive(Component)] + struct C; + + #[test] + fn change_expiration() { + fn change_detected(query: Query>) -> bool { + query.single().is_changed() + } + + fn change_expired(query: Query>) -> bool { + query.single().is_changed() + } + + let mut world = World::new(); + + // component added: 1, changed: 1 + world.spawn().insert(C); + + let mut change_detected_system = IntoSystem::into_system(change_detected); + let mut change_expired_system = IntoSystem::into_system(change_expired); + change_detected_system.initialize(&mut world); + change_expired_system.initialize(&mut world); + + // world: 1, system last ran: 0, component changed: 1 + // The spawn will be detected since it happened after the system "last ran". + assert!(change_detected_system.run((), &mut world)); + + // world: 1 + MAX_CHANGE_AGE + let change_tick = world.change_tick.get_mut(); + *change_tick = change_tick.wrapping_add(MAX_CHANGE_AGE); + + // Both the system and component appeared `MAX_CHANGE_AGE` ticks ago. + // Since we clamp things to `MAX_CHANGE_AGE` for determinism, + // `ComponentTicks::is_changed` will now see `MAX_CHANGE_AGE > MAX_CHANGE_AGE` + // and return `false`. + assert!(!change_expired_system.run((), &mut world)); + } + + #[test] + fn change_tick_wraparound() { + fn change_detected(query: Query>) -> bool { + query.single().is_changed() + } + + let mut world = World::new(); + world.last_change_tick = u32::MAX; + *world.change_tick.get_mut() = 0; + + // component added: 0, changed: 0 + world.spawn().insert(C); + + // system last ran: u32::MAX + let mut change_detected_system = IntoSystem::into_system(change_detected); + change_detected_system.initialize(&mut world); + + // Since the world is always ahead, as long as changes can't get older than `u32::MAX` (which we ensure), + // the wrapping difference will always be positive, so wraparound doesn't matter. + assert!(change_detected_system.run((), &mut world)); + } + + #[test] + fn change_tick_scan() { + let mut world = World::new(); + + // component added: 1, changed: 1 + world.spawn().insert(C); + + // a bunch of stuff happens, the component is now older than `MAX_CHANGE_AGE` + *world.change_tick.get_mut() += MAX_CHANGE_AGE + CHECK_TICK_THRESHOLD; + let change_tick = world.change_tick(); + + let mut query = world.query::>(); + for tracker in query.iter(&world) { + let ticks_since_insert = change_tick.wrapping_sub(tracker.component_ticks.added); + let ticks_since_change = change_tick.wrapping_sub(tracker.component_ticks.changed); + assert!(ticks_since_insert > MAX_CHANGE_AGE); + assert!(ticks_since_change > MAX_CHANGE_AGE); + } + + // scan change ticks and clamp those at risk of overflow + world.check_change_ticks(); + + for tracker in query.iter(&world) { + let ticks_since_insert = change_tick.wrapping_sub(tracker.component_ticks.added); + let ticks_since_change = change_tick.wrapping_sub(tracker.component_ticks.changed); + assert!(ticks_since_insert == MAX_CHANGE_AGE); + assert!(ticks_since_change == MAX_CHANGE_AGE); + } + } +} diff --git a/crates/bevy_ecs/src/component.rs b/crates/bevy_ecs/src/component.rs index 9276738674..97adb2f5be 100644 --- a/crates/bevy_ecs/src/component.rs +++ b/crates/bevy_ecs/src/component.rs @@ -1,6 +1,7 @@ //! Types for declaring and storing [`Component`]s. use crate::{ + change_detection::MAX_CHANGE_AGE, storage::{SparseSetIndex, Storages}, system::Resource, }; @@ -345,6 +346,7 @@ impl Components { } } +/// Records when a component was added and when it was last mutably dereferenced (or added). #[derive(Copy, Clone, Debug)] pub struct ComponentTicks { pub(crate) added: u32, @@ -353,22 +355,35 @@ pub struct ComponentTicks { impl ComponentTicks { #[inline] + /// Returns `true` if the component was added after the system last ran. pub fn is_added(&self, last_change_tick: u32, change_tick: u32) -> bool { - // The comparison is relative to `change_tick` so that we can detect changes over the whole - // `u32` range. Comparing directly the ticks would limit to half that due to overflow - // handling. - let component_delta = change_tick.wrapping_sub(self.added); - let system_delta = change_tick.wrapping_sub(last_change_tick); + // This works even with wraparound because the world tick (`change_tick`) is always "newer" than + // `last_change_tick` and `self.added`, and we scan periodically to clamp `ComponentTicks` values + // so they never get older than `u32::MAX` (the difference would overflow). + // + // The clamp here ensures determinism (since scans could differ between app runs). + let ticks_since_insert = change_tick.wrapping_sub(self.added).min(MAX_CHANGE_AGE); + let ticks_since_system = change_tick + .wrapping_sub(last_change_tick) + .min(MAX_CHANGE_AGE); - component_delta < system_delta + ticks_since_system > ticks_since_insert } #[inline] + /// Returns `true` if the component was added or mutably dereferenced after the system last ran. pub fn is_changed(&self, last_change_tick: u32, change_tick: u32) -> bool { - let component_delta = change_tick.wrapping_sub(self.changed); - let system_delta = change_tick.wrapping_sub(last_change_tick); + // This works even with wraparound because the world tick (`change_tick`) is always "newer" than + // `last_change_tick` and `self.changed`, and we scan periodically to clamp `ComponentTicks` values + // so they never get older than `u32::MAX` (the difference would overflow). + // + // The clamp here ensures determinism (since scans could differ between app runs). + let ticks_since_change = change_tick.wrapping_sub(self.changed).min(MAX_CHANGE_AGE); + let ticks_since_system = change_tick + .wrapping_sub(last_change_tick) + .min(MAX_CHANGE_AGE); - component_delta < system_delta + ticks_since_system > ticks_since_change } pub(crate) fn new(change_tick: u32) -> Self { @@ -384,8 +399,10 @@ impl ComponentTicks { } /// Manually sets the change tick. - /// Usually, this is done automatically via the [`DerefMut`](std::ops::DerefMut) implementation - /// on [`Mut`](crate::world::Mut) or [`ResMut`](crate::system::ResMut) etc. + /// + /// This is normally done automatically via the [`DerefMut`](std::ops::DerefMut) implementation + /// on [`Mut`](crate::change_detection::Mut), [`ResMut`](crate::change_detection::ResMut), etc. + /// However, components and resources that make use of interior mutability might require manual updates. /// /// # Example /// ```rust,no_run @@ -402,10 +419,10 @@ impl ComponentTicks { } fn check_tick(last_change_tick: &mut u32, change_tick: u32) { - let tick_delta = change_tick.wrapping_sub(*last_change_tick); - const MAX_DELTA: u32 = (u32::MAX / 4) * 3; - // Clamp to max delta - if tick_delta > MAX_DELTA { - *last_change_tick = change_tick.wrapping_sub(MAX_DELTA); + let age = change_tick.wrapping_sub(*last_change_tick); + // This comparison assumes that `age` has not overflowed `u32::MAX` before, which will be true + // so long as this check always runs before that can happen. + if age > MAX_CHANGE_AGE { + *last_change_tick = change_tick.wrapping_sub(MAX_CHANGE_AGE); } } diff --git a/crates/bevy_ecs/src/query/filter.rs b/crates/bevy_ecs/src/query/filter.rs index 276b43459e..63c83f6350 100644 --- a/crates/bevy_ecs/src/query/filter.rs +++ b/crates/bevy_ecs/src/query/filter.rs @@ -650,17 +650,12 @@ macro_rules! impl_tick_filter { } impl_tick_filter!( - /// Filter that retrieves components of type `T` that have been added since the last execution - /// of this system. + /// A filter on a component that only retains results added after the system last ran. /// - /// This filter is useful to do one-time post-processing on components. + /// A common use for this filter is one-time initialization. /// - /// Because the ordering of systems can change and this filter is only effective on changes - /// before the query executes you need to use explicit dependency ordering or ordered stages to - /// avoid frame delays. - /// - /// If instead behavior is meant to change on whether the component changed or not - /// [`ChangeTrackers`](crate::query::ChangeTrackers) may be used. + /// To retain all results without filtering but still check whether they were added after the + /// system last ran, use [`ChangeTrackers`](crate::query::ChangeTrackers). /// /// # Examples /// @@ -690,18 +685,15 @@ impl_tick_filter!( ); impl_tick_filter!( - /// Filter that retrieves components of type `T` that have been changed since the last - /// execution of this system. + /// A filter on a component that only retains results added or mutably dereferenced after the system last ran. + /// + /// A common use for this filter is avoiding redundant work when values have not changed. /// - /// This filter is useful for synchronizing components, and as a performance optimization as it - /// means that the query contains fewer items for a system to iterate over. + /// **Note** that simply *mutably dereferencing* a component is considered a change ([`DerefMut`](std::ops::DerefMut)). + /// Bevy does not compare components to their previous values. /// - /// Because the ordering of systems can change and this filter is only effective on changes - /// before the query executes you need to use explicit dependency ordering or ordered - /// stages to avoid frame delays. - /// - /// If instead behavior is meant to change on whether the component changed or not - /// [`ChangeTrackers`](crate::query::ChangeTrackers) may be used. + /// To retain all results without filtering but still check whether they were changed after the + /// system last ran, use [`ChangeTrackers`](crate::query::ChangeTrackers). /// /// # Examples /// diff --git a/crates/bevy_ecs/src/schedule/stage.rs b/crates/bevy_ecs/src/schedule/stage.rs index c2a040d648..2cf14f0d00 100644 --- a/crates/bevy_ecs/src/schedule/stage.rs +++ b/crates/bevy_ecs/src/schedule/stage.rs @@ -1,4 +1,5 @@ use crate::{ + change_detection::CHECK_TICK_THRESHOLD, component::ComponentId, prelude::IntoSystem, schedule::{ @@ -423,15 +424,15 @@ impl SystemStage { /// Rearranges all systems in topological orders. Systems must be initialized. fn rebuild_orders_and_dependencies(&mut self) { - // This assertion is there to document that a maximum of `u32::MAX / 8` systems should be - // added to a stage to guarantee that change detection has no false positive, but it - // can be circumvented using exclusive or chained systems + // This assertion exists to document that the number of systems in a stage is limited + // to guarantee that change detection never yields false positives. However, it's possible + // (but still unlikely) to circumvent this by abusing exclusive or chained systems. assert!( self.exclusive_at_start.len() + self.exclusive_before_commands.len() + self.exclusive_at_end.len() + self.parallel.len() - < (u32::MAX / 8) as usize + < (CHECK_TICK_THRESHOLD as usize) ); debug_assert!( self.uninitialized_run_criteria.is_empty() @@ -561,17 +562,18 @@ impl SystemStage { } } - /// Checks for old component and system change ticks + /// All system and component change ticks are scanned once the world counter has incremented + /// at least [`CHECK_TICK_THRESHOLD`](crate::change_detection::CHECK_TICK_THRESHOLD) + /// times since the previous `check_tick` scan. + /// + /// During each scan, any change ticks older than [`MAX_CHANGE_AGE`](crate::change_detection::MAX_CHANGE_AGE) + /// are clamped to that age. This prevents false positives from appearing due to overflow. fn check_change_ticks(&mut self, world: &mut World) { let change_tick = world.change_tick(); - let time_since_last_check = change_tick.wrapping_sub(self.last_tick_check); - // Only check after at least `u32::MAX / 8` counts, and at most `u32::MAX / 4` counts - // since the max number of [System] in a [SystemStage] is limited to `u32::MAX / 8` - // and this function is called at the end of each [SystemStage] loop - const MIN_TIME_SINCE_LAST_CHECK: u32 = u32::MAX / 8; + let ticks_since_last_check = change_tick.wrapping_sub(self.last_tick_check); - if time_since_last_check > MIN_TIME_SINCE_LAST_CHECK { - // Check all system change ticks + if ticks_since_last_check >= CHECK_TICK_THRESHOLD { + // Check all system change ticks. for exclusive_system in &mut self.exclusive_at_start { exclusive_system.system_mut().check_change_tick(change_tick); } @@ -585,9 +587,8 @@ impl SystemStage { parallel_system.system_mut().check_change_tick(change_tick); } - // Check component ticks + // Check all component change ticks. world.check_change_ticks(); - self.last_tick_check = change_tick; } } @@ -940,8 +941,6 @@ impl Stage for SystemStage { #[cfg(test)] mod tests { use crate::{ - entity::Entity, - query::{ChangeTrackers, Changed}, schedule::{ BoxedSystemLabel, ExclusiveSystemDescriptorCoercion, ParallelSystemDescriptorCoercion, RunCriteria, RunCriteriaDescriptorCoercion, ShouldRun, SingleThreadedExecutor, Stage, @@ -2014,75 +2013,6 @@ mod tests { assert_eq!(*world.resource::(), 1); } - #[test] - fn change_ticks_wrapover() { - const MIN_TIME_SINCE_LAST_CHECK: u32 = u32::MAX / 8; - const MAX_DELTA: u32 = (u32::MAX / 4) * 3; - - let mut world = World::new(); - world.spawn().insert(W(0usize)); - *world.change_tick.get_mut() += MAX_DELTA + 1; - - let mut stage = SystemStage::parallel(); - fn work() {} - stage.add_system(work); - - // Overflow twice - for _ in 0..10 { - stage.run(&mut world); - for tracker in world.query::>>().iter(&world) { - let time_since_last_check = tracker - .change_tick - .wrapping_sub(tracker.component_ticks.added); - assert!(time_since_last_check <= MAX_DELTA); - let time_since_last_check = tracker - .change_tick - .wrapping_sub(tracker.component_ticks.changed); - assert!(time_since_last_check <= MAX_DELTA); - } - let change_tick = world.change_tick.get_mut(); - *change_tick = change_tick.wrapping_add(MIN_TIME_SINCE_LAST_CHECK + 1); - } - } - - #[test] - fn change_query_wrapover() { - use crate::{self as bevy_ecs, component::Component}; - - #[derive(Component)] - struct C; - let mut world = World::new(); - - // Spawn entities at various ticks - let component_ticks = [0, u32::MAX / 4, u32::MAX / 2, u32::MAX / 4 * 3, u32::MAX]; - let ids = component_ticks - .iter() - .map(|tick| { - *world.change_tick.get_mut() = *tick; - world.spawn().insert(C).id() - }) - .collect::>(); - - let test_cases = [ - // normal - (0, u32::MAX / 2, vec![ids[1], ids[2]]), - // just wrapped over - (u32::MAX / 2, 0, vec![ids[0], ids[3], ids[4]]), - ]; - for (last_change_tick, change_tick, changed_entities) in &test_cases { - *world.change_tick.get_mut() = *change_tick; - world.last_change_tick = *last_change_tick; - - assert_eq!( - world - .query_filtered::>() - .iter(&world) - .collect::>(), - *changed_entities - ); - } - } - #[test] fn run_criteria_with_query() { use crate::{self as bevy_ecs, component::Component}; diff --git a/crates/bevy_ecs/src/system/exclusive_system.rs b/crates/bevy_ecs/src/system/exclusive_system.rs index 7baacb24fe..46d2e9eb8c 100644 --- a/crates/bevy_ecs/src/system/exclusive_system.rs +++ b/crates/bevy_ecs/src/system/exclusive_system.rs @@ -1,4 +1,5 @@ use crate::{ + change_detection::MAX_CHANGE_AGE, system::{check_system_change_tick, BoxedSystem, IntoSystem}, world::World, }; @@ -43,7 +44,9 @@ where world.last_change_tick = saved_last_tick; } - fn initialize(&mut self, _: &mut World) {} + fn initialize(&mut self, world: &mut World) { + self.last_change_tick = world.change_tick().wrapping_sub(MAX_CHANGE_AGE); + } fn check_change_tick(&mut self, change_tick: u32) { check_system_change_tick(&mut self.last_change_tick, change_tick, self.name.as_ref()); diff --git a/crates/bevy_ecs/src/system/function_system.rs b/crates/bevy_ecs/src/system/function_system.rs index 1d20222341..55ff0752f3 100644 --- a/crates/bevy_ecs/src/system/function_system.rs +++ b/crates/bevy_ecs/src/system/function_system.rs @@ -1,5 +1,6 @@ use crate::{ archetype::{ArchetypeComponentId, ArchetypeGeneration, ArchetypeId}, + change_detection::MAX_CHANGE_AGE, component::ComponentId, prelude::FromWorld, query::{Access, FilteredAccessSet}, @@ -140,6 +141,7 @@ pub struct SystemState { impl SystemState { pub fn new(world: &mut World) -> Self { let mut meta = SystemMeta::new::(); + meta.last_change_tick = world.change_tick().wrapping_sub(MAX_CHANGE_AGE); let param_state = ::init(world, &mut meta); Self { meta, @@ -398,6 +400,7 @@ where #[inline] fn initialize(&mut self, world: &mut World) { self.world_id = Some(world.id()); + self.system_meta.last_change_tick = world.change_tick().wrapping_sub(MAX_CHANGE_AGE); self.param_state = Some(::init( world, &mut self.system_meta, diff --git a/crates/bevy_ecs/src/system/system.rs b/crates/bevy_ecs/src/system/system.rs index 92f30532ca..44b719e526 100644 --- a/crates/bevy_ecs/src/system/system.rs +++ b/crates/bevy_ecs/src/system/system.rs @@ -1,8 +1,8 @@ use bevy_utils::tracing::warn; use crate::{ - archetype::ArchetypeComponentId, component::ComponentId, query::Access, schedule::SystemLabel, - world::World, + archetype::ArchetypeComponentId, change_detection::MAX_CHANGE_AGE, component::ComponentId, + query::Access, schedule::SystemLabel, world::World, }; use std::borrow::Cow; @@ -69,14 +69,17 @@ pub(crate) fn check_system_change_tick( change_tick: u32, system_name: &str, ) { - let tick_delta = change_tick.wrapping_sub(*last_change_tick); - const MAX_DELTA: u32 = (u32::MAX / 4) * 3; - // Clamp to max delta - if tick_delta > MAX_DELTA { + let age = change_tick.wrapping_sub(*last_change_tick); + // This comparison assumes that `age` has not overflowed `u32::MAX` before, which will be true + // so long as this check always runs before that can happen. + if age > MAX_CHANGE_AGE { warn!( - "Too many intervening systems have run since the last time System '{}' was last run; it may fail to detect changes.", - system_name + "System '{}' has not run for {} ticks. \ + Changes older than {} ticks will not be detected.", + system_name, + age, + MAX_CHANGE_AGE - 1, ); - *last_change_tick = change_tick.wrapping_sub(MAX_DELTA); + *last_change_tick = change_tick.wrapping_sub(MAX_CHANGE_AGE); } } diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index 55cb02176a..c6cc0556f7 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/crates/bevy_ecs/src/system/system_param.rs @@ -213,14 +213,12 @@ where } impl<'w, T: Resource> Res<'w, T> { - /// Returns true if (and only if) this resource been added since the last execution of this - /// system. + /// Returns `true` if the resource was added after the system last ran. pub fn is_added(&self) -> bool { self.ticks.is_added(self.last_change_tick, self.change_tick) } - /// Returns true if (and only if) this resource been changed since the last execution of this - /// system. + /// Returns `true` if the resource was added or mutably dereferenced after the system last ran. pub fn is_changed(&self) -> bool { self.ticks .is_changed(self.last_change_tick, self.change_tick) @@ -779,14 +777,12 @@ where } impl<'w, T: 'static> NonSend<'w, T> { - /// Returns true if (and only if) this resource been added since the last execution of this - /// system. + /// Returns `true` if the resource was added after the system last ran. pub fn is_added(&self) -> bool { self.ticks.is_added(self.last_change_tick, self.change_tick) } - /// Returns true if (and only if) this resource been changed since the last execution of this - /// system. + /// Returns `true` if the resource was added or mutably dereferenced after the system last ran. pub fn is_changed(&self) -> bool { self.ticks .is_changed(self.last_change_tick, self.change_tick)