Add set_if_neq method to DetectChanges trait
#5373
Conversation
alice-i-cecile
added
A-ECS
|
@aevyrie IIRC you've wanted this functionality :) |
|
Maybe out of scope, but I think the real problem is that mutable dereferences of At minimum, I think you need to add a test here that verifies |
|
Agreed, working on a test here :) |
|
@maniwani, do you have any insight into how to manually update the change tick correctly? The test is currently failing because the resource is being set as
Obviously I can do something more realistic like use a schedule, but I'd like to understand what's going on. EDIT: |
Co-authored-by: Aevyrie <aevyrie@gmail.com>
Following #5007 we could consider doing fake-specialization with associated types, and then allow users to choose whether to use naive That's a much bigger and more controversial design (even if I agree with it); this is just intended to be a nice little helper for now. |
|
🚲 🏠 time. Would |
|
It's more precise, but also more jargon-y. Does get bonus points for brevity though. I'm not sure which one I prefer. I would have gone with |
Normally I'd agree, I always attempt to err on the side of explicitness. However, |
|
@alice-i-cecile remember to update the title and original post with the changed method name. :) |
alice-i-cecile
changed the title
set_if_differs method to DetectChanges traitset_if_neq method to DetectChanges trait
| Self: Deref<Target = T> + DerefMut<Target = T>, | ||
| T: PartialEq, | ||
| { | ||
| // This dereference is immutable, so does not trigger change detection |
There was a problem hiding this comment.
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.
|
Yeah, I should do a pass through and see both a) are there any internal usages and b) is there a good example to put this in? |
|
The change detection example might be a good place for this: https://github.com/bevyengine/bevy/blob/main/examples/ecs/component_change_detection.rs |
|
I found this pattern used in a handful of places internally, but could only find a couple of places where I could use this method directly. There are two other patterns that aren't covered:
|
alice-i-cecile
force-pushed
the
set-if-differs
branch
from
5981564 to
6502c58
Compare
alice-i-cecile
force-pushed
the
set-if-differs
branch
from
6502c58 to
3f64dd3
Compare
Can you give an example of this pattern?
We should be able to use |
alice-i-cecile
added
the
S-Ready-For-Final-Review
|
bors r+ |
# 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.
## 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).
|
Build failed: |
|
This super isn't compiling ATM: I'm going to fix up the merge locally. |
| if **self != value { | ||
| **self = value; | ||
| } |
There was a problem hiding this comment.
Perhaps explicit <Self as Deref<Target = T>>::deref(*self, ...) and <Self as DerefMut<Target = T>>::deref_mut(*self, ...) would be beneficial for clarity?
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 **self.
It's also a bit clearer than trying to explain that one is "immutable" as a prior commenter mentioned, as it becomes explicit; then you can just comment "triggers change tracking" or similar on the second one.
There was a problem hiding this comment.
Just to weigh in, I think it's fine as-is.
|
Closing in favor of #6853, which fixed the rebase for me <3 |
# 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 <Dessix@Dessix.net>
…e#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 bevyengine#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 bevyengine#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 <Dessix@Dessix.net>
…e#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 bevyengine#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 bevyengine#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 <Dessix@Dessix.net>
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:
This is confusing to read, and heavy on boilerplate.
Solution
DetectChangestrait that implements this boilerplate when the appropriate trait bounds are met.Changelog
set_if_neqmethod 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_neqmethod.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).