Skip to content

Commit 6d60541

Browse files
cartcart
authored
Support skipping Relationship on_replace hooks (#18378)
# Objective Fixes #18357 ## Solution Generalize `RelationshipInsertHookMode` to `RelationshipHookMode`, wire it up to on_replace execution, and use it in the `Relationship::on_replace` hook.
1 parent ac53e4c commit 6d60541

File tree

13 files changed

+180
-158
lines changed

13 files changed

+180
-158
lines changed

‎crates/bevy_ecs/src/bundle.rs

Lines changed: 6 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -18,7 +18,7 @@ use crate::{
1818
observer::Observers,
1919
prelude::World,
2020
query::DebugCheckedUnwrap,
21-
relationship::RelationshipInsertHookMode,
21+
relationship::RelationshipHookMode,
2222
storage::{SparseSetIndex, SparseSets, Storages, Table, TableRow},
2323
world::{unsafe_world_cell::UnsafeWorldCell, EntityWorldMut, ON_ADD, ON_INSERT, ON_REPLACE},
2424
};
@@ -1104,7 +1104,7 @@ impl<'w> BundleInserter<'w> {
11041104
bundle: T,
11051105
insert_mode: InsertMode,
11061106
caller: MaybeLocation,
1107-
relationship_insert_hook_mode: RelationshipInsertHookMode,
1107+
relationship_hook_mode: RelationshipHookMode,
11081108
) -> (EntityLocation, T::Effect) {
11091109
let bundle_info = self.bundle_info.as_ref();
11101110
let archetype_after_insert = self.archetype_after_insert.as_ref();
@@ -1130,6 +1130,7 @@ impl<'w> BundleInserter<'w> {
11301130
entity,
11311131
archetype_after_insert.iter_existing(),
11321132
caller,
1133+
relationship_hook_mode,
11331134
);
11341135
}
11351136
}
@@ -1317,7 +1318,7 @@ impl<'w> BundleInserter<'w> {
13171318
entity,
13181319
archetype_after_insert.iter_inserted(),
13191320
caller,
1320-
relationship_insert_hook_mode,
1321+
relationship_hook_mode,
13211322
);
13221323
if new_archetype.has_insert_observer() {
13231324
deferred_world.trigger_observers(
@@ -1336,7 +1337,7 @@ impl<'w> BundleInserter<'w> {
13361337
entity,
13371338
archetype_after_insert.iter_added(),
13381339
caller,
1339-
relationship_insert_hook_mode,
1340+
relationship_hook_mode,
13401341
);
13411342
if new_archetype.has_insert_observer() {
13421343
deferred_world.trigger_observers(
@@ -1484,7 +1485,7 @@ impl<'w> BundleSpawner<'w> {
14841485
entity,
14851486
bundle_info.iter_contributed_components(),
14861487
caller,
1487-
RelationshipInsertHookMode::Run,
1488+
RelationshipHookMode::Run,
14881489
);
14891490
if archetype.has_insert_observer() {
14901491
deferred_world.trigger_observers(

‎crates/bevy_ecs/src/component.rs

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6,7 +6,7 @@ use crate::{
66
change_detection::{MaybeLocation, MAX_CHANGE_AGE},
77
entity::{ComponentCloneCtx, Entity, SourceComponent},
88
query::DebugCheckedUnwrap,
9-
relationship::RelationshipInsertHookMode,
9+
relationship::RelationshipHookMode,
1010
resource::Resource,
1111
storage::{SparseSetIndex, SparseSets, Table, TableRow},
1212
system::{Commands, Local, SystemParam},
@@ -584,7 +584,7 @@ pub struct HookContext {
584584
/// The caller location is `Some` if the `track_caller` feature is enabled.
585585
pub caller: MaybeLocation,
586586
/// Configures how relationship hooks will run
587-
pub relationship_insert_hook_mode: RelationshipInsertHookMode,
587+
pub relationship_hook_mode: RelationshipHookMode,
588588
}
589589

590590
/// [`World`]-mutating functions that run as part of lifecycle events of a [`Component`].

‎crates/bevy_ecs/src/entity/clone_entities.rs

Lines changed: 6 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,7 @@ use alloc::boxed::Box;
1010
use crate::component::{ComponentCloneBehavior, ComponentCloneFn};
1111
use crate::entity::hash_map::EntityHashMap;
1212
use crate::entity::{Entities, EntityMapper};
13-
use crate::relationship::RelationshipInsertHookMode;
13+
use crate::relationship::RelationshipHookMode;
1414
use crate::system::Commands;
1515
use crate::{
1616
bundle::Bundle,
@@ -415,7 +415,7 @@ impl<'a> BundleScratch<'a> {
415415
self,
416416
world: &mut World,
417417
entity: Entity,
418-
relationship_hook_insert_mode: RelationshipInsertHookMode,
418+
relationship_hook_insert_mode: RelationshipHookMode,
419419
) {
420420
// SAFETY:
421421
// - All `component_ids` are from the same world as `target` entity
@@ -453,7 +453,7 @@ impl EntityCloner {
453453
world: &mut World,
454454
source: Entity,
455455
mapper: &mut dyn EntityMapper,
456-
relationship_hook_insert_mode: RelationshipInsertHookMode,
456+
relationship_hook_insert_mode: RelationshipHookMode,
457457
) -> Entity {
458458
let target = mapper.get_mapped(source);
459459
// PERF: reusing allocated space across clones would be more efficient. Consider an allocation model similar to `Commands`.
@@ -581,16 +581,15 @@ impl EntityCloner {
581581
mapper: &mut dyn EntityMapper,
582582
) -> Entity {
583583
// All relationships on the root should have their hooks run
584-
let target =
585-
self.clone_entity_internal(world, source, mapper, RelationshipInsertHookMode::Run);
584+
let target = self.clone_entity_internal(world, source, mapper, RelationshipHookMode::Run);
586585
let child_hook_insert_mode = if self.linked_cloning {
587586
// When spawning "linked relationships", we want to ignore hooks for relationships we are spawning, while
588587
// still registering with original relationship targets that are "not linked" to the current recursive spawn.
589-
RelationshipInsertHookMode::RunIfNotLinked
588+
RelationshipHookMode::RunIfNotLinked
590589
} else {
591590
// If we are not cloning "linked relationships" recursively, then we want any cloned relationship components to
592591
// register themselves with their original relationship target.
593-
RelationshipInsertHookMode::Run
592+
RelationshipHookMode::Run
594593
};
595594
loop {
596595
let queued = self.clone_queue.pop_front();

‎crates/bevy_ecs/src/hierarchy.rs

Lines changed: 18 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -331,7 +331,7 @@ mod tests {
331331
use crate::{
332332
entity::Entity,
333333
hierarchy::{ChildOf, Children},
334-
relationship::RelationshipTarget,
334+
relationship::{RelationshipHookMode, RelationshipTarget},
335335
spawn::{Spawn, SpawnRelated},
336336
world::World,
337337
};
@@ -492,4 +492,21 @@ mod tests {
492492
let id = world.spawn(Children::spawn((Spawn(()), Spawn(())))).id();
493493
assert_eq!(world.entity(id).get::<Children>().unwrap().len(), 2,);
494494
}
495+
496+
#[test]
497+
fn child_replace_hook_skip() {
498+
let mut world = World::new();
499+
let parent = world.spawn_empty().id();
500+
let other = world.spawn_empty().id();
501+
let child = world.spawn(ChildOf { parent }).id();
502+
world.entity_mut(child).insert_with_relationship_hook_mode(
503+
ChildOf { parent: other },
504+
RelationshipHookMode::Skip,
505+
);
506+
assert_eq!(
507+
&**world.entity(parent).get::<Children>().unwrap(),
508+
&[child],
509+
"Children should still have the old value, as on_insert/on_replace didn't run"
510+
);
511+
}
495512
}

‎crates/bevy_ecs/src/reflect/bundle.rs

Lines changed: 49 additions & 52 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@ use crate::{
1111
bundle::BundleFromComponents,
1212
entity::EntityMapper,
1313
prelude::Bundle,
14-
relationship::RelationshipInsertHookMode,
14+
relationship::RelationshipHookMode,
1515
world::{EntityMut, EntityWorldMut},
1616
};
1717
use bevy_reflect::{
@@ -42,7 +42,7 @@ pub struct ReflectBundleFns {
4242
&dyn PartialReflect,
4343
&TypeRegistry,
4444
&mut dyn EntityMapper,
45-
RelationshipInsertHookMode,
45+
RelationshipHookMode,
4646
),
4747
/// Function pointer implementing [`ReflectBundle::remove`].
4848
pub remove: fn(&mut EntityWorldMut),
@@ -93,15 +93,9 @@ impl ReflectBundle {
9393
bundle: &dyn PartialReflect,
9494
registry: &TypeRegistry,
9595
mapper: &mut dyn EntityMapper,
96-
relationship_insert_hook_mode: RelationshipInsertHookMode,
96+
relationship_hook_mode: RelationshipHookMode,
9797
) {
98-
(self.0.apply_or_insert_mapped)(
99-
entity,
100-
bundle,
101-
registry,
102-
mapper,
103-
relationship_insert_hook_mode,
104-
);
98+
(self.0.apply_or_insert_mapped)(entity, bundle, registry, mapper, relationship_hook_mode);
10599
}
106100

107101
/// Removes this [`Bundle`] type from the entity. Does nothing if it doesn't exist.
@@ -183,46 +177,49 @@ impl<B: Bundle + Reflect + TypePath + BundleFromComponents> FromType<B> for Refl
183177
}
184178
}
185179
},
186-
apply_or_insert_mapped:
187-
|entity, reflected_bundle, registry, mapper, relationship_insert_hook_mode| {
188-
if let Some(reflect_component) =
189-
registry.get_type_data::<ReflectComponent>(TypeId::of::<B>())
190-
{
191-
reflect_component.apply_or_insert_mapped(
192-
entity,
193-
reflected_bundle,
194-
registry,
195-
mapper,
196-
relationship_insert_hook_mode,
197-
);
198-
} else {
199-
match reflected_bundle.reflect_ref() {
200-
ReflectRef::Struct(bundle) => bundle.iter_fields().for_each(|field| {
201-
apply_or_insert_field_mapped(
202-
entity,
203-
field,
204-
registry,
205-
mapper,
206-
relationship_insert_hook_mode,
207-
);
208-
}),
209-
ReflectRef::Tuple(bundle) => bundle.iter_fields().for_each(|field| {
210-
apply_or_insert_field_mapped(
211-
entity,
212-
field,
213-
registry,
214-
mapper,
215-
relationship_insert_hook_mode,
216-
);
217-
}),
218-
_ => panic!(
219-
"expected bundle `{}` to be a named struct or tuple",
220-
// FIXME: once we have unique reflect, use `TypePath`.
221-
core::any::type_name::<B>(),
222-
),
223-
}
180+
apply_or_insert_mapped: |entity,
181+
reflected_bundle,
182+
registry,
183+
mapper,
184+
relationship_hook_mode| {
185+
if let Some(reflect_component) =
186+
registry.get_type_data::<ReflectComponent>(TypeId::of::<B>())
187+
{
188+
reflect_component.apply_or_insert_mapped(
189+
entity,
190+
reflected_bundle,
191+
registry,
192+
mapper,
193+
relationship_hook_mode,
194+
);
195+
} else {
196+
match reflected_bundle.reflect_ref() {
197+
ReflectRef::Struct(bundle) => bundle.iter_fields().for_each(|field| {
198+
apply_or_insert_field_mapped(
199+
entity,
200+
field,
201+
registry,
202+
mapper,
203+
relationship_hook_mode,
204+
);
205+
}),
206+
ReflectRef::Tuple(bundle) => bundle.iter_fields().for_each(|field| {
207+
apply_or_insert_field_mapped(
208+
entity,
209+
field,
210+
registry,
211+
mapper,
212+
relationship_hook_mode,
213+
);
214+
}),
215+
_ => panic!(
216+
"expected bundle `{}` to be a named struct or tuple",
217+
// FIXME: once we have unique reflect, use `TypePath`.
218+
core::any::type_name::<B>(),
219+
),
224220
}
225-
},
221+
}
222+
},
226223
remove: |entity| {
227224
entity.remove::<B>();
228225
},
@@ -259,7 +256,7 @@ fn apply_or_insert_field_mapped(
259256
field: &dyn PartialReflect,
260257
registry: &TypeRegistry,
261258
mapper: &mut dyn EntityMapper,
262-
relationship_insert_hook_mode: RelationshipInsertHookMode,
259+
relationship_hook_mode: RelationshipHookMode,
263260
) {
264261
let Some(type_id) = field.try_as_reflect().map(Any::type_id) else {
265262
panic!(
@@ -274,15 +271,15 @@ fn apply_or_insert_field_mapped(
274271
field,
275272
registry,
276273
mapper,
277-
relationship_insert_hook_mode,
274+
relationship_hook_mode,
278275
);
279276
} else if let Some(reflect_bundle) = registry.get_type_data::<ReflectBundle>(type_id) {
280277
reflect_bundle.apply_or_insert_mapped(
281278
entity,
282279
field,
283280
registry,
284281
mapper,
285-
relationship_insert_hook_mode,
282+
relationship_hook_mode,
286283
);
287284
} else {
288285
let is_component = entity.world().components().get_id(type_id).is_some();

‎crates/bevy_ecs/src/reflect/component.rs

Lines changed: 25 additions & 38 deletions
Original file line numberDiff line numberDiff line change
@@ -63,7 +63,7 @@ use crate::{
6363
component::{ComponentId, ComponentMutability},
6464
entity::{Entity, EntityMapper},
6565
prelude::Component,
66-
relationship::RelationshipInsertHookMode,
66+
relationship::RelationshipHookMode,
6767
world::{
6868
unsafe_world_cell::UnsafeEntityCell, EntityMut, EntityWorldMut, FilteredEntityMut,
6969
FilteredEntityRef, World,
@@ -111,7 +111,7 @@ pub struct ReflectComponentFns {
111111
&dyn PartialReflect,
112112
&TypeRegistry,
113113
&mut dyn EntityMapper,
114-
RelationshipInsertHookMode,
114+
RelationshipHookMode,
115115
),
116116
/// Function pointer implementing [`ReflectComponent::remove()`].
117117
pub remove: fn(&mut EntityWorldMut),
@@ -180,15 +180,9 @@ impl ReflectComponent {
180180
component: &dyn PartialReflect,
181181
registry: &TypeRegistry,
182182
map: &mut dyn EntityMapper,
183-
relationship_insert_hook_mode: RelationshipInsertHookMode,
183+
relationship_hook_mode: RelationshipHookMode,
184184
) {
185-
(self.0.apply_or_insert_mapped)(
186-
entity,
187-
component,
188-
registry,
189-
map,
190-
relationship_insert_hook_mode,
191-
);
185+
(self.0.apply_or_insert_mapped)(entity, component, registry, map, relationship_hook_mode);
192186
}
193187

194188
/// Removes this [`Component`] type from the entity. Does nothing if it doesn't exist.
@@ -333,40 +327,33 @@ impl<C: Component + Reflect + TypePath> FromType<C> for ReflectComponent {
333327
let mut component = unsafe { entity.get_mut_assume_mutable::<C>() }.unwrap();
334328
component.apply(reflected_component);
335329
},
336-
apply_or_insert_mapped:
337-
|entity, reflected_component, registry, mapper, relationship_insert_hook_mode| {
338-
let map_fn = map_function(mapper);
339-
if C::Mutability::MUTABLE {
340-
// SAFETY: guard ensures `C` is a mutable component
341-
if let Some(mut component) = unsafe { entity.get_mut_assume_mutable::<C>() }
342-
{
343-
component.apply(reflected_component.as_partial_reflect());
344-
C::visit_entities_mut(&mut component, map_fn);
345-
} else {
346-
let mut component = entity.world_scope(|world| {
347-
from_reflect_with_fallback::<C>(
348-
reflected_component,
349-
world,
350-
registry,
351-
)
352-
});
353-
C::visit_entities_mut(&mut component, map_fn);
354-
entity.insert_with_relationship_insert_hook_mode(
355-
component,
356-
relationship_insert_hook_mode,
357-
);
358-
}
330+
apply_or_insert_mapped: |entity,
331+
reflected_component,
332+
registry,
333+
mapper,
334+
relationship_hook_mode| {
335+
let map_fn = map_function(mapper);
336+
if C::Mutability::MUTABLE {
337+
// SAFETY: guard ensures `C` is a mutable component
338+
if let Some(mut component) = unsafe { entity.get_mut_assume_mutable::<C>() } {
339+
component.apply(reflected_component.as_partial_reflect());
340+
C::visit_entities_mut(&mut component, map_fn);
359341
} else {
360342
let mut component = entity.world_scope(|world| {
361343
from_reflect_with_fallback::<C>(reflected_component, world, registry)
362344
});
363345
C::visit_entities_mut(&mut component, map_fn);
364-
entity.insert_with_relationship_insert_hook_mode(
365-
component,
366-
relationship_insert_hook_mode,
367-
);
346+
entity
347+
.insert_with_relationship_hook_mode(component, relationship_hook_mode);
368348
}
369-
},
349+
} else {
350+
let mut component = entity.world_scope(|world| {
351+
from_reflect_with_fallback::<C>(reflected_component, world, registry)
352+
});
353+
C::visit_entities_mut(&mut component, map_fn);
354+
entity.insert_with_relationship_hook_mode(component, relationship_hook_mode);
355+
}
356+
},
370357
remove: |entity| {
371358
entity.remove::<C>();
372359
},

0 commit comments

Comments
 (0)