-
-
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
PartialEq and Eq based change detection #5618
Comments
I believe @Ratysz has some opinions on how |
My opinion is more about this mechanism being often misused. Instead of using it for behaviors that have to tightly trigger on every single change it's used paired with a specific change being made at a specific point - a case where events are the intended tool. I'm not sure if it's sufficiently reflected in documentation; if we go by my run-ins with the bugs caused by this, it's not. |
I agree with |
I can work on this if it's considered to still be useful? Would |
This would definitely be useful. If you'd like to accelerate it @cBournhonesque, your review on #6659 would be appreciated :)
I would start with the former: should be simple to implement and seriously reduce the false positives. |
Not sure about "simple to implement", i'm blocked in a lot of places lol :p will need some help |
What problem does this solve or what need does it fill?
Change detection can be accidentally triggered, as it occurs via
DerefMut
.What solution would you like?
Once #5577 is complete, add the ability to configure the change detection strategy via a macro associated with the
Component
andResource
derives.There should be 4 options:
DerefMut
(current strategy)PartialEq
(only mark as changed if the new and old values are not equal according toPartialEq
)Eq
(only mark as changed if the new and old values are not equal according toEq
)The default should be set according to the following rules:
DerefMut
change detection.This quasi-specialization should be achievable by setting an associated constant for the trait.
In order to get the
PartialEq
/Eq
strategy to work, we'll need to cache the last value for the data in theMut
wrapper, and then compare to it when determining whether or not to actually update the change tick.What alternative(s) have you considered?
#5373 is a manual workaround, but is both hard to discover and must be used everywhere a value is mutated. It's likely correct to add even if the design in this issue is merged however.
We could make PartialEq and Eq change detection automatically be enabled.
Eq
is derived, use that impl.PartialEq
is derived, use that impl.However, this is not a complete solution, as it does not work with manual impls.
Furthermore, this will double the memory cost of storage for that component / resource , which I'm reluctant to do implicitly.
Additional context
@aevyrie has expressed concerns about the error-prone nature of change detection.
This design will make it much safer to use change detection with non-idempotent operations.
The text was updated successfully, but these errors were encountered: