Refactor ResMut/Mut/ReflectMut to remove duplicated code (#2217)

`ResMut`, `Mut` and `ReflectMut` all share very similar code for change detection.
This PR is a first pass at refactoring these implementation and removing a lot of the duplicated code.

Note, this introduces a new trait `ChangeDetectable`.

Please feel free to comment away and let me know what you think!

crates/bevy_ecs/src/change_detection.rs

@@ -0,0 +1,169 @@
+use crate::component::{Component, ComponentTicks};
+use bevy_reflect::Reflect;
+use std::ops::{Deref, DerefMut};
+
+/// Types that implement reliable change detection.
+///
+/// ## Example
+/// Using types that implement [`DetectChanges`], such as [`ResMut`], provide
+/// a way to query if a value has been mutated in another system.
+/// Normally change detecting is triggered by either [`DerefMut`] or [`AsMut`], however
+/// it can be manually triggered via [`DetectChanges::set_changed`].
+///
+/// ```
+/// use bevy_ecs::prelude::*;
+///
+/// struct MyResource(u32);
+///
+/// fn my_system(mut resource: ResMut<MyResource>) {
+///     if resource.is_changed() {
+///         println!("My resource was mutated!");
+///     }
+///
+///    resource.0 = 42; // triggers change detection via [`DerefMut`]
+/// }
+/// ```
+///
+pub trait DetectChanges {
+    /// Returns true if (and only if) this value been added since the last execution of this
+    /// system.
+    fn is_added(&self) -> bool;
+
+    /// Returns true if (and only if) this value been changed since the last execution of this
+    /// system.
+    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".
+    ///
+    /// **Note**: This operation is irreversible.
+    fn set_changed(&mut self);
+}
+
+macro_rules! change_detection_impl {
+    ($name:ident < $( $generics:tt ),+ >, $target:ty, $($traits:ident)?) => {
+        impl<$($generics),* $(: $traits)?> DetectChanges for $name<$($generics),*> {
+            #[inline]
+            fn is_added(&self) -> bool {
+                self.ticks
+                    .component_ticks
+                    .is_added(self.ticks.last_change_tick, self.ticks.change_tick)
+            }
+
+            #[inline]
+            fn is_changed(&self) -> bool {
+                self.ticks
+                    .component_ticks
+                    .is_changed(self.ticks.last_change_tick, self.ticks.change_tick)
+            }
+
+            #[inline]
+            fn set_changed(&mut self) {
+                self.ticks
+                    .component_ticks
+                    .set_changed(self.ticks.change_tick);
+            }
+        }
+
+        impl<$($generics),* $(: $traits)?> Deref for $name<$($generics),*> {
+            type Target = $target;
+
+            #[inline]
+            fn deref(&self) -> &Self::Target {
+                self.value
+            }
+        }
+
+        impl<$($generics),* $(: $traits)?> DerefMut for $name<$($generics),*> {
+            #[inline]
+            fn deref_mut(&mut self) -> &mut Self::Target {
+                self.set_changed();
+                self.value
+            }
+        }
+
+        impl<$($generics),* $(: $traits)?> AsRef<$target> for $name<$($generics),*> {
+            #[inline]
+            fn as_ref(&self) -> &$target {
+                self.deref()
+            }
+        }
+
+        impl<$($generics),* $(: $traits)?> AsMut<$target> for $name<$($generics),*> {
+            #[inline]
+            fn as_mut(&mut self) -> &mut $target {
+                self.deref_mut()
+            }
+        }
+    };
+}
+
+macro_rules! impl_into_inner {
+    ($name:ident < $( $generics:tt ),+ >, $target:ty, $($traits:ident)?) => {
+        impl<$($generics),* $(: $traits)?> $name<$($generics),*> {
+            /// Consume `self` and return a mutable reference to the
+            /// contained value while marking `self` as "changed".
+            #[inline]
+            pub fn into_inner(mut self) -> &'a mut $target {
+                self.set_changed();
+                self.value
+            }
+        }
+    };
+}
+
+macro_rules! impl_debug {
+    ($name:ident < $( $generics:tt ),+ >, $($traits:ident)?) => {
+        impl<$($generics),* $(: $traits)?> std::fmt::Debug for $name<$($generics),*>
+            where T: std::fmt::Debug
+        {
+            fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
+                f.debug_tuple(stringify!($name))
+                    .field(self.value)
+                    .finish()
+            }
+        }
+
+    };
+}
+
+pub(crate) struct Ticks<'a> {
+    pub(crate) component_ticks: &'a mut ComponentTicks,
+    pub(crate) last_change_tick: u32,
+    pub(crate) change_tick: u32,
+}
+
+/// Unique mutable borrow of a resource.
+///
+/// # Panics
+///
+/// Panics when used as a [`SystemParameter`](crate::system::SystemParam) if the resource does not exist.
+///
+/// Use `Option<ResMut<T>>` instead if the resource might not always exist.
+pub struct ResMut<'a, T: Component> {
+    pub(crate) value: &'a mut T,
+    pub(crate) ticks: Ticks<'a>,
+}
+
+change_detection_impl!(ResMut<'a, T>, T, Component);
+impl_into_inner!(ResMut<'a, T>, T, Component);
+impl_debug!(ResMut<'a, T>, Component);
+
+/// Unique mutable borrow of an entity's component
+pub struct Mut<'a, T> {
+    pub(crate) value: &'a mut T,
+    pub(crate) ticks: Ticks<'a>,
+}
+
+change_detection_impl!(Mut<'a, T>, T,);
+impl_into_inner!(Mut<'a, T>, T,);
+impl_debug!(Mut<'a, T>,);
+
+/// Unique mutable borrow of a Reflected component
+pub struct ReflectMut<'a> {
+    pub(crate) value: &'a mut dyn Reflect,
+    pub(crate) ticks: Ticks<'a>,
+}
+
+change_detection_impl!(ReflectMut<'a>, dyn Reflect,);

crates/bevy_ecs/src/lib.rs

@@ -1,5 +1,6 @@
 pub mod archetype;
 pub mod bundle;
+pub mod change_detection;
 pub mod component;
 pub mod entity;
 pub mod event;
@@ -18,6 +19,7 @@ pub mod prelude {
     #[doc(hidden)]
     pub use crate::{
         bundle::Bundle,
+        change_detection::DetectChanges,
         entity::Entity,
         event::{EventReader, EventWriter},
         query::{Added, ChangeTrackers, Changed, Or, QueryState, With, WithBundle, Without},

crates/bevy_ecs/src/query/fetch.rs

@@ -1,5 +1,6 @@
 use crate::{
     archetype::{Archetype, ArchetypeComponentId},
+    change_detection::Ticks,
     component::{Component, ComponentId, ComponentTicks, StorageType},
     entity::Entity,
     query::{Access, FilteredAccess},
@@ -529,9 +530,11 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch<T> {
                 let table_row = *self.entity_table_rows.add(archetype_index);
                 Mut {
                     value: &mut *self.table_components.as_ptr().add(table_row),
-                    component_ticks: &mut *self.table_ticks.add(table_row),
+                    ticks: Ticks {
-                    change_tick: self.change_tick,
+                        component_ticks: &mut *self.table_ticks.add(table_row),
-                    last_change_tick: self.last_change_tick,
+                        change_tick: self.change_tick,
+                        last_change_tick: self.last_change_tick,
+                    },
                 }
             }
             StorageType::SparseSet => {
@@ -540,9 +543,11 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch<T> {
                     (*self.sparse_set).get_with_ticks(entity).unwrap();
                 Mut {
                     value: &mut *component.cast::<T>(),
-                    component_ticks: &mut *component_ticks,
+                    ticks: Ticks {
-                    change_tick: self.change_tick,
+                        component_ticks: &mut *component_ticks,
-                    last_change_tick: self.last_change_tick,
+                        change_tick: self.change_tick,
+                        last_change_tick: self.last_change_tick,
+                    },
                 }
             }
         }
@@ -552,9 +557,11 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch<T> {
     unsafe fn table_fetch(&mut self, table_row: usize) -> Self::Item {
         Mut {
             value: &mut *self.table_components.as_ptr().add(table_row),
-            component_ticks: &mut *self.table_ticks.add(table_row),
+            ticks: Ticks {
-            change_tick: self.change_tick,
+                component_ticks: &mut *self.table_ticks.add(table_row),
-            last_change_tick: self.last_change_tick,
+                change_tick: self.change_tick,
+                last_change_tick: self.last_change_tick,
+            },
         }
     }
 }

crates/bevy_ecs/src/reflect.rs

@@ -1,7 +1,6 @@
-use std::ops::{Deref, DerefMut};
+pub use crate::change_detection::ReflectMut;
-
 use crate::{
-    component::{Component, ComponentTicks},
+    component::Component,
     entity::{Entity, EntityMap, MapEntities, MapEntitiesError},
     world::{FromWorld, World},
 };
@@ -104,56 +103,13 @@ impl<C: Component + Reflect + FromWorld> FromType<C> for ReflectComponent {
                     .get_unchecked_mut::<C>(world.last_change_tick(), world.read_change_tick())
                     .map(|c| ReflectMut {
                         value: c.value as &mut dyn Reflect,
-                        component_ticks: c.component_ticks,
+                        ticks: c.ticks,
-                        last_change_tick: c.last_change_tick,
-                        change_tick: c.change_tick,
                     })
             },
         }
     }
 }
 
-/// Unique borrow of a Reflected component
-pub struct ReflectMut<'a> {
-    pub(crate) value: &'a mut dyn Reflect,
-    pub(crate) component_ticks: &'a mut ComponentTicks,
-    pub(crate) last_change_tick: u32,
-    pub(crate) change_tick: u32,
-}
-
-impl<'a> Deref for ReflectMut<'a> {
-    type Target = dyn Reflect;
-
-    #[inline]
-    fn deref(&self) -> &dyn Reflect {
-        self.value
-    }
-}
-
-impl<'a> DerefMut for ReflectMut<'a> {
-    #[inline]
-    fn deref_mut(&mut self) -> &mut dyn Reflect {
-        self.component_ticks.set_changed(self.change_tick);
-        self.value
-    }
-}
-
-impl<'a> ReflectMut<'a> {
-    /// Returns true if (and only if) this component been added since the last execution of this
-    /// system.
-    pub fn is_added(&self) -> bool {
-        self.component_ticks
-            .is_added(self.last_change_tick, self.change_tick)
-    }
-
-    /// Returns true if (and only if) this component been changed since the last execution of this
-    /// system.
-    pub fn is_changed(&self) -> bool {
-        self.component_ticks
-            .is_changed(self.last_change_tick, self.change_tick)
-    }
-}
-
 impl_reflect_value!(Entity(Hash, PartialEq, Serialize, Deserialize));
 
 #[derive(Clone)]

crates/bevy_ecs/src/system/system_param.rs

@@ -1,6 +1,8 @@
+pub use crate::change_detection::ResMut;
 use crate::{
     archetype::{Archetype, Archetypes},
     bundle::Bundles,
+    change_detection::Ticks,
     component::{Component, ComponentId, ComponentTicks, Components},
     entity::{Entities, Entity},
     query::{FilterFetch, FilteredAccess, FilteredAccessSet, QueryState, WorldQuery},
@@ -333,83 +335,6 @@ impl<'a, T: Component> SystemParamFetch<'a> for OptionResState<T> {
     }
 }
 
-/// Unique borrow of a resource.
-///
-/// # Panics
-///
-/// Panics when used as a [`SystemParameter`](SystemParam) if the resource does not exist.
-///
-/// Use `Option<ResMut<T>>` instead if the resource might not always exist.
-pub struct ResMut<'w, T: Component> {
-    value: &'w mut T,
-    ticks: &'w mut ComponentTicks,
-    last_change_tick: u32,
-    change_tick: u32,
-}
-
-impl<'w, T: Component> Debug for ResMut<'w, T>
-where
-    T: Debug,
-{
-    fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
-        f.debug_tuple("ResMut").field(&self.value).finish()
-    }
-}
-
-impl<'w, T: Component> ResMut<'w, T> {
-    /// Returns true if (and only if) this resource been added since the last execution of this
-    /// system.
-    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.
-    pub fn is_changed(&self) -> bool {
-        self.ticks
-            .is_changed(self.last_change_tick, self.change_tick)
-    }
-
-    /// Manually flags this resource as having been changed. This normally isn't
-    /// required because accessing this pointer mutably automatically flags this
-    /// resource as "changed".
-    ///
-    /// **Note**: This operation is irreversible.
-    #[inline]
-    pub fn set_changed(&mut self) {
-        self.ticks.set_changed(self.change_tick);
-    }
-}
-
-impl<'w, T: Component> Deref for ResMut<'w, T> {
-    type Target = T;
-
-    fn deref(&self) -> &Self::Target {
-        self.value
-    }
-}
-
-impl<'w, T: Component> DerefMut for ResMut<'w, T> {
-    fn deref_mut(&mut self) -> &mut Self::Target {
-        self.set_changed();
-        self.value
-    }
-}
-
-impl<'w, T: Component> AsRef<T> for ResMut<'w, T> {
-    #[inline]
-    fn as_ref(&self) -> &T {
-        self.deref()
-    }
-}
-
-impl<'w, T: Component> AsMut<T> for ResMut<'w, T> {
-    #[inline]
-    fn as_mut(&mut self) -> &mut T {
-        self.deref_mut()
-    }
-}
-
 /// The [`SystemParamState`] of [`ResMut`].
 pub struct ResMutState<T> {
     component_id: ComponentId,
@@ -476,9 +401,11 @@ impl<'a, T: Component> SystemParamFetch<'a> for ResMutState<T> {
             });
         ResMut {
             value: value.value,
-            ticks: value.component_ticks,
+            ticks: Ticks {
-            last_change_tick: system_state.last_change_tick,
+                component_ticks: value.ticks.component_ticks,
-            change_tick,
+                last_change_tick: system_state.last_change_tick,
+                change_tick,
+            },
         }
     }
 }
