Split EntityClonerBuilder in OptOut and OptIn variants

[urben1680]

Objective

Further tests after #19326 showed that configuring EntityCloner with required components is bug prone and the current design has several weaknesses in it's API:

• Mixing EntityClonerBuilder::allow and EntityClonerBuilder::deny requires extra care how to support that which has an impact on surrounding code that has to keep edge cases in mind. This is especially true for attempts to fix the following issues. There is no use-case known (to me) why someone would mix those.
• A builder with EntityClonerBuilder::allow_all configuration tries to support required components like EntityClonerBuilder::deny_all does, but the meaning of that is conflicting with how you'd expect things to work:
  • If all components should be cloned except component A, do you also want to exclude required components of A too? Or are these also valid without A at the target entity?
  • If EntityClonerBuilder::allow_all should ignore required components and not add them to be filtered away, which purpose has EntityClonerBuilder::without_required_components for this cloner?
• Other bugs found with the linked PR are:
  • Denying A also denies required components of A even when A does not exist at the source entity
  • Allowing A also allows required components of A even when A does not exist at the source entity
• Adding allow_if_new filters to the cloner faces the same issues and require a common solution to dealing with source-archetype sensitive cloning

Alternative to #19632 and #19635.

Solution

EntityClonerBuilder is made generic and split into EntityClonerBuilder<OptOut> and EntityClonerBuilder<OptIn>

For an overview of the changes, see the migration guide. It is generally a good idea to start a review of that.

Algorithm

The generic of EntityClonerBuilder contains the filter data that is needed to build and clone the entity components.

As the filter needs to be borrowed mutably for the duration of the clone, the borrow checker forced me to separate the filter value and all other fields in EntityCloner. The latter are now in the EntityClonerConfig struct. This caused many changed LOC, sorry.

To make reviewing easier:

1. Check the migration guide
2. Many methods of EntityCloner now just call identitcal EntityClonerConfig methods with a mutable borrow of the filter
3. Check EntityClonerConfig::clone_entity_internal which changed a bit regarding the filter usage that is now trait powered (CloneByFilter) to support OptOut, OptIn and EntityClonerFilter (an enum combining the first two)
4. Check OptOut type that no longer tracks required components but has a insert_mode field
5. Check OptIn type that has the most logic changes

Testing

I added a bunch of tests that cover the new logic parts and the fixed issues.

Benchmarks are in a comment a bit below which shows ~4% to 9% regressions, but it varied wildly for me. For example at one run the reflection-based clonings were on-par with main while the other are not, and redoing that swapped the situation for both.

It would be really cool if I could get some hints how to get better benchmark results or if you could run them on your machine too.

Just be aware this is not a Performance PR but a Bugfix PR, even if I smuggled in some more functionalities. So doing changes to EntityClonerBuilder is kind of required here which might make us bite the bullet.

[alice-i-cecile]

I think that giving up on handling required components is a very reasonable choice.

I quite like this PR: this is well-designed after quite a bit of back-and-forth, presents a clear API, and has excellent docs, tests and benchmarks. The builder pattern with a generic for the type state is a bit fancy, but clear enough, and this isn't a high enough traffic beginner API that I'm going to complain.

[urben1680]

One last minute idea, while it makes no sense to include required components for OptOut...

• Does not consider required components anymore. Denying A, which requires B, does not imply B alone would not be useful at the target. So if you do not want to clone B too, you need to deny it explicitly.

... what about denying components required by this component? When you don't want A and it is required by B, would you want to deny B and those requiring B and so on? ComponentInfo offers that information so it would be trivial to add.

I could also add an equivalent to without_required, maybe without_required_by to opt out there?
Or, since this is the "inverse" of OptIn, maybe you would have to actually activate this behavior with with_required_by?

It would fit the "symmetry" here but it also sounds like a footgun to indirectly not clone "higher" components that, because of technicality, might be unknown to the API user. But then again, if that can be the case the user should not deny a required component in the first place. 🤔

I currently lean into implementing this idea with without_required_by to optionally deactivate including required by components.

[eugineerd]

The duplication of various APIs due to the split is a bit unfortunate, but I think it should be fine overall.

---

One last minute idea

I think this does make sense since components that require other components expect them to be present on the entity to function. That being said, I'm not sure how useful that would actually be in practice.

[urben1680]

I fixed the bug, added missing benches and introduced the new "backward" behavior of required components for the OptOut filter. I also redid most docs explaining that so it is a bit more clear.

[eugineerd]

I think everything should be good now, only needs some minor code consolidation changes.

---

As an aside, not related to this PR, just something I though about while reviewing the code and will probably experiment with later:

• EntityClonerBuilder uses &mut self style build since I wanted the closure configuration style api for Commands and without_required_components, however now I feel like self-based builder would've actually been better. clone_entity on the builder was never intended to be called multiple times and is implemented this way just to allow chaining style api. Using self-based builder would allow us to remove the *_opt_in/*_opt_out split at the Commands level and just require the user to pass in their configured EntityCloner. Not sure if this will be more ergonomic, but maybe?
• *_by_ids/*_by_type_ids/*_by_bundle_id resulted in a combinatorial explosion due to the addition of *_if_new. I think it makes sense to introduce a trait similar to WorldEntityFetch for ids that will be implemented for ComponentId, TypeId and BundleId as well as their IntoIter versions.

[urben1680]

however now I feel like self-based builder would've actually been better.

I agree, for reusing the cloner the builder should be finished. I also noticed when writing the benchmarks that a by-value builder would have been nicer to work with there.

I think it makes sense to introduce a trait similar to WorldEntityFetch for ids

That is a good idea as well.

I agree both fit more into a follow-up though. This PR is already not very concrete in it's task.

Thank you for helping me improving these changes. ❤️