From 7e63d608b34b4622790c8930e90255f7ce48a803 Mon Sep 17 00:00:00 2001 From: Guillaume Wafo-Tapa Date: Thu, 3 Jul 2025 12:36:28 +0200 Subject: [PATCH 1/4] Defer the whole operation of the on_insert hook --- crates/bevy_ecs/src/relationship/mod.rs | 57 +++++++++++++++---------- 1 file changed, 35 insertions(+), 22 deletions(-) diff --git a/crates/bevy_ecs/src/relationship/mod.rs b/crates/bevy_ecs/src/relationship/mod.rs index f95214262b548..180365ebbd751 100644 --- a/crates/bevy_ecs/src/relationship/mod.rs +++ b/crates/bevy_ecs/src/relationship/mod.rs @@ -16,7 +16,7 @@ use crate::{ entity::{ComponentCloneCtx, Entity, SourceComponent}, error::CommandWithEntity, lifecycle::HookContext, - world::{DeferredWorld, EntityWorldMut}, + world::{DeferredWorld, EntityWorldMut, World}, }; use log::warn; @@ -126,27 +126,40 @@ pub trait Relationship: Component + Sized { world.commands().entity(entity).remove::(); return; } - if let Ok(mut entity_commands) = world.commands().get_entity(target_entity) { - // Deferring is necessary for batch mode - entity_commands - .entry::() - .and_modify(move |mut relationship_target| { - relationship_target.collection_mut_risky().add(entity); - }) - .or_insert_with(|| { - let mut target = Self::RelationshipTarget::with_capacity(1); - target.collection_mut_risky().add(entity); - target - }); - } else { - warn!( - "{}The {}({target_entity:?}) relationship on entity {entity:?} relates to an entity that does not exist. The invalid {} relationship has been removed.", - caller.map(|location|format!("{location}: ")).unwrap_or_default(), - DebugName::type_name::(), - DebugName::type_name::() - ); - world.commands().entity(entity).remove::(); - } + + // Deferring is necessary for batch mode + world.commands().queue(move |world: &mut World| { + if let Ok(mut target_entity_world) = world.get_entity_mut(target_entity) { + target_entity_world + .entry::() + .and_modify(move |mut relationship_target| { + relationship_target.collection_mut_risky().add(entity); + }) + .or_insert_with(|| { + let mut target = Self::RelationshipTarget::with_capacity(1); + target.collection_mut_risky().add(entity); + target + }); + } else if let Ok(mut entity_world) = world.get_entity_mut(entity) { + warn!( + "{}The {}({target_entity:?}) relationship on entity {entity:?} relates to an entity that does not exist. The invalid {} relationship has been removed.", + caller + .map(|location| format!("{location}: ")) + .unwrap_or_default(), + DebugName::type_name::(), + DebugName::type_name::() + ); + entity_world.remove::(); + } else { + warn!( + "{}The {} relationship between entity {entity:?} and its target {target_entity:?} could not be set up because these entities don't exist.", + caller + .map(|location| format!("{location}: ")) + .unwrap_or_default(), + DebugName::type_name::(), + ); + } + }); } /// The `on_replace` component hook that maintains the [`Relationship`] / [`RelationshipTarget`] connection. From a1df0865603d44b583a19ec3b3f8e15f75560723 Mon Sep 17 00:00:00 2001 From: Guillaume Wafo-Tapa Date: Fri, 4 Jul 2025 19:17:18 +0200 Subject: [PATCH 2/4] Update the comment explaining why we defer --- crates/bevy_ecs/src/relationship/mod.rs | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/relationship/mod.rs b/crates/bevy_ecs/src/relationship/mod.rs index 180365ebbd751..60b7d15aa1cc4 100644 --- a/crates/bevy_ecs/src/relationship/mod.rs +++ b/crates/bevy_ecs/src/relationship/mod.rs @@ -127,7 +127,11 @@ pub trait Relationship: Component + Sized { return; } - // Deferring is necessary for batch mode + // We defer the insertion of RelationshipTarget in the target. Otherwise this is problematic + // when inserting in batch if the target doesn't have the component yet. In that case, since + // the hooks run with no flush in between, we end up deferring a batch of commands all + // inserting RelationshipTarget with a single entity, each command overwriting the previous + // one when applied, resulting in the target having an only child instead of the whole batch. world.commands().queue(move |world: &mut World| { if let Ok(mut target_entity_world) = world.get_entity_mut(target_entity) { target_entity_world From 106e8a2f2a39514166f0edd536da28d647a8834d Mon Sep 17 00:00:00 2001 From: Guillaume Wafo-Tapa Date: Fri, 4 Jul 2025 19:38:29 +0200 Subject: [PATCH 3/4] Shorten a call --- crates/bevy_ecs/src/relationship/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/bevy_ecs/src/relationship/mod.rs b/crates/bevy_ecs/src/relationship/mod.rs index 60b7d15aa1cc4..8df80bc93f0e8 100644 --- a/crates/bevy_ecs/src/relationship/mod.rs +++ b/crates/bevy_ecs/src/relationship/mod.rs @@ -115,7 +115,7 @@ pub trait Relationship: Component + Sized { } } } - let target_entity = world.entity(entity).get::().unwrap().get(); + let target_entity = world.get::(entity).unwrap().get(); if target_entity == entity { warn!( "{}The {}({target_entity:?}) relationship on entity {entity:?} points to itself. The invalid {} relationship has been removed.", From 370698c3fc52bfae5f40d54201e8091faa14e6ce Mon Sep 17 00:00:00 2001 From: Guillaume Wafo-Tapa Date: Fri, 4 Jul 2025 22:19:53 +0200 Subject: [PATCH 4/4] Refine the comment --- crates/bevy_ecs/src/relationship/mod.rs | 11 ++++++----- 1 file changed, 6 insertions(+), 5 deletions(-) diff --git a/crates/bevy_ecs/src/relationship/mod.rs b/crates/bevy_ecs/src/relationship/mod.rs index 8df80bc93f0e8..74e74eb6f5b2d 100644 --- a/crates/bevy_ecs/src/relationship/mod.rs +++ b/crates/bevy_ecs/src/relationship/mod.rs @@ -127,11 +127,12 @@ pub trait Relationship: Component + Sized { return; } - // We defer the insertion of RelationshipTarget in the target. Otherwise this is problematic - // when inserting in batch if the target doesn't have the component yet. In that case, since - // the hooks run with no flush in between, we end up deferring a batch of commands all - // inserting RelationshipTarget with a single entity, each command overwriting the previous - // one when applied, resulting in the target having an only child instead of the whole batch. + // We defer the decision of inserting or modifying RelationshipTarget. Deciding now to defer + // either an insertion or a modification causes an issue when inserting in batch if the + // target doesn't have the component yet. In that case, since the hooks run with no flush in + // between, we end up deferring a batch of commands all inserting RelationshipTarget with a + // single entity, each command overwriting the previous one when applied, resulting in the + // target having an only child instead of the whole batch. world.commands().queue(move |world: &mut World| { if let Ok(mut target_entity_world) = world.get_entity_mut(target_entity) { target_entity_world