Add component change detection conditions

[A-Walrus]

Added any_component_changed and any_component_changed_or_removed

Objective

I believe these conditions are generally useful, and having them built in makes sense. Also makes it more symmetric with the resource conditions

Solution

Added them to the common_conditions module

---

Changelog

Added any_component_changed , any_component_changed_or_removed and any_component_added run conditions

[alice-i-cecile]

I think that we should be able to specify this for T: WorldQuery, which would make this quite a bit more powerful.

[joseph-gio]

It would be helpful to have a variant that checks for changed but not added components. We don't have this variant for Mut or Ref since it's easy enough to do val.is_changed() && !val.is_added(), but it's less convenient to add a custom run condition that checks this.

[A-Walrus]

I think that we should be able to specify this for T: WorldQuery, which would make this quite a bit more powerful.

@alice-i-cecile I tried making this change and am getting this monster of an error:

error[E0599]: the method `is_empty` exists for struct `Query<'_, '_, (), Changed<T>>`, but its trait bounds were not satisfied
   --> crates/bevy_ecs/src/schedule/condition.rs:955:52
    |
955 |         move |query: Query<(), Changed<T>>| !query.is_empty()
    |                                                    ^^^^^^^^ method cannot be called on `Query<'_, '_, (), Changed<T>>` due to unsatisfied trait bounds
    |
   ::: crates/bevy_ecs/src/system/query.rs:325:1
    |
325 | pub struct Query<'world, 'state, Q: WorldQuery, F: ReadOnlyWorldQuery = ()> {
    | --------------------------------------------------------------------------- method `is_empty` not found for this struct
    |
   ::: crates/bevy_ecs/src/query/filter.rs:396:9
    |
396 |         pub struct $name<T>(PhantomData<T>);
    |         ------------------------------------
    |         |
    |         doesn't satisfy `<_ as WorldQuery>::ReadOnly = Changed<T>`
    |         doesn't satisfy `filter::Changed<T>: fetch::ReadOnlyWorldQuery`
    |         doesn't satisfy `filter::Changed<T>: fetch::WorldQuery`
    |
note: the following trait bounds were not satisfied:
      `<filter::Changed<T> as fetch::WorldQuery>::ReadOnly = filter::Changed<T>`
      `filter::Changed<T>: fetch::WorldQuery`
   --> crates/bevy_ecs/src/query/fetch.rs:452:38
    |
452 | pub unsafe trait ReadOnlyWorldQuery: WorldQuery<ReadOnly = Self> {}
    |                  ------------------  ^^^^^^^^^^^^^^^^^^^^^^^^^^^
    |                                      |          |
    |                                      |          unsatisfied trait bound introduced here
    |                                      unsatisfied trait bound introduced here
note: trait bound `filter::Changed<T>: fetch::ReadOnlyWorldQuery` was not satisfied
   --> crates/bevy_ecs/src/system/query.rs:351:32
    |
351 | impl<'w, 's, Q: WorldQuery, F: ReadOnlyWorldQuery> Query<'w, 's, Q, F> {
    |                                ^^^^^^^^^^^^^^^^^^  -------------------
    |                                |
    |                                unsatisfied trait bound introduced here
note: the traits `fetch::WorldQuery` and `fetch::ReadOnlyWorldQuery` must be implemented
   --> crates/bevy_ecs/src/query/fetch.rs:316:1
    |
316 | pub unsafe trait WorldQuery {
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^
...
452 | pub unsafe trait ReadOnlyWorldQuery: WorldQuery<ReadOnly = Self> {}
    | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

For more information about this error, try `rustc --explain E0599`.
error: could not compile `bevy_ecs` (lib) due to previous error

[SkiFire13]

The problem is that Changed<T> only works for T: Component, not T: WorldQuery.

[alice-i-cecile]

Right, I can see why that change failed. I do think an added variant would be nice here, but it doesn't have to be this PR.

[joseph-gio]

It doesn't really matter, but I'd like to comment that using the or_else combinator has slightly more overhead than just doing removals.iter().count() != 0 || !changed.is_empty()

[james7132]

This was previously attempted to be added in #7711, which was marked controversial for the same comments I'm adding below.

[james7132]

This runs a potentially very expensive query (iterating through all instances of component T) single-threaded and inline in the scheduler. This blocks the scheduler from launching other systems and can be a very big performance footgun, particularly when there are a large number of entities in the world with T. At a minimum, this should be noted in the doc comments, and it should recommend directly using change detection in the system as the recommenced alternative.

[james7132]

Similar performance footgun concerns here.

[james7132]

Similar performance footgun concerns here.

[A-Walrus]

@james7132 makes a really good point. I don't think it makes sense to add this to bevy with the current performance drawbacks. Checking for changes within the system is more performant, and not much worse ergonomics. If in the future we track this kind of stuff directly, then these conditions would be nice. Closing this PR

[SkiFire13]

Since this is the second time a PR like this was opened, would it be reasonable to leave a comment in the source code explaining why these functions are missing?