[Merged by Bors] - Make change lifespan deterministic and update docs

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
     ///