From 1c5e850b88c1eb80db41f806341985d6244dd8fa Mon Sep 17 00:00:00 2001 From: Zoey Date: Sun, 11 Dec 2022 19:24:19 +0000 Subject: [PATCH] Add `set_if_neq` method to `DetectChanges` trait (Rebased) (#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 #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 #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 --- crates/bevy_ecs/src/change_detection.rs | 77 +++++++++++++++++++++- crates/bevy_ui/src/focus.rs | 11 ++-- examples/ecs/component_change_detection.rs | 9 ++- 3 files changed, 88 insertions(+), 9 deletions(-) diff --git a/crates/bevy_ecs/src/change_detection.rs b/crates/bevy_ecs/src/change_detection.rs index 0831f316ef4f8..2bd966fd1845e 100644 --- a/crates/bevy_ecs/src/change_detection.rs +++ b/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(&mut self, value: Target) + where + Self: Deref + DerefMut, + 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(&mut self, value: Target) + where + Self: Deref + DerefMut, + Target: PartialEq, + { + // This dereference is immutable, so does not trigger change detection + if *::deref(self) != value { + // `DerefMut` usage triggers change detection + *::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(&mut self, value: Target) + where + Self: Deref + DerefMut, + Target: PartialEq, + { + // This dereference is immutable, so does not trigger change detection + if *::deref(self) != value { + // `DerefMut` usage triggers change detection + *::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>) -> 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::(); + 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." + ); + } } diff --git a/crates/bevy_ui/src/focus.rs b/crates/bevy_ui/src/focus.rs index f3e63bf3e2b95..c27c6fe041159 100644 --- a/crates/bevy_ui/src/focus.rs +++ b/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); } } } diff --git a/examples/ecs/component_change_detection.rs b/examples/ecs/component_change_detection.rs index 56823144d945f..c706f0fdb6da5 100644 --- a/examples/ecs/component_change_detection.rs +++ b/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