@@ -514,9 +441,11 @@ impl<'a, T: Component> SystemParamFetch<'a> for OptionResMutState<T> {
             .get_resource_unchecked_mut_with_id(state.0.component_id)
             .map(|value| ResMut {
                 value: value.value,
-                ticks: value.component_ticks,
+                ticks: Ticks {
-                last_change_tick: system_state.last_change_tick,
+                    component_ticks: value.ticks.component_ticks,
-                change_tick,
+                    last_change_tick: system_state.last_change_tick,
+                    change_tick,
+                },
             })
     }
 }

crates/bevy_ecs/src/world/entity_ref.rs

@@ -1,6 +1,7 @@
 use crate::{
     archetype::{Archetype, ArchetypeId, Archetypes, ComponentStatus},
     bundle::{Bundle, BundleInfo},
+    change_detection::Ticks,
     component::{Component, ComponentId, ComponentTicks, Components, StorageType},
     entity::{Entities, Entity, EntityLocation},
     storage::{SparseSet, Storages},
@@ -80,9 +81,11 @@ impl<'w> EntityRef<'w> {
         get_component_and_ticks_with_type(self.world, TypeId::of::<T>(), self.entity, self.location)
             .map(|(value, ticks)| Mut {
                 value: &mut *value.cast::<T>(),
-                component_ticks: &mut *ticks,
+                ticks: Ticks {
-                last_change_tick,
+                    component_ticks: &mut *ticks,
-                change_tick,
+                    last_change_tick,
+                    change_tick,
+                },
             })
     }
 }
