546711b807
# 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. --------- Co-authored-by: eugineerd <70062110+eugineerd@users.noreply.github.com>
74 lines
3.6 KiB
Markdown
74 lines
3.6 KiB
Markdown
---
|
|
title: EntityClonerBuilder Split
|
|
pull_requests: [19649]
|
|
---
|
|
|
|
`EntityClonerBuilder` is now generic and has different methods depending on the generic.
|
|
|
|
To get the wanted one, `EntityCloner::build` got split too:
|
|
|
|
- `EntityCloner::build_opt_out` to get `EntityClonerBuilder<OptOut>`
|
|
- `EntityCloner::build_opt_in` to get `EntityClonerBuilder<OptIn>`
|
|
|
|
The first is used to clone all components possible and optionally _opting out_ of some.
|
|
The second is used to only clone components as specified by _opting in_ for them.
|
|
|
|
```rs
|
|
// 0.16
|
|
let mut builder = EntityCloner.build(&mut world);
|
|
builder.allow_all().deny::<ComponentThatShouldNotBeCloned>();
|
|
builder.clone_entity(source_entity, target_entity);
|
|
|
|
let mut builder = EntityCloner.build(&mut world);
|
|
builder.deny_all().allow::<ComponentThatShouldBeCloned>();
|
|
builder.clone_entity(source_entity, target_entity);
|
|
|
|
// 0.17
|
|
let mut builder = EntityCloner.build_opt_out(&mut world);
|
|
builder.deny::<ComponentThatShouldNotBeCloned>();
|
|
builder.clone_entity(source_entity, target_entity);
|
|
|
|
let mut builder = EntityCloner.build_opt_in(&mut world);
|
|
builder.allow::<ComponentThatShouldBeCloned>();
|
|
builder.clone_entity(source_entity, target_entity);
|
|
```
|
|
|
|
Still, using `EntityClonerBuilder::finish` will return a non-generic `EntityCloner`.
|
|
This change is done because the behavior of the two is too different to share the same struct and same methods and mixing them caused bugs.
|
|
|
|
The methods of the two builder types are different to 0.16 and to each other now:
|
|
|
|
## Opt-Out variant
|
|
|
|
- Still offers variants of the `deny` methods which now also includes one with a `BundleId` argument.
|
|
- No longer offers `allow` methods, you need to be exact with denying components.
|
|
- Offers now the `insert_mode` method to configure if components are cloned if they already exist at the target.
|
|
- Required components of denied components are no longer considered. 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. This also means there is no `without_required_components` method anymore as that would be redundant.
|
|
- It is now the other way around: Denying `A`, which is required _by_ `C`, will now also deny `C`. This can be bypassed with the new `without_required_by_components` method.
|
|
|
|
## Opt-In variant
|
|
|
|
- Still offers variants of the `allow` methods which now also includes one with a `BundleId` argument.
|
|
- No longer offers `deny` methods, you need to be exact with allowing components.
|
|
- Offers now `allow_if_new` method variants that only clone this component if the target does not contain it. If it does, required components of it will also not be cloned, except those that are also required by one that is actually cloned.
|
|
- Still offers the `without_required_components` method.
|
|
|
|
## Common methods
|
|
|
|
All other methods `EntityClonerBuilder` had in 0.16 are still available for both variants:
|
|
|
|
- `with_default_clone_fn`
|
|
- `move_components`
|
|
- `clone_behavior` variants
|
|
- `linked_cloning`
|
|
|
|
## Other affected APIs
|
|
|
|
| 0.16 | 0.17 |
|
|
| - | - |
|
|
| `EntityWorldMut::clone_with` | `EntityWorldMut::clone_with_opt_out` `EntityWorldMut::clone_with_opt_in` |
|
|
| `EntityWorldMut::clone_and_spawn_with` | `EntityWorldMut::clone_and_spawn_with_opt_out` `EntityWorldMut::clone_and_spawn_with_opt_in` |
|
|
| `EntityCommands::clone_with` | `EntityCommands::clone_with_opt_out` `EntityCommands::clone_with_opt_in` |
|
|
| `EntityCommands::clone_and_spawn_with` | `EntityCommands::clone_and_spawn_with_opt_out` `EntityCommands::clone_and_spawn_with_opt_in` |
|
|
| `entity_command::clone_with` | `entity_command::clone_with_opt_out` `entity_command::clone_with_opt_in` |
|