-
-
Notifications
You must be signed in to change notification settings - Fork 3.7k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add set_if_neq
method to DetectChanges
trait
#5373
Changes from 9 commits
6fd2b76
95ee07b
cb3c70d
c89965d
679343e
fbb4c73
596941e
dacb039
6111e20
37b3a51
bd1e44b
3f64dd3
7e267bb
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -28,6 +28,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::*; | ||
/// | ||
|
@@ -65,6 +72,24 @@ pub trait DetectChanges { | |
/// [`SystemChangeTick`](crate::system::SystemChangeTick) | ||
/// [`SystemParam`](crate::system::SystemParam). | ||
fn last_changed(&self) -> u32; | ||
|
||
/// 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. | ||
#[inline] | ||
fn set_if_neq<T>(&mut self, value: T) | ||
where | ||
Self: Deref<Target = T> + DerefMut<Target = T>, | ||
T: PartialEq, | ||
{ | ||
// This dereference is immutable, so does not trigger change detection | ||
if **self != value { | ||
**self = value; | ||
} | ||
Comment on lines
+98
to
+100
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Perhaps explicit This pattern may be common, but it's rather opaque if you aren't aware that Rust will not cache one DerefMut implicitly to share across both references to There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Just to weigh in, I think it's fine as-is. |
||
} | ||
} | ||
|
||
macro_rules! change_detection_impl { | ||
|
@@ -293,9 +318,14 @@ mod tests { | |
world::World, | ||
}; | ||
|
||
#[derive(Component)] | ||
use super::DetectChanges; | ||
|
||
#[derive(Component, PartialEq)] | ||
struct C; | ||
|
||
#[derive(PartialEq)] | ||
struct R(u8); | ||
|
||
#[test] | ||
fn change_expiration() { | ||
fn change_detected(query: Query<ChangeTrackers<C>>) -> bool { | ||
|
@@ -382,4 +412,30 @@ mod tests { | |
assert!(ticks_since_change == MAX_CHANGE_AGE); | ||
} | ||
} | ||
|
||
#[test] | ||
fn set_if_neq() { | ||
let mut world = World::new(); | ||
|
||
world.insert_resource(R(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::<R>(); | ||
assert!(!r.is_changed(), "Resource must begin unchanged."); | ||
|
||
r.set_if_neq(R(0)); | ||
assert!( | ||
!r.is_changed(), | ||
"Resource must not be changed after setting to the same value." | ||
); | ||
|
||
r.set_if_neq(R(3)); | ||
assert!( | ||
r.is_changed(), | ||
"Resource must be changed after setting to a different value." | ||
); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At first reading, I wasn't a huge fan of the term
immutable
here. However, it seems to be the standard terminology, so it's fine.