add Clone to common conditions

[B-Reif]

Objective

The system configuration distributive_run_if requires the given condition to be Clone. This doesn't currently work with the closures returned by common_conditions modules. Partially fixes #8058.

Solution

Add Clone to the return type of common conditions wherever possible.

Caveats

• Because the common conditions resource_equals and resource_exists_and_equals have to dereference the resource they're comparing, these functions would have to specify the resource type to be T: Clone if we want the returned closure to also be Clone.
• The not condition takes any arbitrary condition which may or may not be Clone, and the returned CombinatorSystem isn't Clone.

I'm not sure how to support Clone in these cases. This would likely require some new traits? Open to ideas on this.

---

Changelog

Add Clone to the return type of most common run conditions.

[github-actions]

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

[alice-i-cecile]

Strongly in favor of this: let's keep this simple and nonbreaking for now so we can ship it in the point release. Can you add a few tests to assert that distributive_run_if compiles with these common conditions?

[B-Reif]

Strongly in favor of this: let's keep this simple and nonbreaking for now so we can ship it in the point release. Can you add a few tests to assert that distributive_run_if compiles with these common conditions?

Sure, where should they go?

[alice-i-cecile]

I would add them in a tests module at the end of each of these files.

[B-Reif]

Oh that probably makes more sense than what I just did, I added a test to distributive_run_if. I'll change it around.

[JoJoJet]

Good change.

This doesn't have to be done now, but here's how we can support Clone for not:

1. Implement Clone for CombinatorSystem when both composite systems are Clone.
2. Instead of returning impl Condition<> from not, return a concrete type (probably a type alias). Then the compiler knows that the returned system is Clone as long as the input system is.

[james7132]

readability nit: FnMut is more important here than Clone so IMO it should come first.

[JoJoJet]

FnMut() -> bool + Clone looks weird to me. It makes it look like the function is returning bool + Clone somehow.

[B-Reif]

Good change.

This doesn't have to be done now, but here's how we can support Clone for not:

1. Implement `Clone` for `CombinatorSystem` when both composite systems are `Clone`.

2. Instead of returning `impl Condition<>` from `not`, return a concrete type (probably a type alias). Then the compiler knows that the returned system is `Clone` as long as the input system is.

How can we get the concrete type for the closure that does the inversion?

[JoJoJet]

Ah, I left out a step. You'd have to replace .pipe() with CombinatorSystem using a custom Combine impl.

[alice-i-cecile]

@B-Reif unless I hear otherwise I'm going to merge this PR in about 10 hours :) We can add the additional fixes in a follow-up.

[B-Reif]

@alice-i-cecile Just fixed the merge conflict, please try again :)

[ilyvion]

I take it that because those commits 👆 happened after the PR was merged, they didn't actually make it in? I'm using bevy 0.11 right now, and resource_equals is still not Clone, e.g., even though the resource I'm trying to use it with definitely is.

[JoJoJet]

In order for resource_equals to be clone, the return type would have to be something like impl FnMut(Res<T>) -> bool + Clone if T: Clone. Rust's type system isn't advanced enough to express a conditional trait impl like that, so resource_equals is just never Clone, unfortunately.