Add set_if_neq method to DetectChanges trait (Rebased) (bevyengine#6853)

# Objective

Change detection can be spuriously triggered by setting a field to the same value as before. As a result, a common pattern is to write:

```rust
if *foo != value {
  *foo = value;
}
```

This is confusing to read, and heavy on boilerplate.

Adopted from bevyengine#5373, but untangled and rebased to current `bevy/main`.

## Solution

    1. Add a method to the `DetectChanges` trait that implements this boilerplate when the appropriate trait bounds are met.

    2. Document this minor footgun, and point users to it.


## Changelog

    * added the `set_if_neq` method to avoid triggering change detection when the new and previous values are equal. This will work on both components and resources.


## Migration Guide

If you are manually checking if a component or resource's value is equal to its new value before setting it to avoid triggering change detection, migrate to the clearer and more convenient `set_if_neq` method.
## Context

Related to bevyengine#2363 as it avoids triggering change detection, but not a complete solution (as it still requires triggering it when real changes are made).



Co-authored-by: Zoey <[email protected]>

Author: 2 people

crates/bevy_ecs/src/change_detection.rs

@@ -31,6 +31,13 @@ pub const MAX_CHANGE_AGE: u32 = u32::MAX - (2 * CHECK_TICK_THRESHOLD - 1);
 /// Normally change detecting is triggered by either [`DerefMut`] or [`AsMut`], however
 /// it can be manually triggered via [`DetectChanges::set_changed`].
 ///
+/// To ensure that changes are only triggered when the value actually differs,
+/// check if the value would change before assignment, such as by checking that `new != old`.
+/// You must be *sure* that you are not mutably derefencing in this process.
+///
+/// [`set_if_neq`](DetectChanges::set_if_neq) is a helper
+/// method for this common functionality.
+///
 /// ```
 /// use bevy_ecs::prelude::*;
 ///
@@ -90,6 +97,17 @@ pub trait DetectChanges {
     /// However, it can be an essential escape hatch when, for example,
     /// you are trying to synchronize representations using change detection and need to avoid infinite recursion.
     fn bypass_change_detection(&mut self) -> &mut Self::Inner;
+
+    /// Sets `self` to `value`, if and only if `*self != *value`
+    ///
+    /// `T` is the type stored within the smart pointer (e.g. [`Mut`] or [`ResMut`]).
+    ///
+    /// This is useful to ensure change detection is only triggered when the underlying value
+    /// changes, instead of every time [`DerefMut`] is used.
+    fn set_if_neq<Target>(&mut self, value: Target)
+    where
+        Self: Deref<Target = Target> + DerefMut<Target = Target>,
+        Target: PartialEq;
 }
 
 macro_rules! change_detection_impl {
@@ -132,6 +150,19 @@ macro_rules! change_detection_impl {
             fn bypass_change_detection(&mut self) -> &mut Self::Inner {
                 self.value
             }
+
+            #[inline]
+            fn set_if_neq<Target>(&mut self, value: Target)
+            where
+                Self: Deref<Target = Target> + DerefMut<Target = Target>,
+                Target: PartialEq,
+            {
+                // This dereference is immutable, so does not trigger change detection
+                if *<Self as Deref>::deref(self) != value {
+                    // `DerefMut` usage triggers change detection
+                    *<Self as DerefMut>::deref_mut(self) = value;
+                }
+            }
         }
 
         impl<$($generics),*: ?Sized $(+ $traits)?> Deref for $name<$($generics),*> {
@@ -435,6 +466,19 @@ impl<'a> DetectChanges for MutUntyped<'a> {
     fn bypass_change_detection(&mut self) -> &mut Self::Inner {
         &mut self.value
     }
+
+    #[inline]
+    fn set_if_neq<Target>(&mut self, value: Target)
+    where
+        Self: Deref<Target = Target> + DerefMut<Target = Target>,
+        Target: PartialEq,
+    {
+        // This dereference is immutable, so does not trigger change detection
+        if *<Self as Deref>::deref(self) != value {
+            // `DerefMut` usage triggers change detection
+            *<Self as DerefMut>::deref_mut(self) = value;
+        }
+    }
 }
 
 impl std::fmt::Debug for MutUntyped<'_> {
@@ -458,12 +502,17 @@ mod tests {
         world::World,
     };
 
-    #[derive(Component)]
+    use super::DetectChanges;
+
+    #[derive(Component, PartialEq)]
     struct C;
 
     #[derive(Resource)]
     struct R;
 
+    #[derive(Resource, PartialEq)]
+    struct R2(u8);
+
     #[test]
     fn change_expiration() {
         fn change_detected(query: Query<ChangeTrackers<C>>) -> bool {
@@ -635,4 +684,30 @@ mod tests {
         // Modifying one field of a component should flag a change for the entire component.
         assert!(component_ticks.is_changed(last_change_tick, change_tick));
     }
+
+    #[test]
+    fn set_if_neq() {
+        let mut world = World::new();
+
+        world.insert_resource(R2(0));
+        // Resources are Changed when first added
+        world.increment_change_tick();
+        // This is required to update world::last_change_tick
+        world.clear_trackers();
+
+        let mut r = world.resource_mut::<R2>();
+        assert!(!r.is_changed(), "Resource must begin unchanged.");
+
+        r.set_if_neq(R2(0));
+        assert!(
+            !r.is_changed(),
+            "Resource must not be changed after setting to the same value."
+        );
+
+        r.set_if_neq(R2(3));
+        assert!(
+            r.is_changed(),
+            "Resource must be changed after setting to a different value."
+        );
+    }
 }

crates/bevy_ui/src/focus.rs

@@ -1,5 +1,6 @@
 use crate::{camera_config::UiCameraConfig, CalculatedClip, Node, UiStack};
 use bevy_ecs::{
+    change_detection::DetectChanges,
     entity::Entity,
     prelude::Component,
     query::WorldQuery,
@@ -179,10 +180,8 @@ pub fn ui_focus_system(
                     Some(*entity)
                 } else {
                     if let Some(mut interaction) = node.interaction {
-                        if *interaction == Interaction::Hovered
-                            || (cursor_position.is_none() && *interaction != Interaction::None)
-                        {
-                            *interaction = Interaction::None;
+                        if *interaction == Interaction::Hovered || (cursor_position.is_none()) {
+                            interaction.set_if_neq(Interaction::None);
                         }
                     }
                     None
@@ -227,8 +226,8 @@ pub fn ui_focus_system(
     while let Some(node) = iter.fetch_next() {
         if let Some(mut interaction) = node.interaction {
             // don't reset clicked nodes because they're handled separately
-            if *interaction != Interaction::Clicked && *interaction != Interaction::None {
-                *interaction = Interaction::None;
+            if *interaction != Interaction::Clicked {
+                interaction.set_if_neq(Interaction::None);
             }
         }
     }

examples/ecs/component_change_detection.rs

@@ -13,7 +13,7 @@ fn main() {
         .run();
 }
 
-#[derive(Component, Debug)]
+#[derive(Component, PartialEq, Debug)]
 struct MyComponent(f32);
 
 fn setup(mut commands: Commands) {
@@ -25,7 +25,12 @@ fn change_component(time: Res<Time>, mut query: Query<(Entity, &mut MyComponent)
     for (entity, mut component) in &mut query {
         if rand::thread_rng().gen_bool(0.1) {
             info!("changing component {:?}", entity);
-            component.0 = time.elapsed_seconds();
+            let new_component = MyComponent(time.elapsed_seconds().round());
+            // Change detection occurs on mutable derefence,
+            // and does not consider whether or not a value is actually equal.
+            // To avoid triggering change detection when nothing has actually changed,
+            // you can use the `set_if_neq` method on any component or resource that implements PartialEq
+            component.set_if_neq(new_component);
         }
     }
 }