bevy/crates/bevy_ecs/src/observer/entity_cloning.rs
urben1680 546711b807
Split EntityClonerBuilder in OptOut and OptIn variants (#19649)
# 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>
2025-06-24 00:12:08 +00:00

112 lines
3.6 KiB
Rust

//! Logic to track observers when cloning entities.
use crate::{
component::ComponentCloneBehavior,
entity::{
CloneByFilter, ComponentCloneCtx, EntityClonerBuilder, EntityMapper, SourceComponent,
},
observer::ObservedBy,
world::World,
};
use super::Observer;
impl<Filter: CloneByFilter> EntityClonerBuilder<'_, Filter> {
/// Sets the option to automatically add cloned entities to the observers targeting source entity.
pub fn add_observers(&mut self, add_observers: bool) -> &mut Self {
if add_observers {
self.override_clone_behavior::<ObservedBy>(ComponentCloneBehavior::Custom(
component_clone_observed_by,
))
} else {
self.remove_clone_behavior_override::<ObservedBy>()
}
}
}
fn component_clone_observed_by(_source: &SourceComponent, ctx: &mut ComponentCloneCtx) {
let target = ctx.target();
let source = ctx.source();
ctx.queue_deferred(move |world: &mut World, _mapper: &mut dyn EntityMapper| {
let observed_by = world
.get::<ObservedBy>(source)
.map(|observed_by| observed_by.0.clone())
.expect("Source entity must have ObservedBy");
world
.entity_mut(target)
.insert(ObservedBy(observed_by.clone()));
for observer_entity in observed_by.iter().copied() {
let mut observer_state = world
.get_mut::<Observer>(observer_entity)
.expect("Source observer entity must have Observer");
observer_state.descriptor.entities.push(target);
let event_types = observer_state.descriptor.events.clone();
let components = observer_state.descriptor.components.clone();
for event_type in event_types {
let observers = world.observers.get_observers_mut(event_type);
if components.is_empty() {
if let Some(map) = observers.entity_observers.get(&source).cloned() {
observers.entity_observers.insert(target, map);
}
} else {
for component in &components {
let Some(observers) = observers.component_observers.get_mut(component)
else {
continue;
};
if let Some(map) =
observers.entity_component_observers.get(&source).cloned()
{
observers.entity_component_observers.insert(target, map);
}
}
}
}
}
});
}
#[cfg(test)]
mod tests {
use crate::{
entity::EntityCloner,
event::{EntityEvent, Event},
observer::On,
resource::Resource,
system::ResMut,
world::World,
};
#[derive(Resource, Default)]
struct Num(usize);
#[derive(Event, EntityEvent)]
struct E;
#[test]
fn clone_entity_with_observer() {
let mut world = World::default();
world.init_resource::<Num>();
let e = world
.spawn_empty()
.observe(|_: On<E>, mut res: ResMut<Num>| res.0 += 1)
.id();
world.flush();
world.trigger_targets(E, e);
let e_clone = world.spawn_empty().id();
EntityCloner::build_opt_out(&mut world)
.add_observers(true)
.clone_entity(e, e_clone);
world.trigger_targets(E, [e, e_clone]);
assert_eq!(world.resource::<Num>().0, 3);
}
}