diff --git a/crates/bevy_ecs/macros/src/component.rs b/crates/bevy_ecs/macros/src/component.rs index 53ba284588a91..ef7fad99f410a 100644 --- a/crates/bevy_ecs/macros/src/component.rs +++ b/crates/bevy_ecs/macros/src/component.rs @@ -792,6 +792,11 @@ fn derive_relationship( #relationship_member: entity } } + + #[inline] + fn set_risky(&mut self, entity: Entity) { + self.#relationship_member = entity; + } } })) } diff --git a/crates/bevy_ecs/src/relationship/mod.rs b/crates/bevy_ecs/src/relationship/mod.rs index d570b9fabc670..8830998663fb7 100644 --- a/crates/bevy_ecs/src/relationship/mod.rs +++ b/crates/bevy_ecs/src/relationship/mod.rs @@ -82,6 +82,20 @@ pub trait Relationship: Component + Sized { /// Creates this [`Relationship`] from the given `entity`. fn from(entity: Entity) -> Self; + /// Changes the current [`Entity`] ID of the entity containing the [`RelationshipTarget`] to another one. + /// + /// This is useful for updating the relationship without overwriting other fields stored in `Self`. + /// + /// # Warning + /// + /// This should generally not be called by user code, as modifying the related entity could invalidate the + /// relationship. If this method is used, then the hooks [`on_replace`](Relationship::on_replace) have to + /// run before and [`on_insert`](Relationship::on_insert) after it. + /// This happens automatically when this method is called with [`EntityWorldMut::modify_component`]. + /// + /// Prefer to use regular means of insertions when possible. + fn set_risky(&mut self, entity: Entity); + /// The `on_insert` component hook that maintains the [`Relationship`] / [`RelationshipTarget`] connection. fn on_insert( mut world: DeferredWorld, diff --git a/crates/bevy_ecs/src/relationship/related_methods.rs b/crates/bevy_ecs/src/relationship/related_methods.rs index 1983b6b37c11b..8bae76a84e44b 100644 --- a/crates/bevy_ecs/src/relationship/related_methods.rs +++ b/crates/bevy_ecs/src/relationship/related_methods.rs @@ -6,7 +6,7 @@ use crate::{ Relationship, RelationshipHookMode, RelationshipSourceCollection, RelationshipTarget, }, system::{Commands, EntityCommands}, - world::{EntityWorldMut, World}, + world::{DeferredWorld, EntityWorldMut, World}, }; use bevy_platform::prelude::{Box, Vec}; use core::{marker::PhantomData, mem}; @@ -42,7 +42,12 @@ impl<'w> EntityWorldMut<'w> { let id = self.id(); self.world_scope(|world| { for related in related { - world.entity_mut(*related).insert(R::from(id)); + world + .entity_mut(*related) + .modify_or_insert_relation_with_relationship_hook_mode::( + id, + RelationshipHookMode::Run, + ); } }); self @@ -98,7 +103,12 @@ impl<'w> EntityWorldMut<'w> { .collection_mut_risky() .place(*related, index); } else { - world.entity_mut(*related).insert(R::from(id)); + world + .entity_mut(*related) + .modify_or_insert_relation_with_relationship_hook_mode::( + id, + RelationshipHookMode::Run, + ); world .get_mut::(id) .expect("hooks should have added relationship target") @@ -165,10 +175,13 @@ impl<'w> EntityWorldMut<'w> { } for related in potential_relations { - // SAFETY: We'll manually be adjusting the contents of the parent to fit the final state. + // SAFETY: We'll manually be adjusting the contents of the `RelationshipTarget` to fit the final state. world .entity_mut(related) - .insert_with_relationship_hook_mode(R::from(id), RelationshipHookMode::Skip); + .modify_or_insert_relation_with_relationship_hook_mode::( + id, + RelationshipHookMode::Skip, + ); } }); @@ -266,7 +279,10 @@ impl<'w> EntityWorldMut<'w> { // We changed the target collection manually so don't run the insert hook world .entity_mut(*new_relation) - .insert_with_relationship_hook_mode(R::from(this), RelationshipHookMode::Skip); + .modify_or_insert_relation_with_relationship_hook_mode::( + this, + RelationshipHookMode::Skip, + ); } }); @@ -352,6 +368,40 @@ impl<'w> EntityWorldMut<'w> { self } + + fn modify_or_insert_relation_with_relationship_hook_mode( + &mut self, + entity: Entity, + relationship_hook_mode: RelationshipHookMode, + ) { + // Check if the relation edge holds additional data + if size_of::() > size_of::() { + self.assert_not_despawned(); + + let this = self.id(); + + let modified = self.world_scope(|world| { + let modified = DeferredWorld::from(&mut *world) + .modify_component_with_relationship_hook_mode::( + this, + relationship_hook_mode, + |r| r.set_risky(entity), + ) + .expect("entity access must be valid") + .is_some(); + + world.flush(); + + modified + }); + + if modified { + return; + } + } + + self.insert_with_relationship_hook_mode(R::from(entity), relationship_hook_mode); + } } impl<'a> EntityCommands<'a> { @@ -689,17 +739,132 @@ mod tests { } #[test] - fn replace_related_keeps_data() { + fn add_related_keeps_relationship_data() { + #[derive(Component, PartialEq, Debug)] + #[relationship(relationship_target = Parent)] + struct Child { + #[relationship] + parent: Entity, + data: u8, + } + + #[derive(Component)] + #[relationship_target(relationship = Child)] + struct Parent(Vec); + + let mut world = World::new(); + let parent1 = world.spawn_empty().id(); + let parent2 = world.spawn_empty().id(); + let child = world + .spawn(Child { + parent: parent1, + data: 42, + }) + .id(); + + world.entity_mut(parent2).add_related::(&[child]); + assert_eq!( + world.get::(child), + Some(&Child { + parent: parent2, + data: 42 + }) + ); + } + + #[test] + fn insert_related_keeps_relationship_data() { + #[derive(Component, PartialEq, Debug)] + #[relationship(relationship_target = Parent)] + struct Child { + #[relationship] + parent: Entity, + data: u8, + } + + #[derive(Component)] + #[relationship_target(relationship = Child)] + struct Parent(Vec); + + let mut world = World::new(); + let parent1 = world.spawn_empty().id(); + let parent2 = world.spawn_empty().id(); + let child = world + .spawn(Child { + parent: parent1, + data: 42, + }) + .id(); + + world + .entity_mut(parent2) + .insert_related::(0, &[child]); + assert_eq!( + world.get::(child), + Some(&Child { + parent: parent2, + data: 42 + }) + ); + } + + #[test] + fn replace_related_keeps_relationship_data() { + #[derive(Component, PartialEq, Debug)] + #[relationship(relationship_target = Parent)] + struct Child { + #[relationship] + parent: Entity, + data: u8, + } + + #[derive(Component)] + #[relationship_target(relationship = Child)] + struct Parent(Vec); + + let mut world = World::new(); + let parent1 = world.spawn_empty().id(); + let parent2 = world.spawn_empty().id(); + let child = world + .spawn(Child { + parent: parent1, + data: 42, + }) + .id(); + + world + .entity_mut(parent2) + .replace_related_with_difference::(&[], &[child], &[child]); + assert_eq!( + world.get::(child), + Some(&Child { + parent: parent2, + data: 42 + }) + ); + + world.entity_mut(parent1).replace_related::(&[child]); + assert_eq!( + world.get::(child), + Some(&Child { + parent: parent1, + data: 42 + }) + ); + } + + #[test] + fn replace_related_keeps_relationship_target_data() { #[derive(Component)] #[relationship(relationship_target = Parent)] - pub struct Child(Entity); + struct Child(Entity); #[derive(Component)] #[relationship_target(relationship = Child)] - pub struct Parent { + struct Parent { #[relationship] children: Vec, - pub data: u8, + data: u8, } let mut world = World::new(); diff --git a/crates/bevy_ecs/src/world/deferred_world.rs b/crates/bevy_ecs/src/world/deferred_world.rs index f59f9e88dca28..1699eadcff85b 100644 --- a/crates/bevy_ecs/src/world/deferred_world.rs +++ b/crates/bevy_ecs/src/world/deferred_world.rs @@ -97,9 +97,10 @@ impl<'w> DeferredWorld<'w> { /// is mutable, prefer using [`get_mut`](DeferredWorld::get_mut). #[inline] #[track_caller] - pub(crate) fn modify_component( + pub(crate) fn modify_component_with_relationship_hook_mode( &mut self, entity: Entity, + relationship_hook_mode: RelationshipHookMode, f: impl FnOnce(&mut T) -> R, ) -> Result, EntityMutableFetchError> { // If the component is not registered, then it doesn't exist on this entity, so no action required. @@ -107,12 +108,17 @@ impl<'w> DeferredWorld<'w> { return Ok(None); }; - self.modify_component_by_id(entity, component_id, move |component| { - // SAFETY: component matches the component_id collected in the above line - let mut component = unsafe { component.with_type::() }; + self.modify_component_by_id_with_relationship_hook_mode( + entity, + component_id, + relationship_hook_mode, + move |component| { + // SAFETY: component matches the component_id collected in the above line + let mut component = unsafe { component.with_type::() }; - f(&mut component) - }) + f(&mut component) + }, + ) } /// Temporarily removes a [`Component`] identified by the provided @@ -127,14 +133,15 @@ impl<'w> DeferredWorld<'w> { /// If you do not need to ensure the above hooks are triggered, and your component /// is mutable, prefer using [`get_mut_by_id`](DeferredWorld::get_mut_by_id). /// - /// You should prefer the typed [`modify_component`](DeferredWorld::modify_component) + /// You should prefer the typed [`modify_component_with_relationship_hook_mode`](DeferredWorld::modify_component_with_relationship_hook_mode) /// whenever possible. #[inline] #[track_caller] - pub(crate) fn modify_component_by_id( + pub(crate) fn modify_component_by_id_with_relationship_hook_mode( &mut self, entity: Entity, component_id: ComponentId, + relationship_hook_mode: RelationshipHookMode, f: impl for<'a> FnOnce(MutUntyped<'a>) -> R, ) -> Result, EntityMutableFetchError> { let entity_cell = self.get_entity_mut(entity)?; @@ -157,7 +164,7 @@ impl<'w> DeferredWorld<'w> { entity, [component_id].into_iter(), MaybeLocation::caller(), - RelationshipHookMode::Run, + relationship_hook_mode, ); if archetype.has_replace_observer() { self.trigger_observers( @@ -197,7 +204,7 @@ impl<'w> DeferredWorld<'w> { entity, [component_id].into_iter(), MaybeLocation::caller(), - RelationshipHookMode::Run, + relationship_hook_mode, ); if archetype.has_insert_observer() { self.trigger_observers( diff --git a/crates/bevy_ecs/src/world/mod.rs b/crates/bevy_ecs/src/world/mod.rs index 798fd8ef60ae4..2fb8f2d0eb856 100644 --- a/crates/bevy_ecs/src/world/mod.rs +++ b/crates/bevy_ecs/src/world/mod.rs @@ -1319,7 +1319,11 @@ impl World { ) -> Result, EntityMutableFetchError> { let mut world = DeferredWorld::from(&mut *self); - let result = world.modify_component(entity, f)?; + let result = world.modify_component_with_relationship_hook_mode( + entity, + RelationshipHookMode::Run, + f, + )?; self.flush(); Ok(result) @@ -1349,7 +1353,12 @@ impl World { ) -> Result, EntityMutableFetchError> { let mut world = DeferredWorld::from(&mut *self); - let result = world.modify_component_by_id(entity, component_id, f)?; + let result = world.modify_component_by_id_with_relationship_hook_mode( + entity, + component_id, + RelationshipHookMode::Run, + f, + )?; self.flush(); Ok(result) diff --git a/release-content/migration-guides/relationship_set_risky.md b/release-content/migration-guides/relationship_set_risky.md new file mode 100644 index 0000000000000..72ef0ee04957b --- /dev/null +++ b/release-content/migration-guides/relationship_set_risky.md @@ -0,0 +1,41 @@ +--- +title: Relationship method set_risky +pull_requests: [19601] +--- + +The trait `Relationship` received a new method, `set_risky`. It is used to alter the entity ID of the entity that contains its `RelationshipTarget` counterpart. +This is needed to leave [other data you can store in these components](https://docs.rs/bevy/latest/bevy/ecs/relationship/trait.Relationship.html#derive) +unchanged at operations that reassign the relationship target, for example `EntityCommands::add_related`. Previously this could have caused the +data to be reset to its default value which may not be what you wanted to happen. + +Manually overwriting the component is still possible everywhere the full component is inserted: + +```rs +#[derive(Component)] +#[relationship(relationship_target = CarOwner)] +struct OwnedCar { + #[relationship] + owner: Entity, + first_owner: Option, // None if `owner` is the first one +} + +#[derive(Component)] +#[relationship_target(relationship = OwnedCar)] +struct CarOwner(Vec); + +let mut me_entity_mut = world.entity_mut(me_entity); + +// if `car_entity` already contains `OwnedCar`, then the first owner remains unchanged +me_entity_mut.add_one_related::(car_entity); + +// if `car_entity` already contains `OwnedCar`, then the first owner is overwritten with None here +car_entity_mut.insert(OwnedCar { + owner: me_entity, + first_owner: None // I swear it is not stolen officer! +}); +``` + +The new method should not be called by user code as that can invalidate the relationship it had or will have. + +If you implement `Relationship` manually (which is strongly discouraged) then this method needs to overwrite the `Entity` +used for the relationship.