Skip to content

Commit bc00178

Browse files
eugineerdmockersf
authored andcommitted
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 50f70eb commit bc00178

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},
@@ -346,6 +347,7 @@ impl<'a, 'b> ComponentCloneCtx<'a, 'b> {
346347
pub struct EntityCloner {
347348
filter_allows_components: bool,
348349
filter: HashSet<ComponentId>,
350+
filter_required: HashSet<ComponentId>,
349351
clone_behavior_overrides: HashMap<ComponentId, ComponentCloneBehavior>,
350352
move_components: bool,
351353
linked_cloning: bool,
@@ -362,6 +364,7 @@ impl Default for EntityCloner {
362364
linked_cloning: false,
363365
default_clone_fn: ComponentCloneBehavior::global_default_fn(),
364366
filter: Default::default(),
367+
filter_required: Default::default(),
365368
clone_behavior_overrides: Default::default(),
366369
clone_queue: Default::default(),
367370
deferred_commands: Default::default(),
@@ -465,6 +468,12 @@ impl EntityCloner {
465468
{
466469
let world = world.as_unsafe_world_cell();
467470
let source_entity = world.get_entity(source).expect("Source entity must exist");
471+
let target_archetype = (!self.filter_required.is_empty()).then(|| {
472+
world
473+
.get_entity(target)
474+
.expect("Target entity must exist")
475+
.archetype()
476+
});
468477

469478
#[cfg(feature = "bevy_reflect")]
470479
// SAFETY: we have unique access to `world`, nothing else accesses the registry at this moment, and we clone
@@ -481,7 +490,7 @@ impl EntityCloner {
481490
bundle_scratch = BundleScratch::with_capacity(archetype.component_count());
482491

483492
for component in archetype.components() {
484-
if !self.is_cloning_allowed(&component) {
493+
if !self.is_cloning_allowed(&component, target_archetype) {
485494
continue;
486495
}
487496

@@ -605,9 +614,19 @@ impl EntityCloner {
605614
target
606615
}
607616

608-
fn is_cloning_allowed(&self, component: &ComponentId) -> bool {
609-
(self.filter_allows_components && self.filter.contains(component))
610-
|| (!self.filter_allows_components && !self.filter.contains(component))
617+
fn is_cloning_allowed(
618+
&self,
619+
component: &ComponentId,
620+
target_archetype: Option<&Archetype>,
621+
) -> bool {
622+
if self.filter_allows_components {
623+
self.filter.contains(component)
624+
|| target_archetype.is_some_and(|archetype| {
625+
!archetype.contains(*component) && self.filter_required.contains(component)
626+
})
627+
} else {
628+
!self.filter.contains(component) && !self.filter_required.contains(component)
629+
}
611630
}
612631
}
613632

@@ -809,9 +828,9 @@ impl<'w> EntityClonerBuilder<'w> {
809828
if let Some(info) = self.world.components().get_info(id) {
810829
for required_id in info.required_components().iter_ids() {
811830
if self.entity_cloner.filter_allows_components {
812-
self.entity_cloner.filter.insert(required_id);
831+
self.entity_cloner.filter_required.insert(required_id);
813832
} else {
814-
self.entity_cloner.filter.remove(&required_id);
833+
self.entity_cloner.filter_required.remove(&required_id);
815834
}
816835
}
817836
}
@@ -829,9 +848,9 @@ impl<'w> EntityClonerBuilder<'w> {
829848
if let Some(info) = self.world.components().get_info(id) {
830849
for required_id in info.required_components().iter_ids() {
831850
if self.entity_cloner.filter_allows_components {
832-
self.entity_cloner.filter.remove(&required_id);
851+
self.entity_cloner.filter_required.remove(&required_id);
833852
} else {
834-
self.entity_cloner.filter.insert(required_id);
853+
self.entity_cloner.filter_required.insert(required_id);
835854
}
836855
}
837856
}
@@ -1406,4 +1425,36 @@ mod tests {
14061425
);
14071426
assert!(world.resource::<FromWorldCalled>().0);
14081427
}
1428+
1429+
#[test]
1430+
fn cloning_with_required_components_preserves_existing() {
1431+
#[derive(Component, Clone, PartialEq, Debug, Default)]
1432+
#[require(B(5))]
1433+
struct A;
1434+
1435+
#[derive(Component, Clone, PartialEq, Debug)]
1436+
struct B(u32);
1437+
1438+
let mut world = World::default();
1439+
1440+
let e = world.spawn((A, B(0))).id();
1441+
let e_clone = world.spawn(B(1)).id();
1442+
1443+
EntityCloner::build(&mut world)
1444+
.deny_all()
1445+
.allow::<A>()
1446+
.clone_entity(e, e_clone);
1447+
1448+
assert_eq!(world.entity(e_clone).get::<A>(), Some(&A));
1449+
assert_eq!(world.entity(e_clone).get::<B>(), Some(&B(1)));
1450+
1451+
let e_clone2 = world.spawn(B(2)).id();
1452+
1453+
EntityCloner::build(&mut world)
1454+
.allow_all()
1455+
.deny::<A>()
1456+
.clone_entity(e, e_clone2);
1457+
1458+
assert_eq!(world.entity(e_clone2).get::<B>(), Some(&B(2)));
1459+
}
14091460
}

0 commit comments

Comments
 (0)