Try #3956:

Author: bors[bot]

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
@@ -199,3 +214,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<ChangeTrackers<C>>) -> bool {
+            query.single().is_changed()
+        }
+
+        fn change_expired(query: Query<ChangeTrackers<C>>) -> 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<ChangeTrackers<C>>) -> 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::<ChangeTrackers<C>>();
+        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);
+        }
+    }
+}

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,
 };
@@ -338,6 +339,7 @@ impl Components {
     }
 }
 
+/// Stores two [`World`](crate::world::World) "ticks" denoting when the component was added and when it was last changed (added or mutably-deferenced).
 #[derive(Clone, Debug)]
 pub struct ComponentTicks {
     pub(crate) added: u32,
@@ -346,22 +348,35 @@ pub struct ComponentTicks {
 
 impl ComponentTicks {
     #[inline]
+    /// Returns `true` if the component was added after the system last ran, `false` otherwise.
     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, `false` otherwise.
     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 {
@@ -377,8 +392,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<T>`](crate::world::Mut), [`ResMut<T>`](crate::system::ResMut), etc.
+    /// However, components and resources that make use of interior mutability might require manual updates.
     ///
     /// # Example
     /// ```rust,no_run
@@ -395,10 +412,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);
     }
 }

crates/bevy_ecs/src/schedule/stage.rs

@@ -1,4 +1,5 @@
 use crate::{
+    change_detection::CHECK_TICK_THRESHOLD,
     component::ComponentId,
     prelude::IntoSystem,
     schedule::{
@@ -424,15 +425,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()
@@ -562,17 +563,18 @@ impl SystemStage {
         }
     }
 
-    /// Checks for old component and system change ticks
+    /// All system and component change ticks are scanned for risk of delta overflow 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 difference, preventing potential false positives 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;
-
-        if time_since_last_check > MIN_TIME_SINCE_LAST_CHECK {
-            // Check all system change ticks
+        let ticks_since_last_check = change_tick.wrapping_sub(self.last_tick_check);
+
+        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);
             }
@@ -586,9 +588,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;
         }
     }
@@ -947,8 +948,6 @@ impl Stage for SystemStage {
 #[cfg(test)]
 mod tests {
     use crate::{
-        entity::Entity,
-        query::{ChangeTrackers, Changed},
         schedule::{
             BoxedSystemLabel, ExclusiveSystemDescriptorCoercion, ParallelSystemDescriptorCoercion,
             RunCriteria, RunCriteriaDescriptorCoercion, RunCriteriaPiping, ShouldRun,
@@ -2011,75 +2010,6 @@ mod tests {
         assert_eq!(*world.resource::<usize>(), 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::<ChangeTrackers<W<usize>>>().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::<Vec<Entity>>();
-
-        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::<Entity, Changed<C>>()
-                    .iter(&world)
-                    .collect::<Vec<Entity>>(),
-                *changed_entities
-            );
-        }
-    }
-
     #[test]
     fn run_criteria_with_query() {
         use crate::{self as bevy_ecs, component::Component};

crates/bevy_ecs/src/system/exclusive_system.rs

@@ -1,5 +1,6 @@
 use crate::{
     archetype::ArchetypeGeneration,
+    change_detection::MAX_CHANGE_AGE,
     system::{check_system_change_tick, BoxedSystem, IntoSystem},
     world::World,
 };
@@ -44,7 +45,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());