bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.28k stars 3.58k forks source link

PartialEq and Eq based change detection #5618

Open alice-i-cecile opened 2 years ago

alice-i-cecile commented 2 years ago

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:

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?

https://github.com/bevyengine/bevy/pull/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.

aevyrie commented 2 years ago

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

Ratysz commented 2 years ago

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.

maniwani commented 2 years ago

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 commented 1 year ago

I can work on this if it's considered to still be useful? Will probably wait on https://github.com/bevyengine/bevy/pull/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 commented 1 year ago

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 commented 1 year ago

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