[Merged by Bors] - Add set_if_neq method to DetectChanges trait (Rebased)#6853
Closed
Dessix wants to merge 4 commits intobevyengine:mainfrom
Closed
[Merged by Bors] - Add set_if_neq method to DetectChanges trait (Rebased)#6853Dessix wants to merge 4 commits intobevyengine:mainfrom
set_if_neq method to DetectChanges trait (Rebased)#6853Dessix wants to merge 4 commits intobevyengine:mainfrom
Conversation
alice-i-cecile
added
A-ECS
alice-i-cecile
approved these changes
Dec 5, 2022
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Thanks! I have no idea how it got so tangled.
Contributor
Author
Something about how GitHub does sequential non-linear merges, I think. It was an unusually tangled rebase for sure. |
Member
|
Is it |
Dessix
changed the title
set_if_differs method to DetectChanges trait (Rebased)set_if_neq method to DetectChanges trait (Rebased)
Contributor
Author
@mockersf I had quoted the original PR name prior to updating it. I've altered this one to match the content. |
hymm
approved these changes
Dec 11, 2022
alice-i-cecile
added
the
S-Ready-For-Final-Review
Member
|
bors r+ |
bors bot
pushed a commit
that referenced
this pull request
Dec 11, 2022
# 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>
Contributor
bors
bot
changed the title
set_if_neq method to DetectChanges trait (Rebased)set_if_neq method to DetectChanges trait (Rebased)
bors bot
pushed a commit
that referenced
this pull request
Jan 6, 2023
# Objective - The doctest for `Mut::map_unchanged` uses a fake function `set_if_not_equal` to demonstrate usage. - Now that #6853 has been merged, we can use `Mut::set_if_neq` directly instead of mocking it.
james7132
pushed a commit
to james7132/bevy
that referenced
this pull request
Jan 21, 2023
# Objective - The doctest for `Mut::map_unchanged` uses a fake function `set_if_not_equal` to demonstrate usage. - Now that bevyengine#6853 has been merged, we can use `Mut::set_if_neq` directly instead of mocking it.
alradish
pushed a commit
to alradish/bevy
that referenced
this pull request
Jan 22, 2023
…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>
alradish
pushed a commit
to alradish/bevy
that referenced
this pull request
Jan 22, 2023
# Objective - The doctest for `Mut::map_unchanged` uses a fake function `set_if_not_equal` to demonstrate usage. - Now that bevyengine#6853 has been merged, we can use `Mut::set_if_neq` directly instead of mocking it.
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
…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>
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective - The doctest for `Mut::map_unchanged` uses a fake function `set_if_not_equal` to demonstrate usage. - Now that bevyengine#6853 has been merged, we can use `Mut::set_if_neq` directly instead of mocking it.
bors bot
pushed a commit
that referenced
this pull request
Feb 15, 2023
# Objective While porting my crate `bevy_trait_query` to bevy 0.10, I ran into an issue with the `DetectChangesMut` trait. Due to the way that the `set_if_neq` method (added in #6853) is implemented, you are forced to write a nonsense implementation of it for dynamically sized types. This edge case shows up when implementing trait queries, since `DetectChangesMut` is implemented for `Mut<dyn Trait>`. ## Solution Simplify the generics for `set_if_neq` and add the `where Self::Target: Sized` trait bound to it. Add a default implementation so implementers don't need to implement a method with nonsensical trait bounds.
myreprise1
pushed a commit
to myreprise1/bevy
that referenced
this pull request
Feb 16, 2023
# Objective While porting my crate `bevy_trait_query` to bevy 0.10, I ran into an issue with the `DetectChangesMut` trait. Due to the way that the `set_if_neq` method (added in bevyengine#6853) is implemented, you are forced to write a nonsense implementation of it for dynamically sized types. This edge case shows up when implementing trait queries, since `DetectChangesMut` is implemented for `Mut<dyn Trait>`. ## Solution Simplify the generics for `set_if_neq` and add the `where Self::Target: Sized` trait bound to it. Add a default implementation so implementers don't need to implement a method with nonsensical trait bounds.
myreprise1
pushed a commit
to myreprise1/bevy
that referenced
this pull request
Feb 16, 2023
# Objective While porting my crate `bevy_trait_query` to bevy 0.10, I ran into an issue with the `DetectChangesMut` trait. Due to the way that the `set_if_neq` method (added in bevyengine#6853) is implemented, you are forced to write a nonsense implementation of it for dynamically sized types. This edge case shows up when implementing trait queries, since `DetectChangesMut` is implemented for `Mut<dyn Trait>`. ## Solution Simplify the generics for `set_if_neq` and add the `where Self::Target: Sized` trait bound to it. Add a default implementation so implementers don't need to implement a method with nonsensical trait bounds.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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.
Adopted from #5373, but untangled and rebased to current
bevy/main.Solution
Changelog
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).