From 461734f12ef0741013f896bcdeabf36c919dc1e4 Mon Sep 17 00:00:00 2001 From: JoJoJet Date: Fri, 7 Oct 2022 20:48:39 -0400 Subject: [PATCH 01/10] Add a function to map `Mut` -> `Mut` --- crates/bevy_ecs/src/change_detection.rs | 42 +++++++++++++++++++++++++ 1 file changed, 42 insertions(+) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index c05226c94b9de..cf4d2d63cc4eb 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -173,6 +173,17 @@ macro_rules! impl_into_inner { self.set_changed(); self.value } + + /// Maps to an inner value by applying a function to the contained reference, without flagging a change. + /// + /// You must *not* modify the argument passed to the closure. + /// Violating this rule will likely result in logic errors, but it will not cause undefined behavior. + pub fn map_unchanged(self, f: impl FnOnce(&mut $target) -> &mut U) -> Mut<'a, U> { + Mut { + value: f(self.value), + ticks: self.ticks, + } + } } }; } @@ -497,4 +508,35 @@ mod tests { assert_eq!(3, into_mut.ticks.last_change_tick); assert_eq!(4, into_mut.ticks.change_tick); } + + #[test] + fn map_mut() { + use super::*; + struct Outer(i64); + + let mut component_ticks = ComponentTicks { + added: 1, + changed: 2, + }; + let ticks = Ticks { + component_ticks: &mut component_ticks, + last_change_tick: 2, + change_tick: 3, + }; + + let mut outer = Outer(0); + let ptr = Mut { + value: &mut outer, + ticks, + }; + assert!(!ptr.is_changed()); + + // Perform a mapping operation. + let mut inner = ptr.map_unchanged(|x| &mut x.0); + assert!(!inner.is_changed()); + + // Mutate the inner value. + *inner = 64; + assert!(inner.is_changed()); + } } From b024b7c75455b72f448391ebb35ad1f5134badbc Mon Sep 17 00:00:00 2001 From: JoJoJet Date: Fri, 7 Oct 2022 20:54:10 -0400 Subject: [PATCH 02/10] rename a macro --- crates/bevy_ecs/src/change_detection.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index cf4d2d63cc4eb..eab057206b9d3 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -163,7 +163,7 @@ macro_rules! change_detection_impl { }; } -macro_rules! impl_into_inner { +macro_rules! impl_methods { ($name:ident < $( $generics:tt ),+ >, $target:ty, $($traits:ident)?) => { impl<$($generics),* : ?Sized $(+ $traits)?> $name<$($generics),*> { /// Consume `self` and return a mutable reference to the @@ -226,7 +226,7 @@ pub struct ResMut<'a, T: ?Sized + Resource> { } change_detection_impl!(ResMut<'a, T>, T, Resource); -impl_into_inner!(ResMut<'a, T>, T, Resource); +impl_methods!(ResMut<'a, T>, T, Resource); impl_debug!(ResMut<'a, T>, Resource); impl<'a, T: Resource> From> for Mut<'a, T> { @@ -258,7 +258,7 @@ pub struct NonSendMut<'a, T: ?Sized + 'static> { } change_detection_impl!(NonSendMut<'a, T>, T,); -impl_into_inner!(NonSendMut<'a, T>, T,); +impl_methods!(NonSendMut<'a, T>, T,); impl_debug!(NonSendMut<'a, T>,); impl<'a, T: 'static> From> for Mut<'a, T> { @@ -279,7 +279,7 @@ pub struct Mut<'a, T: ?Sized> { } change_detection_impl!(Mut<'a, T>, T,); -impl_into_inner!(Mut<'a, T>, T,); +impl_methods!(Mut<'a, T>, T,); impl_debug!(Mut<'a, T>,); /// Unique mutable borrow of resources or an entity's component. From 27f7787632066752396bb469bb0b42bffbfc6980 Mon Sep 17 00:00:00 2001 From: JoJoJet Date: Sat, 8 Oct 2022 16:10:50 -0400 Subject: [PATCH 03/10] add an assert --- crates/bevy_ecs/src/change_detection.rs | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index eab057206b9d3..28851a33ae0c7 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -518,10 +518,11 @@ mod tests { added: 1, changed: 2, }; + let (last_change_tick, change_tick) = (2, 3); let ticks = Ticks { component_ticks: &mut component_ticks, - last_change_tick: 2, - change_tick: 3, + last_change_tick, + change_tick, }; let mut outer = Outer(0); @@ -538,5 +539,7 @@ mod tests { // Mutate the inner value. *inner = 64; assert!(inner.is_changed()); + // Modifying one field of a component should flag a change for the entire component. + assert!(component_ticks.is_changed(last_change_tick, change_tick)); } } From c05528834f8a8f800f1125b0787ea7bf55f4b270 Mon Sep 17 00:00:00 2001 From: JoJoJet Date: Sat, 8 Oct 2022 16:11:59 -0400 Subject: [PATCH 04/10] Soften a warning --- crates/bevy_ecs/src/change_detection.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index 28851a33ae0c7..2405c0e96c5a3 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -176,7 +176,7 @@ macro_rules! impl_methods { /// Maps to an inner value by applying a function to the contained reference, without flagging a change. /// - /// You must *not* modify the argument passed to the closure. + /// You should not modify the argument passed to the closure, unless you are familiar with `bevy_ecs` internals. /// Violating this rule will likely result in logic errors, but it will not cause undefined behavior. pub fn map_unchanged(self, f: impl FnOnce(&mut $target) -> &mut U) -> Mut<'a, U> { Mut { From b839a69c37b895d4640a5dab1f57fade42fcab21 Mon Sep 17 00:00:00 2001 From: JoJoJet Date: Sat, 8 Oct 2022 16:23:39 -0400 Subject: [PATCH 05/10] Add an example --- crates/bevy_ecs/src/change_detection.rs | 21 ++++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index 2405c0e96c5a3..0ec81c4627cdc 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -177,7 +177,26 @@ macro_rules! impl_methods { /// Maps to an inner value by applying a function to the contained reference, without flagging a change. /// /// You should not modify the argument passed to the closure, unless you are familiar with `bevy_ecs` internals. - /// Violating this rule will likely result in logic errors, but it will not cause undefined behavior. + /// + /// ```rust + /// # use bevy_ecs::prelude::*; + /// # pub struct Transform { translation: Vec2 } + /// # mod my_utils { + /// # pub fn set_if_not_equal(x: Mut, val: T) { + /// # if *x != val { *x = val; } + /// # } + /// # } + /// // When run, zeroes the translation of every entity. + /// fn reset_positions(mut transforms: Query<&mut Transform>) { + /// for transform in &mut transforms { + /// // We pinky promise not to modify `t` within the closure. + /// // Breaking this promise will result in logic errors, but will never cause undefined behavior. + /// let translation = transform.map_unchanged(|t| &mut t.translation); + /// // Only reset the translation if it isn't already zero; + /// my_utils::set_if_not_equal(translation, Vec2::ZERO); + /// } + /// } + /// ``` pub fn map_unchanged(self, f: impl FnOnce(&mut $target) -> &mut U) -> Mut<'a, U> { Mut { value: f(self.value), From d3b37a62ab505d2ec2931741e8dae8059be14264 Mon Sep 17 00:00:00 2001 From: JoJoJet Date: Sat, 8 Oct 2022 16:27:24 -0400 Subject: [PATCH 06/10] add a paranoid assert --- crates/bevy_ecs/src/change_detection.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index 0ec81c4627cdc..4d3abb7bb6bda 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -196,6 +196,7 @@ macro_rules! impl_methods { /// my_utils::set_if_not_equal(translation, Vec2::ZERO); /// } /// } + /// # bevy_ecs::system::assert_is_system(reset_positions); /// ``` pub fn map_unchanged(self, f: impl FnOnce(&mut $target) -> &mut U) -> Mut<'a, U> { Mut { From cd0b460556490551cd370d54ee84a63d9f2d6c3b Mon Sep 17 00:00:00 2001 From: JoJoJet Date: Sat, 8 Oct 2022 16:34:24 -0400 Subject: [PATCH 07/10] fix the example --- crates/bevy_ecs/src/change_detection.rs | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index 4d3abb7bb6bda..aed5b56d82d2b 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -180,9 +180,12 @@ macro_rules! impl_methods { /// /// ```rust /// # use bevy_ecs::prelude::*; + /// # pub struct Vec2; + /// # impl Vec2 { pub const ZERO: Self = Self; } /// # pub struct Transform { translation: Vec2 } /// # mod my_utils { - /// # pub fn set_if_not_equal(x: Mut, val: T) { + /// # use super::*; + /// # pub fn set_if_not_equal(x: Mut, val: T) { /// # if *x != val { *x = val; } /// # } /// # } From 73b09aa7410cc2ecbb8834644282c20424abf506 Mon Sep 17 00:00:00 2001 From: JoJoJet Date: Sat, 8 Oct 2022 16:40:56 -0400 Subject: [PATCH 08/10] actually fix the example --- crates/bevy_ecs/src/change_detection.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index aed5b56d82d2b..10fb899da4941 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -182,7 +182,7 @@ macro_rules! impl_methods { /// # use bevy_ecs::prelude::*; /// # pub struct Vec2; /// # impl Vec2 { pub const ZERO: Self = Self; } - /// # pub struct Transform { translation: Vec2 } + /// # #[derive(Component)] pub struct Transform { translation: Vec2 } /// # mod my_utils { /// # use super::*; /// # pub fn set_if_not_equal(x: Mut, val: T) { From 2a8304c57df4799777c1341665325d3e575bb9de Mon Sep 17 00:00:00 2001 From: JoJoJet Date: Sat, 8 Oct 2022 16:48:59 -0400 Subject: [PATCH 09/10] actually really fix the example --- crates/bevy_ecs/src/change_detection.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index 10fb899da4941..d408e27f52a70 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -184,10 +184,7 @@ macro_rules! impl_methods { /// # impl Vec2 { pub const ZERO: Self = Self; } /// # #[derive(Component)] pub struct Transform { translation: Vec2 } /// # mod my_utils { - /// # use super::*; - /// # pub fn set_if_not_equal(x: Mut, val: T) { - /// # if *x != val { *x = val; } - /// # } + /// # pub fn set_if_not_equal(x: bevy_ecs::prelude::Mut, val: T) { unimplemented!() } /// # } /// // When run, zeroes the translation of every entity. /// fn reset_positions(mut transforms: Query<&mut Transform>) { From b5768b41678ea68f0793187cae73ba4e11ad1e38 Mon Sep 17 00:00:00 2001 From: JoJoJet Date: Sat, 8 Oct 2022 17:04:03 -0400 Subject: [PATCH 10/10] Suggest using `bypass_change_detection` --- crates/bevy_ecs/src/change_detection.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index d408e27f52a70..914b1951376bf 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/crates/bevy_ecs/src/change_detection.rs @@ -176,7 +176,8 @@ macro_rules! impl_methods { /// Maps to an inner value by applying a function to the contained reference, without flagging a change. /// - /// You should not modify the argument passed to the closure, unless you are familiar with `bevy_ecs` internals. + /// You should never modify the argument passed to the closure -- if you want to modify the data + /// without flagging a change, consider using [`DetectChanges::bypass_change_detection`] to make your intent explicit. /// /// ```rust /// # use bevy_ecs::prelude::*;