Skip to content
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

Open
alice-i-cecile opened this issue Aug 8, 2022 · 6 comments
Open

PartialEq and Eq based change detection #5618

alice-i-cecile opened this issue Aug 8, 2022 · 6 comments
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible

Comments

@alice-i-cecile
Copy link
Member

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 and Resource derives.

There should be 4 options:

  • DerefMut (current strategy)
  • PartialEq (only mark as changed if the new and old values are not equal according to PartialEq)
  • Eq (only mark as changed if the new and old values are not equal according to Eq)
  • disabled (avoids storing change tick information completely)

The default should be set according to the following rules:

  1. If the type has no fields (it is a unit struct or an enum with a single variant), change detection is disabled.
  2. Otherwise use 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 the Mut 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.

  1. If Eq is derived, use that impl.
  2. If only 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.

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events S-Blocked This cannot move forward until something else changes labels Aug 8, 2022
@aevyrie
Copy link
Member

aevyrie commented Aug 8, 2022

I believe @Ratysz has some opinions on how DerefMut is used for change detection. Any thoughts on this?

@Ratysz
Copy link
Contributor

Ratysz commented Aug 8, 2022

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.

@alice-i-cecile alice-i-cecile removed the S-Blocked This cannot move forward until something else changes label Aug 9, 2022
@maniwani
Copy link
Contributor

maniwani commented Aug 9, 2022

I agree with @Ratysz, but I personally have no preferences as long as the options are all still powered by the one internal change tick mechanism internally.

@cBournhonesque
Copy link
Contributor

cBournhonesque commented Jan 17, 2023

I can work on this if it's considered to still be useful?
Will probably wait on #6659

Would PartialEq and Eq change detections also check first that DerefMut was used (as an optimization); or would they not use ticks at all and just do raw comparisons?

@alice-i-cecile
Copy link
Member Author

This would definitely be useful. If you'd like to accelerate it @cBournhonesque, your review on #6659 would be appreciated :)

Would PartialEq and Eq change detections also check first that DerefMut was used (as an optimization); or would they not use ticks at all and just do raw comparisons?

I would start with the former: should be simple to implement and seriously reduce the false positives.

@cBournhonesque
Copy link
Contributor

Not sure about "simple to implement", i'm blocked in a lot of places lol :p will need some help

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Feature A new feature, making something new possible
Projects
None yet
Development

No branches or pull requests

5 participants