@@ -161,9 +164,11 @@ impl<'w> EntityMut<'w> {
             )
             .map(|(value, ticks)| Mut {
                 value: &mut *value.cast::<T>(),
-                component_ticks: &mut *ticks,
+                ticks: Ticks {
-                last_change_tick: self.world.last_change_tick(),
+                    component_ticks: &mut *ticks,
-                change_tick: self.world.change_tick(),
+                    last_change_tick: self.world.last_change_tick(),
+                    change_tick: self.world.change_tick(),
+                },
             })
         }
     }
@@ -176,9 +181,11 @@ impl<'w> EntityMut<'w> {
         get_component_and_ticks_with_type(self.world, TypeId::of::<T>(), self.entity, self.location)
             .map(|(value, ticks)| Mut {
                 value: &mut *value.cast::<T>(),
-                component_ticks: &mut *ticks,
+                ticks: Ticks {
-                last_change_tick: self.world.last_change_tick(),
+                    component_ticks: &mut *ticks,
-                change_tick: self.world.read_change_tick(),
+                    last_change_tick: self.world.last_change_tick(),
+                    change_tick: self.world.read_change_tick(),
+                },
             })
     }
 

crates/bevy_ecs/src/world/mod.rs

@@ -11,6 +11,7 @@ pub use world_cell::*;
 use crate::{
     archetype::{ArchetypeComponentId, ArchetypeComponentInfo, ArchetypeId, Archetypes},
     bundle::{Bundle, Bundles},
+    change_detection::Ticks,
     component::{
         Component, ComponentDescriptor, ComponentId, ComponentTicks, Components, ComponentsError,
         StorageType,
@@ -708,9 +709,11 @@ impl World {
         // SAFE: pointer is of type T
         let value = Mut {
             value: unsafe { &mut *ptr.cast::<T>() },
-            component_ticks: &mut ticks,
+            ticks: Ticks {
-            last_change_tick: self.last_change_tick(),
+                component_ticks: &mut ticks,
-            change_tick: self.change_tick(),
+                last_change_tick: self.last_change_tick(),
+                change_tick: self.change_tick(),
+            },
         };
         let result = f(self, value);
         let resource_archetype = self.archetypes.resource_mut();
@@ -747,9 +750,11 @@ impl World {
         let column = self.get_populated_resource_column(component_id)?;
         Some(Mut {
             value: &mut *column.get_ptr().as_ptr().cast::<T>(),
-            component_ticks: &mut *column.get_ticks_mut_ptr(),
+            ticks: Ticks {
-            last_change_tick: self.last_change_tick(),
+                component_ticks: &mut *column.get_ticks_mut_ptr(),
-            change_tick: self.read_change_tick(),
+                last_change_tick: self.last_change_tick(),
+                change_tick: self.read_change_tick(),
+            },
         })
     }
 

crates/bevy_ecs/src/world/pointer.rs

@@ -1,80 +1 @@
-use crate::component::ComponentTicks;
+pub use crate::change_detection::Mut;
-use std::ops::{Deref, DerefMut};
-
-/// Unique borrow of an entity's component
-pub struct Mut<'a, T> {
-    pub(crate) value: &'a mut T,
-    pub(crate) component_ticks: &'a mut ComponentTicks,
-    pub(crate) last_change_tick: u32,
-    pub(crate) change_tick: u32,
-}
-
-impl<'a, T> Mut<'a, T> {
-    pub fn into_inner(self) -> &'a mut T {
-        self.component_ticks.set_changed(self.change_tick);
-        self.value
-    }
-
-    /// Manually flags this value as having been changed. This normally isn't
-    /// required because accessing this pointer mutably automatically flags this
-    /// value as "changed".
-    ///
-    /// **Note**: This operation is irreversible.
-    #[inline]
-    pub fn set_changed(&mut self) {
-        self.component_ticks.set_changed(self.change_tick);
-    }
-}
-
-impl<'a, T> Deref for Mut<'a, T> {
-    type Target = T;
-
-    #[inline]
-    fn deref(&self) -> &T {
-        self.value
-    }
-}
-
-impl<'a, T> DerefMut for Mut<'a, T> {
-    #[inline]
-    fn deref_mut(&mut self) -> &mut T {
-        self.set_changed();
-        self.value
-    }
-}
-
-impl<'a, T: core::fmt::Debug> core::fmt::Debug for Mut<'a, T> {
-    fn fmt(&self, f: &mut core::fmt::Formatter<'_>) -> core::fmt::Result {
-        self.value.fmt(f)
-    }
-}
-
-impl<'w, T> AsRef<T> for Mut<'w, T> {
-    #[inline]
-    fn as_ref(&self) -> &T {
-        self.deref()
-    }
-}
-
-impl<'w, T> AsMut<T> for Mut<'w, T> {
-    #[inline]
-    fn as_mut(&mut self) -> &mut T {
-        self.deref_mut()
-    }
-}
-
-impl<'w, T> Mut<'w, T> {
-    /// Returns true if (and only if) this component been added since the last execution of this
-    /// system.
-    pub fn is_added(&self) -> bool {
-        self.component_ticks
-            .is_added(self.last_change_tick, self.change_tick)
-    }
-
-    /// Returns true if (and only if) this component been changed
-    /// since the last execution of this system.
-    pub fn is_changed(&self) -> bool {
-        self.component_ticks
-            .is_changed(self.last_change_tick, self.change_tick)
-    }
-}

crates/bevy_render/src/camera/camera.rs

@@ -1,5 +1,6 @@
 use super::CameraProjection;
 use bevy_ecs::{
+    change_detection::DetectChanges,
     component::Component,
     entity::Entity,
     event::EventReader,