Fix EntityCloner replacing required components. (#19326)

# Objective
Fix #19324

## Solution
`EntityCloner` replaces required components when filtering. This is
unexpected when comparing with the way the rest of bevy handles required
components. This PR separates required components from explicit
components when filtering in `EntityClonerBuilder`.

## Testing
Added a regression test for this case.
This commit is contained in:
eugineerd 2025-05-30 22:28:53 +03:00 committed by François Mockers
parent 50f70ebb91
commit bc00178b59

View File

@ -5,6 +5,7 @@ use bumpalo::Bump;
use core::any::TypeId; use core::any::TypeId;
use crate::{ use crate::{
archetype::Archetype,
bundle::Bundle, bundle::Bundle,
component::{Component, ComponentCloneBehavior, ComponentCloneFn, ComponentId, ComponentInfo}, component::{Component, ComponentCloneBehavior, ComponentCloneFn, ComponentId, ComponentInfo},
entity::{hash_map::EntityHashMap, Entities, Entity, EntityMapper}, entity::{hash_map::EntityHashMap, Entities, Entity, EntityMapper},
@ -346,6 +347,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> {
pub struct EntityCloner { pub struct EntityCloner {
filter_allows_components: bool, filter_allows_components: bool,
filter: HashSet<ComponentId>, filter: HashSet<ComponentId>,
filter_required: HashSet<ComponentId>,
clone_behavior_overrides: HashMap<ComponentId, ComponentCloneBehavior>, clone_behavior_overrides: HashMap<ComponentId, ComponentCloneBehavior>,
move_components: bool, move_components: bool,
linked_cloning: bool, linked_cloning: bool,
@ -362,6 +364,7 @@ impl Default for EntityCloner {
linked_cloning: false, linked_cloning: false,
default_clone_fn: ComponentCloneBehavior::global_default_fn(), default_clone_fn: ComponentCloneBehavior::global_default_fn(),
filter: Default::default(), filter: Default::default(),
filter_required: Default::default(),
clone_behavior_overrides: Default::default(), clone_behavior_overrides: Default::default(),
clone_queue: Default::default(), clone_queue: Default::default(),
deferred_commands: Default::default(), deferred_commands: Default::default(),
@ -465,6 +468,12 @@ impl EntityCloner {
{ {
let world = world.as_unsafe_world_cell(); let world = world.as_unsafe_world_cell();
let source_entity = world.get_entity(source).expect("Source entity must exist"); let source_entity = world.get_entity(source).expect("Source entity must exist");
let target_archetype = (!self.filter_required.is_empty()).then(|| {
world
.get_entity(target)
.expect("Target entity must exist")
.archetype()
});
#[cfg(feature = "bevy_reflect")] #[cfg(feature = "bevy_reflect")]
// SAFETY: we have unique access to `world`, nothing else accesses the registry at this moment, and we clone // SAFETY: we have unique access to `world`, nothing else accesses the registry at this moment, and we clone
@ -481,7 +490,7 @@ impl EntityCloner {
bundle_scratch = BundleScratch::with_capacity(archetype.component_count()); bundle_scratch = BundleScratch::with_capacity(archetype.component_count());
for component in archetype.components() { for component in archetype.components() {
if !self.is_cloning_allowed(&component) { if !self.is_cloning_allowed(&component, target_archetype) {
continue; continue;
} }
@ -605,9 +614,19 @@ impl EntityCloner {
target target
} }
fn is_cloning_allowed(&self, component: &ComponentId) -> bool { fn is_cloning_allowed(
(self.filter_allows_components && self.filter.contains(component)) &self,
|| (!self.filter_allows_components && !self.filter.contains(component)) component: &ComponentId,
target_archetype: Option<&Archetype>,
) -> bool {
if self.filter_allows_components {
self.filter.contains(component)
|| target_archetype.is_some_and(|archetype| {
!archetype.contains(*component) && self.filter_required.contains(component)
})
} else {
!self.filter.contains(component) && !self.filter_required.contains(component)
}
} }
} }
@ -809,9 +828,9 @@ impl<'w> EntityClonerBuilder<'w> {
if let Some(info) = self.world.components().get_info(id) { if let Some(info) = self.world.components().get_info(id) {
for required_id in info.required_components().iter_ids() { for required_id in info.required_components().iter_ids() {
if self.entity_cloner.filter_allows_components { if self.entity_cloner.filter_allows_components {
self.entity_cloner.filter.insert(required_id); self.entity_cloner.filter_required.insert(required_id);
} else { } else {
self.entity_cloner.filter.remove(&required_id); self.entity_cloner.filter_required.remove(&required_id);
} }
} }
} }
@ -829,9 +848,9 @@ impl<'w> EntityClonerBuilder<'w> {
if let Some(info) = self.world.components().get_info(id) { if let Some(info) = self.world.components().get_info(id) {
for required_id in info.required_components().iter_ids() { for required_id in info.required_components().iter_ids() {
if self.entity_cloner.filter_allows_components { if self.entity_cloner.filter_allows_components {
self.entity_cloner.filter.remove(&required_id); self.entity_cloner.filter_required.remove(&required_id);
} else { } else {
self.entity_cloner.filter.insert(required_id); self.entity_cloner.filter_required.insert(required_id);
} }
} }
} }
@ -1406,4 +1425,36 @@ mod tests {
); );
assert!(world.resource::<FromWorldCalled>().0); assert!(world.resource::<FromWorldCalled>().0);
} }
#[test]
fn cloning_with_required_components_preserves_existing() {
#[derive(Component, Clone, PartialEq, Debug, Default)]
#[require(B(5))]
struct A;
#[derive(Component, Clone, PartialEq, Debug)]
struct B(u32);
let mut world = World::default();
let e = world.spawn((A, B(0))).id();
let e_clone = world.spawn(B(1)).id();
EntityCloner::build(&mut world)
.deny_all()
.allow::<A>()
.clone_entity(e, e_clone);
assert_eq!(world.entity(e_clone).get::<A>(), Some(&A));
assert_eq!(world.entity(e_clone).get::<B>(), Some(&B(1)));
let e_clone2 = world.spawn(B(2)).id();
EntityCloner::build(&mut world)
.allow_all()
.deny::<A>()
.clone_entity(e, e_clone2);
assert_eq!(world.entity(e_clone2).get::<B>(), Some(&B(2)));
}
} }