Skip to content

Commit 0694743

Browse files
authored
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.
1 parent 0ee8bf9 commit 0694743

File tree

1 file changed

+59
-8
lines changed

1 file changed

+59
-8
lines changed

crates/bevy_ecs/src/entity/clone_entities.rs

Lines changed: 59 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@ use bumpalo::Bump;
55
use core::any::TypeId;
66

77
use crate::{
8+
archetype::Archetype,
89
bundle::Bundle,
910
component::{Component, ComponentCloneBehavior, ComponentCloneFn, ComponentId, ComponentInfo},
1011
entity::{hash_map::EntityHashMap, Entities, Entity, EntityMapper},
@@ -340,6 +341,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> {
340341
pub struct EntityCloner {
341342
filter_allows_components: bool,
342343
filter: HashSet<ComponentId>,
344+
filter_required: HashSet<ComponentId>,
343345
clone_behavior_overrides: HashMap<ComponentId, ComponentCloneBehavior>,
344346
move_components: bool,
345347
linked_cloning: bool,
@@ -356,6 +358,7 @@ impl Default for EntityCloner {
356358
linked_cloning: false,
357359
default_clone_fn: ComponentCloneBehavior::global_default_fn(),
358360
filter: Default::default(),
361+
filter_required: Default::default(),
359362
clone_behavior_overrides: Default::default(),
360363
clone_queue: Default::default(),
361364
deferred_commands: Default::default(),
@@ -459,6 +462,12 @@ impl EntityCloner {
459462
{
460463
let world = world.as_unsafe_world_cell();
461464
let source_entity = world.get_entity(source).expect("Source entity must exist");
465+
let target_archetype = (!self.filter_required.is_empty()).then(|| {
466+
world
467+
.get_entity(target)
468+
.expect("Target entity must exist")
469+
.archetype()
470+
});
462471

463472
#[cfg(feature = "bevy_reflect")]
464473
// SAFETY: we have unique access to `world`, nothing else accesses the registry at this moment, and we clone
@@ -475,7 +484,7 @@ impl EntityCloner {
475484
bundle_scratch = BundleScratch::with_capacity(archetype.component_count());
476485

477486
for component in archetype.components() {
478-
if !self.is_cloning_allowed(&component) {
487+
if !self.is_cloning_allowed(&component, target_archetype) {
479488
continue;
480489
}
481490

@@ -599,9 +608,19 @@ impl EntityCloner {
599608
target
600609
}
601610

602-
fn is_cloning_allowed(&self, component: &ComponentId) -> bool {
603-
(self.filter_allows_components && self.filter.contains(component))
604-
|| (!self.filter_allows_components && !self.filter.contains(component))
611+
fn is_cloning_allowed(
612+
&self,
613+
component: &ComponentId,
614+
target_archetype: Option<&Archetype>,
615+
) -> bool {
616+
if self.filter_allows_components {
617+
self.filter.contains(component)
618+
|| target_archetype.is_some_and(|archetype| {
619+
!archetype.contains(*component) && self.filter_required.contains(component)
620+
})
621+
} else {
622+
!self.filter.contains(component) && !self.filter_required.contains(component)
623+
}
605624
}
606625
}
607626

@@ -803,9 +822,9 @@ impl<'w> EntityClonerBuilder<'w> {
803822
if let Some(info) = self.world.components().get_info(id) {
804823
for required_id in info.required_components().iter_ids() {
805824
if self.entity_cloner.filter_allows_components {
806-
self.entity_cloner.filter.insert(required_id);
825+
self.entity_cloner.filter_required.insert(required_id);
807826
} else {
808-
self.entity_cloner.filter.remove(&required_id);
827+
self.entity_cloner.filter_required.remove(&required_id);
809828
}
810829
}
811830
}
@@ -823,9 +842,9 @@ impl<'w> EntityClonerBuilder<'w> {
823842
if let Some(info) = self.world.components().get_info(id) {
824843
for required_id in info.required_components().iter_ids() {
825844
if self.entity_cloner.filter_allows_components {
826-
self.entity_cloner.filter.remove(&required_id);
845+
self.entity_cloner.filter_required.remove(&required_id);
827846
} else {
828-
self.entity_cloner.filter.insert(required_id);
847+
self.entity_cloner.filter_required.insert(required_id);
829848
}
830849
}
831850
}
@@ -1400,4 +1419,36 @@ mod tests {
14001419
);
14011420
assert!(world.resource::<FromWorldCalled>().0);
14021421
}
1422+
1423+
#[test]
1424+
fn cloning_with_required_components_preserves_existing() {
1425+
#[derive(Component, Clone, PartialEq, Debug, Default)]
1426+
#[require(B(5))]
1427+
struct A;
1428+
1429+
#[derive(Component, Clone, PartialEq, Debug)]
1430+
struct B(u32);
1431+
1432+
let mut world = World::default();
1433+
1434+
let e = world.spawn((A, B(0))).id();
1435+
let e_clone = world.spawn(B(1)).id();
1436+
1437+
EntityCloner::build(&mut world)
1438+
.deny_all()
1439+
.allow::<A>()
1440+
.clone_entity(e, e_clone);
1441+
1442+
assert_eq!(world.entity(e_clone).get::<A>(), Some(&A));
1443+
assert_eq!(world.entity(e_clone).get::<B>(), Some(&B(1)));
1444+
1445+
let e_clone2 = world.spawn(B(2)).id();
1446+
1447+
EntityCloner::build(&mut world)
1448+
.allow_all()
1449+
.deny::<A>()
1450+
.clone_entity(e, e_clone2);
1451+
1452+
assert_eq!(world.entity(e_clone2).get::<B>(), Some(&B(2)));
1453+
}
14031454
}

0 commit comments

Comments
 (0)