From 173bb48d7879c96c077862be032f4751cd110442 Mon Sep 17 00:00:00 2001 From: Nathan Ward Date: Sun, 30 May 2021 19:29:31 +0000 Subject: [PATCH] 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 | 169 +++++++++++++++++++++ crates/bevy_ecs/src/lib.rs | 2 + crates/bevy_ecs/src/query/fetch.rs | 25 +-- crates/bevy_ecs/src/reflect.rs | 50 +----- crates/bevy_ecs/src/system/system_param.rs | 95 ++---------- crates/bevy_ecs/src/world/entity_ref.rs | 25 +-- crates/bevy_ecs/src/world/mod.rs | 17 ++- crates/bevy_ecs/src/world/pointer.rs | 81 +--------- crates/bevy_render/src/camera/camera.rs | 1 + 9 files changed, 231 insertions(+), 234 deletions(-) create mode 100644 crates/bevy_ecs/src/change_detection.rs diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs new file mode 100644 index 0000000000..bc420879f2 --- /dev/null +++ b/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) { +/// 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>` 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,); diff --git a/crates/bevy_ecs/src/lib.rs b/crates/bevy_ecs/src/lib.rs index cdb3432494..f5e01120c1 100644 --- a/crates/bevy_ecs/src/lib.rs +++ b/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}, diff --git a/crates/bevy_ecs/src/query/fetch.rs b/crates/bevy_ecs/src/query/fetch.rs index af858cb634..b24a9e8b90 100644 --- a/crates/bevy_ecs/src/query/fetch.rs +++ b/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 { 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), - change_tick: self.change_tick, - last_change_tick: self.last_change_tick, + ticks: Ticks { + component_ticks: &mut *self.table_ticks.add(table_row), + 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 { (*self.sparse_set).get_with_ticks(entity).unwrap(); Mut { value: &mut *component.cast::(), - component_ticks: &mut *component_ticks, - change_tick: self.change_tick, - last_change_tick: self.last_change_tick, + ticks: Ticks { + component_ticks: &mut *component_ticks, + change_tick: self.change_tick, + last_change_tick: self.last_change_tick, + }, } } } @@ -552,9 +557,11 @@ impl<'w, T: Component> Fetch<'w> for WriteFetch { 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), - change_tick: self.change_tick, - last_change_tick: self.last_change_tick, + ticks: Ticks { + component_ticks: &mut *self.table_ticks.add(table_row), + change_tick: self.change_tick, + last_change_tick: self.last_change_tick, + }, } } } diff --git a/crates/bevy_ecs/src/reflect.rs b/crates/bevy_ecs/src/reflect.rs index 3e79161578..d1f4897b00 100644 --- a/crates/bevy_ecs/src/reflect.rs +++ b/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 FromType for ReflectComponent { .get_unchecked_mut::(world.last_change_tick(), world.read_change_tick()) .map(|c| ReflectMut { value: c.value as &mut dyn Reflect, - component_ticks: c.component_ticks, - last_change_tick: c.last_change_tick, - change_tick: c.change_tick, + ticks: c.ticks, }) }, } } } -/// 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)] diff --git a/crates/bevy_ecs/src/system/system_param.rs b/crates/bevy_ecs/src/system/system_param.rs index bd93e844eb..eb7bbcc1a6 100644 --- a/crates/bevy_ecs/src/system/system_param.rs +++ b/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 { } } -/// Unique borrow of a resource. -/// -/// # Panics -/// -/// Panics when used as a [`SystemParameter`](SystemParam) if the resource does not exist. -/// -/// Use `Option>` 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 for ResMut<'w, T> { - #[inline] - fn as_ref(&self) -> &T { - self.deref() - } -} - -impl<'w, T: Component> AsMut for ResMut<'w, T> { - #[inline] - fn as_mut(&mut self) -> &mut T { - self.deref_mut() - } -} - /// The [`SystemParamState`] of [`ResMut`]. pub struct ResMutState { component_id: ComponentId, @@ -476,9 +401,11 @@ impl<'a, T: Component> SystemParamFetch<'a> for ResMutState { }); ResMut { value: value.value, - ticks: value.component_ticks, - last_change_tick: system_state.last_change_tick, - change_tick, + ticks: Ticks { + component_ticks: value.ticks.component_ticks, + last_change_tick: system_state.last_change_tick, + change_tick, + }, } } } @@ -514,9 +441,11 @@ impl<'a, T: Component> SystemParamFetch<'a> for OptionResMutState { .get_resource_unchecked_mut_with_id(state.0.component_id) .map(|value| ResMut { value: value.value, - ticks: value.component_ticks, - last_change_tick: system_state.last_change_tick, - change_tick, + ticks: Ticks { + component_ticks: value.ticks.component_ticks, + last_change_tick: system_state.last_change_tick, + change_tick, + }, }) } } diff --git a/crates/bevy_ecs/src/world/entity_ref.rs b/crates/bevy_ecs/src/world/entity_ref.rs index daf5c31d7b..cb4a10b66b 100644 --- a/crates/bevy_ecs/src/world/entity_ref.rs +++ b/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::(), self.entity, self.location) .map(|(value, ticks)| Mut { value: &mut *value.cast::(), - component_ticks: &mut *ticks, - last_change_tick, - change_tick, + ticks: Ticks { + component_ticks: &mut *ticks, + last_change_tick, + change_tick, + }, }) } } @@ -161,9 +164,11 @@ impl<'w> EntityMut<'w> { ) .map(|(value, ticks)| Mut { value: &mut *value.cast::(), - component_ticks: &mut *ticks, - last_change_tick: self.world.last_change_tick(), - change_tick: self.world.change_tick(), + ticks: Ticks { + component_ticks: &mut *ticks, + 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::(), self.entity, self.location) .map(|(value, ticks)| Mut { value: &mut *value.cast::(), - component_ticks: &mut *ticks, - last_change_tick: self.world.last_change_tick(), - change_tick: self.world.read_change_tick(), + ticks: Ticks { + component_ticks: &mut *ticks, + last_change_tick: self.world.last_change_tick(), + change_tick: self.world.read_change_tick(), + }, }) } diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 9f46b2f355..f549ff791a 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/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::() }, - component_ticks: &mut ticks, - last_change_tick: self.last_change_tick(), - change_tick: self.change_tick(), + ticks: Ticks { + component_ticks: &mut ticks, + 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::(), - component_ticks: &mut *column.get_ticks_mut_ptr(), - last_change_tick: self.last_change_tick(), - change_tick: self.read_change_tick(), + ticks: Ticks { + component_ticks: &mut *column.get_ticks_mut_ptr(), + last_change_tick: self.last_change_tick(), + change_tick: self.read_change_tick(), + }, }) } diff --git a/crates/bevy_ecs/src/world/pointer.rs b/crates/bevy_ecs/src/world/pointer.rs index dbca45b6f9..f60995a43c 100644 --- a/crates/bevy_ecs/src/world/pointer.rs +++ b/crates/bevy_ecs/src/world/pointer.rs @@ -1,80 +1 @@ -use crate::component::ComponentTicks; -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 for Mut<'w, T> { - #[inline] - fn as_ref(&self) -> &T { - self.deref() - } -} - -impl<'w, T> AsMut 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) - } -} +pub use crate::change_detection::Mut; diff --git a/crates/bevy_render/src/camera/camera.rs b/crates/bevy_render/src/camera/camera.rs index df4074001c..f3f63ca40a 100644 --- a/crates/bevy_render/src/camera/camera.rs +++ b/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,