Make change lifespan deterministic and update docs (#3956)

## Objective

- ~~Make absurdly long-lived changes stay detectable for even longer (without leveling up to `u64`).~~
- Give all changes a consistent maximum lifespan.
- Improve code clarity.

## Solution

- ~~Increase the frequency of `check_tick` scans to increase the oldest reliably-detectable change.~~
(Deferred until we can benchmark the cost of a scan.)
- Ignore changes older than the maximum reliably-detectable age.
- General refactoring—name the constants, use them everywhere, and update the docs.
- Update test cases to check for the specified behavior.

## Related

This PR addresses (at least partially) the concerns raised in:

- #3071
- #3082 (and associated PR #3084)

## Background

- #1471

Given the minimum interval between `check_ticks` scans, `N`, the oldest reliably-detectable change is `u32::MAX - (2 * N - 1)` (or `MAX_CHANGE_AGE`). Reducing `N` from ~530 million (current value) to something like ~2 million would extend the lifetime of changes by a billion.

| minimum `check_ticks` interval | oldest reliably-detectable change  | usable % of `u32::MAX` |
| --- | --- | --- |
| `u32::MAX / 8`  (536,870,911) | `(u32::MAX / 4) * 3` | 75.0% |
| `2_000_000` | `u32::MAX - 3_999_999` | 99.9% |

Similarly, changes are still allowed to be between `MAX_CHANGE_AGE`-old and `u32::MAX`-old in the interim between `check_tick` scans. While we prevent their age from overflowing, the test to detect changes still compares raw values. This makes failure ultimately unreliable, since when ancient changes stop being detected varies depending on when the next scan occurs.

## Open Question

Currently, systems and system states are incorrectly initialized with their `last_change_tick` set to `0`, which doesn't handle wraparound correctly.

For consistent behavior, they should either be initialized to the world's `last_change_tick` (and detect no changes) or to `MAX_CHANGE_AGE` behind the world's current `change_tick` (and detect everything as a change). I've currently gone with the latter since that was closer to the existing behavior.

## Follow-up Work

(Edited: entire section)

We haven't actually profiled how long a `check_ticks` scan takes on a "large" `World` , so we don't know if it's safe to increase their frequency. However, we are currently relying on play sessions not lasting long enough to trigger a scan and apps not having enough entities/archetypes for it to be "expensive" (our assumption). That isn't a real solution. (Either scanning never costs enough to impact frame times or we provide an option to use `u64` change ticks. Nobody will accept random hiccups.)

To further extend the lifetime of changes, we actually only need to increment the world tick if a system has `Fetch: !ReadOnlySystemParamFetch`. The behavior will be identical because all writes are sequenced, but I'm not sure how to implement that in a way that the compiler can optimize the branch out.

Also, since having no false positives depends on a `check_ticks` scan running at least every `2 * N - 1` ticks, a `last_check_tick` should also be stored in the `World` so that any lull in system execution (like a command flush) could trigger a scan if needed. To be completely robust, all the systems initialized on the world should be scanned, not just those in the current stage.

Author: maniwani

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<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,
 };
@@ -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<T>`](crate::change_detection::Mut), [`ResMut<T>`](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);
     }
 }

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<T>`](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.
-    ///
-    /// 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.
+    /// 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.
     ///
-    /// 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.
+    /// **Note** that simply *mutably dereferencing* a component is considered a change ([`DerefMut`](std::ops::DerefMut)).
+    /// Bevy does not compare components to their previous values.
     ///
-    /// 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<T>`](crate::query::ChangeTrackers).
     ///
     /// # Examples
     ///