Skip to content

Commit 069f110

Browse files
muddxyiichescock
andauthored
Rework one-to-one relationships to remove existing instead of panicking (#20232)
# Objective - Issue: #18847 - Resolves panic when establishing a one-to-one relationship to an entity that already has an existing relationship from another entity. ## Solution - Modified the `on_insert` hook in the `Relationship` trait to detect one-to-one relationships (where the target collection type is `Entity`) and automatically remove any existing relationship before establishing the new one. - Uses runtime type checking with `TypeId` to identify one-to-one vs one-to-many relationships. - Safely removes the existing source relationship using `try_remove()` to handle edge cases gracefully. ## Testing - Removed panic assertions from `Entity::add()` and `Entity::extend_from_iter()` methods that would previously crash when attempting to establish overlapping one-to-one relationships - Modified existing test `one_to_one_relationship_shared_target` by removing `#[should_panic]` and adding assertions to verify:   - Original relationship is properly removed   - New relationship is correctly established   - Target entity points to the new source --------- Co-authored-by: Chris Russell <[email protected]>
1 parent 5bb8f29 commit 069f110

File tree

2 files changed

+45
-14
lines changed

2 files changed

+45
-14
lines changed

crates/bevy_ecs/src/relationship/mod.rs

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -128,6 +128,21 @@ pub trait Relationship: Component + Sized {
128128
world.commands().entity(entity).remove::<Self>();
129129
return;
130130
}
131+
// For one-to-one relationships, remove existing relationship before adding new one
132+
let current_source_to_remove = world
133+
.get_entity(target_entity)
134+
.ok()
135+
.and_then(|target_entity_ref| target_entity_ref.get::<Self::RelationshipTarget>())
136+
.and_then(|relationship_target| {
137+
relationship_target
138+
.collection()
139+
.source_to_remove_before_add()
140+
});
141+
142+
if let Some(current_source) = current_source_to_remove {
143+
world.commands().entity(current_source).try_remove::<Self>();
144+
}
145+
131146
if let Ok(mut entity_commands) = world.commands().get_entity(target_entity) {
132147
// Deferring is necessary for batch mode
133148
entity_commands

crates/bevy_ecs/src/relationship/relationship_source_collection.rs

Lines changed: 30 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -69,6 +69,12 @@ pub trait RelationshipSourceCollection {
6969
self.len() == 0
7070
}
7171

72+
/// For one-to-one relationships, returns the entity that should be removed before adding a new one.
73+
/// Returns `None` for one-to-many relationships or when no entity needs to be removed.
74+
fn source_to_remove_before_add(&self) -> Option<Entity> {
75+
None
76+
}
77+
7278
/// Add multiple entities to collection at once.
7379
///
7480
/// May be faster than repeatedly calling [`Self::add`].
@@ -345,14 +351,7 @@ impl RelationshipSourceCollection for Entity {
345351
}
346352

347353
fn add(&mut self, entity: Entity) -> bool {
348-
assert_eq!(
349-
*self,
350-
Entity::PLACEHOLDER,
351-
"Entity {entity} attempted to target an entity with a one-to-one relationship, but it is already targeted by {}. You must remove the original relationship first.",
352-
*self
353-
);
354354
*self = entity;
355-
356355
true
357356
}
358357

@@ -389,15 +388,17 @@ impl RelationshipSourceCollection for Entity {
389388

390389
fn extend_from_iter(&mut self, entities: impl IntoIterator<Item = Entity>) {
391390
for entity in entities {
392-
assert_eq!(
393-
*self,
394-
Entity::PLACEHOLDER,
395-
"Entity {entity} attempted to target an entity with a one-to-one relationship, but it is already targeted by {}. You must remove the original relationship first.",
396-
*self
397-
);
398391
*self = entity;
399392
}
400393
}
394+
395+
fn source_to_remove_before_add(&self) -> Option<Entity> {
396+
if *self != Entity::PLACEHOLDER {
397+
Some(*self)
398+
} else {
399+
None
400+
}
401+
}
401402
}
402403

403404
impl<const N: usize> OrderedRelationshipSourceCollection for SmallVec<[Entity; N]> {
@@ -724,7 +725,6 @@ mod tests {
724725
}
725726

726727
#[test]
727-
#[should_panic]
728728
fn one_to_one_relationship_shared_target() {
729729
#[derive(Component)]
730730
#[relationship(relationship_target = Below)]
@@ -740,6 +740,22 @@ mod tests {
740740

741741
world.entity_mut(a).insert(Above(c));
742742
world.entity_mut(b).insert(Above(c));
743+
744+
// The original relationship (a -> c) should be removed and the new relationship (b -> c) should be established
745+
assert!(
746+
world.get::<Above>(a).is_none(),
747+
"Original relationship should be removed"
748+
);
749+
assert_eq!(
750+
world.get::<Above>(b).unwrap().0,
751+
c,
752+
"New relationship should be established"
753+
);
754+
assert_eq!(
755+
world.get::<Below>(c).unwrap().0,
756+
b,
757+
"Target should point to new source"
758+
);
743759
}
744760

745761
#[test]

0 commit comments

Comments
 (0)