forestia/bevy
2
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

EntityWorldMut methods do not automatically overwrite Relationship components (#19601)

Browse Source 
# Objective

Some methods and commands carelessly overwrite `Relationship`
components. This may overwrite additional data stored at them which is
undesired.

Part of #19589

## Solution

A new private method will be used instead of insert:
`modify_or_insert_relation_with_relationship_hook_mode`.

This method behaves different to `insert` if `Relationship` is a larger
type than `Entity` and already contains this component. It will then use
the `modify_component` API and a new `Relationship::set_risky` method to
set the related entity, keeping all other data untouched.

For the `replace_related`(`_with_difference`) methods this also required
a `InsertHookMode` parameter for efficient modifications of multiple
children. The changes here are limited to the non-public methods.

I would appreciate feedback if this is all good.

# Testing

Added tests of all methods that previously could reset `Relationship`
data.

---------

Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
This commit is contained in:
urben1680 2025-06-22 02:22:05 +02:00 committed by GitHub
parent 2d897380a0
commit c6ae964709
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
6 changed files with 264 additions and 23 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

5
crates/bevy_ecs/macros/src/component.rs
View File

@ -792,6 +792,11 @@ fn derive_relationship(
#relationship_member: entity
}
}
#[inline]
fn set_risky(&mut self, entity: Entity) {
self.#relationship_member = entity;
}
}
}))
}

14
crates/bevy_ecs/src/relationship/mod.rs
View File

@ -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,

187
crates/bevy_ecs/src/relationship/related_methods.rs
View File

@ -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::<R>(
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::<R>(
id,
RelationshipHookMode::Run,
);
world
.get_mut::<R::RelationshipTarget>(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::<R>(
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::<R>(
this,
RelationshipHookMode::Skip,
);
}
});
@ -352,6 +368,40 @@ impl<'w> EntityWorldMut<'w> {
self
}
fn modify_or_insert_relation_with_relationship_hook_mode<R: Relationship>(
&mut self,
entity: Entity,
relationship_hook_mode: RelationshipHookMode,
) {
// Check if the relation edge holds additional data
if size_of::<R>() > size_of::<Entity>() {
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::<R, _>(
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() {
#[derive(Component)]
fn add_related_keeps_relationship_data() {
#[derive(Component, PartialEq, Debug)]
#[relationship(relationship_target = Parent)]
pub struct Child(Entity);
struct Child {
#[relationship]
parent: Entity,
data: u8,
}
#[derive(Component)]
#[relationship_target(relationship = Child)]
pub struct Parent {
struct Parent(Vec<Entity>);
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>(&[child]);
assert_eq!(
world.get::<Child>(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<Entity>);
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::<Child>(0, &[child]);
assert_eq!(
world.get::<Child>(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<Entity>);
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], &[child]);
assert_eq!(
world.get::<Child>(child),
Some(&Child {
parent: parent2,
data: 42
})
);
world.entity_mut(parent1).replace_related::<Child>(&[child]);
assert_eq!(
world.get::<Child>(child),
Some(&Child {
parent: parent1,
data: 42
})
);
}
#[test]
fn replace_related_keeps_relationship_target_data() {
#[derive(Component)]
#[relationship(relationship_target = Parent)]
struct Child(Entity);
#[derive(Component)]
#[relationship_target(relationship = Child)]
struct Parent {
#[relationship]
children: Vec<Entity>,
pub data: u8,
data: u8,
}
let mut world = World::new();

27
crates/bevy_ecs/src/world/deferred_world.rs
View File

@ -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<T: Component, R>(
pub(crate) fn modify_component_with_relationship_hook_mode<T: Component, R>(
&mut self,
entity: Entity,
relationship_hook_mode: RelationshipHookMode,
f: impl FnOnce(&mut T) -> R,
) -> Result<Option<R>, 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::<T>() };
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::<T>() };
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<R>(
pub(crate) fn modify_component_by_id_with_relationship_hook_mode<R>(
&mut self,
entity: Entity,
component_id: ComponentId,
relationship_hook_mode: RelationshipHookMode,
f: impl for<'a> FnOnce(MutUntyped<'a>) -> R,
) -> Result<Option<R>, 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(

13
crates/bevy_ecs/src/world/mod.rs
View File

@ -1319,7 +1319,11 @@ impl World {
) -> Result<Option<R>, 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<Option<R>, 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)

41
release-content/migration-guides/relationship_set_risky.md Normal file
View File

@ -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<Entity>, // None if `owner` is the first one
}
#[derive(Component)]
#[relationship_target(relationship = OwnedCar)]
struct CarOwner(Vec<Entity>);
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::<OwnedCar>(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.