Skip to content

Commit 064e5e4

Browse files
ElliottjPierceElliottjPierce
authored
Remove entity placeholder from observers (#19440)
# Objective `Entity::PLACEHOLDER` acts as a magic number that will *probably* never really exist, but it certainly could. And, `Entity` has a niche, so the only reason to use `PLACEHOLDER` is as an alternative to `MaybeUninit` that trades safety risks for logic risks. As a result, bevy has generally advised against using `PLACEHOLDER`, but we still use if for a lot internally. This pr starts removing internal uses of it, starting from observers. ## Solution Change all trigger target related types from `Entity` to `Option<Entity>` Small migration guide to come. ## Testing CI ## Future Work This turned a lot of code from ```rust trigger.target() ``` to ```rust trigger.target().unwrap() ``` The extra panic is no worse than before; it's just earlier than panicking after passing the placeholder to something else. But this is kinda annoying. I would like to add a `TriggerMode` or something to `Event` that would restrict what kinds of targets can be used for that event. Many events like `Removed` etc, are always triggered with a target. We can make those have a way to assume Some, etc. But I wanted to save that for a future pr.
1 parent 860ff78 commit 064e5e4

File tree

30 files changed

+134
-99
lines changed

30 files changed

+134
-99
lines changed

‎crates/bevy_ecs/README.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -340,8 +340,8 @@ let mut world = World::new();
340340
let entity = world.spawn_empty().id();
341341

342342
world.add_observer(|trigger: Trigger<Explode>, mut commands: Commands| {
343-
println!("Entity {} goes BOOM!", trigger.target());
344-
commands.entity(trigger.target()).despawn();
343+
println!("Entity {} goes BOOM!", trigger.target().unwrap());
344+
commands.entity(trigger.target().unwrap()).despawn();
345345
});
346346

347347
world.flush();

‎crates/bevy_ecs/src/bundle.rs

Lines changed: 8 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -1143,7 +1143,7 @@ impl<'w> BundleInserter<'w> {
11431143
if archetype.has_replace_observer() {
11441144
deferred_world.trigger_observers(
11451145
ON_REPLACE,
1146-
entity,
1146+
Some(entity),
11471147
archetype_after_insert.iter_existing(),
11481148
caller,
11491149
);
@@ -1328,7 +1328,7 @@ impl<'w> BundleInserter<'w> {
13281328
if new_archetype.has_add_observer() {
13291329
deferred_world.trigger_observers(
13301330
ON_ADD,
1331-
entity,
1331+
Some(entity),
13321332
archetype_after_insert.iter_added(),
13331333
caller,
13341334
);
@@ -1346,7 +1346,7 @@ impl<'w> BundleInserter<'w> {
13461346
if new_archetype.has_insert_observer() {
13471347
deferred_world.trigger_observers(
13481348
ON_INSERT,
1349-
entity,
1349+
Some(entity),
13501350
archetype_after_insert.iter_inserted(),
13511351
caller,
13521352
);
@@ -1365,7 +1365,7 @@ impl<'w> BundleInserter<'w> {
13651365
if new_archetype.has_insert_observer() {
13661366
deferred_world.trigger_observers(
13671367
ON_INSERT,
1368-
entity,
1368+
Some(entity),
13691369
archetype_after_insert.iter_added(),
13701370
caller,
13711371
);
@@ -1519,7 +1519,7 @@ impl<'w> BundleRemover<'w> {
15191519
if self.old_archetype.as_ref().has_replace_observer() {
15201520
deferred_world.trigger_observers(
15211521
ON_REPLACE,
1522-
entity,
1522+
Some(entity),
15231523
bundle_components_in_archetype(),
15241524
caller,
15251525
);
@@ -1534,7 +1534,7 @@ impl<'w> BundleRemover<'w> {
15341534
if self.old_archetype.as_ref().has_remove_observer() {
15351535
deferred_world.trigger_observers(
15361536
ON_REMOVE,
1537-
entity,
1537+
Some(entity),
15381538
bundle_components_in_archetype(),
15391539
caller,
15401540
);
@@ -1785,7 +1785,7 @@ impl<'w> BundleSpawner<'w> {
17851785
if archetype.has_add_observer() {
17861786
deferred_world.trigger_observers(
17871787
ON_ADD,
1788-
entity,
1788+
Some(entity),
17891789
bundle_info.iter_contributed_components(),
17901790
caller,
17911791
);
@@ -1800,7 +1800,7 @@ impl<'w> BundleSpawner<'w> {
18001800
if archetype.has_insert_observer() {
18011801
deferred_world.trigger_observers(
18021802
ON_INSERT,
1803-
entity,
1803+
Some(entity),
18041804
bundle_info.iter_contributed_components(),
18051805
caller,
18061806
);

‎crates/bevy_ecs/src/observer/mod.rs

Lines changed: 17 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -68,7 +68,7 @@ impl<'w, E, B: Bundle> Trigger<'w, E, B> {
6868
}
6969

7070
/// Returns the [`Entity`] that was targeted by the `event` that triggered this observer. It may
71-
/// be [`Entity::PLACEHOLDER`].
71+
/// be [`None`] if the trigger is not for a particular entity.
7272
///
7373
/// Observable events can target specific entities. When those events fire, they will trigger
7474
/// any observers on the targeted entities. In this case, the `target()` and `observer()` are
@@ -81,7 +81,7 @@ impl<'w, E, B: Bundle> Trigger<'w, E, B> {
8181
///
8282
/// This is an important distinction: the entity reacting to an event is not always the same as
8383
/// the entity triggered by the event.
84-
pub fn target(&self) -> Entity {
84+
pub fn target(&self) -> Option<Entity> {
8585
self.trigger.target
8686
}
8787

@@ -341,7 +341,7 @@ pub struct ObserverTrigger {
341341
/// The [`ComponentId`]s the trigger targeted.
342342
components: SmallVec<[ComponentId; 2]>,
343343
/// The entity the trigger targeted.
344-
pub target: Entity,
344+
pub target: Option<Entity>,
345345
/// The location of the source code that triggered the observer.
346346
pub caller: MaybeLocation,
347347
}
@@ -416,7 +416,7 @@ impl Observers {
416416
pub(crate) fn invoke<T>(
417417
mut world: DeferredWorld,
418418
event_type: ComponentId,
419-
target: Entity,
419+
target: Option<Entity>,
420420
components: impl Iterator<Item = ComponentId> + Clone,
421421
data: &mut T,
422422
propagate: &mut bool,
@@ -455,8 +455,8 @@ impl Observers {
455455
observers.map.iter().for_each(&mut trigger_observer);
456456

457457
// Trigger entity observers listening for this kind of trigger
458-
if target != Entity::PLACEHOLDER {
459-
if let Some(map) = observers.entity_observers.get(&target) {
458+
if let Some(target_entity) = target {
459+
if let Some(map) = observers.entity_observers.get(&target_entity) {
460460
map.iter().for_each(&mut trigger_observer);
461461
}
462462
}
@@ -469,8 +469,8 @@ impl Observers {
469469
.iter()
470470
.for_each(&mut trigger_observer);
471471

472-
if target != Entity::PLACEHOLDER {
473-
if let Some(map) = component_observers.entity_map.get(&target) {
472+
if let Some(target_entity) = target {
473+
if let Some(map) = component_observers.entity_map.get(&target_entity) {
474474
map.iter().for_each(&mut trigger_observer);
475475
}
476476
}
@@ -695,7 +695,7 @@ impl World {
695695
unsafe {
696696
world.trigger_observers_with_data::<_, E::Traversal>(
697697
event_id,
698-
Entity::PLACEHOLDER,
698+
None,
699699
targets.components(),
700700
event_data,
701701
false,
@@ -708,7 +708,7 @@ impl World {
708708
unsafe {
709709
world.trigger_observers_with_data::<_, E::Traversal>(
710710
event_id,
711-
target_entity,
711+
Some(target_entity),
712712
targets.components(),
713713
event_data,
714714
E::AUTO_PROPAGATE,
@@ -999,20 +999,20 @@ mod tests {
999999
world.add_observer(
10001000
|obs: Trigger<OnAdd, A>, mut res: ResMut<Order>, mut commands: Commands| {
10011001
res.observed("add_a");
1002-
commands.entity(obs.target()).insert(B);
1002+
commands.entity(obs.target().unwrap()).insert(B);
10031003
},
10041004
);
10051005
world.add_observer(
10061006
|obs: Trigger<OnRemove, A>, mut res: ResMut<Order>, mut commands: Commands| {
10071007
res.observed("remove_a");
1008-
commands.entity(obs.target()).remove::<B>();
1008+
commands.entity(obs.target().unwrap()).remove::<B>();
10091009
},
10101010
);
10111011

10121012
world.add_observer(
10131013
|obs: Trigger<OnAdd, B>, mut res: ResMut<Order>, mut commands: Commands| {
10141014
res.observed("add_b");
1015-
commands.entity(obs.target()).remove::<A>();
1015+
commands.entity(obs.target().unwrap()).remove::<A>();
10161016
},
10171017
);
10181018
world.add_observer(|_: Trigger<OnRemove, B>, mut res: ResMut<Order>| {
@@ -1181,7 +1181,7 @@ mod tests {
11811181
};
11821182
world.spawn_empty().observe(system);
11831183
world.add_observer(move |obs: Trigger<EventA>, mut res: ResMut<Order>| {
1184-
assert_eq!(obs.target(), Entity::PLACEHOLDER);
1184+
assert_eq!(obs.target(), None);
11851185
res.observed("event_a");
11861186
});
11871187

@@ -1208,7 +1208,7 @@ mod tests {
12081208
.observe(|_: Trigger<EventA>, mut res: ResMut<Order>| res.observed("a_1"))
12091209
.id();
12101210
world.add_observer(move |obs: Trigger<EventA>, mut res: ResMut<Order>| {
1211-
assert_eq!(obs.target(), entity);
1211+
assert_eq!(obs.target().unwrap(), entity);
12121212
res.observed("a_2");
12131213
});
12141214

@@ -1628,7 +1628,7 @@ mod tests {
16281628

16291629
world.add_observer(
16301630
|trigger: Trigger<EventPropagating>, query: Query<&A>, mut res: ResMut<Order>| {
1631-
if query.get(trigger.target()).is_ok() {
1631+
if query.get(trigger.target().unwrap()).is_ok() {
16321632
res.observed("event");
16331633
}
16341634
},
@@ -1651,7 +1651,7 @@ mod tests {
16511651
fn observer_modifies_relationship() {
16521652
fn on_add(trigger: Trigger<OnAdd, A>, mut commands: Commands) {
16531653
commands
1654-
.entity(trigger.target())
1654+
.entity(trigger.target().unwrap())
16551655
.with_related_entities::<crate::hierarchy::ChildOf>(|rsc| {
16561656
rsc.spawn_empty();
16571657
});

‎crates/bevy_ecs/src/observer/runner.rs

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -123,8 +123,8 @@ pub type ObserverRunner = fn(DeferredWorld, ObserverTrigger, PtrMut, propagate:
123123
/// struct Explode;
124124
///
125125
/// world.add_observer(|trigger: Trigger<Explode>, mut commands: Commands| {
126-
/// println!("Entity {} goes BOOM!", trigger.target());
127-
/// commands.entity(trigger.target()).despawn();
126+
/// println!("Entity {} goes BOOM!", trigger.target().unwrap());
127+
/// commands.entity(trigger.target().unwrap()).despawn();
128128
/// });
129129
///
130130
/// world.flush();
@@ -157,7 +157,7 @@ pub type ObserverRunner = fn(DeferredWorld, ObserverTrigger, PtrMut, propagate:
157157
/// # struct Explode;
158158
/// world.entity_mut(e1).observe(|trigger: Trigger<Explode>, mut commands: Commands| {
159159
/// println!("Boom!");
160-
/// commands.entity(trigger.target()).despawn();
160+
/// commands.entity(trigger.target().unwrap()).despawn();
161161
/// });
162162
///
163163
/// world.entity_mut(e2).observe(|trigger: Trigger<Explode>, mut commands: Commands| {

‎crates/bevy_ecs/src/world/deferred_world.rs

Lines changed: 26 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@ use super::{unsafe_world_cell::UnsafeWorldCell, Mut, World, ON_INSERT, ON_REPLAC
2323
///
2424
/// This means that in order to add entities, for example, you will need to use commands instead of the world directly.
2525
pub struct DeferredWorld<'w> {
26-
// SAFETY: Implementors must not use this reference to make structural changes
26+
// SAFETY: Implementers must not use this reference to make structural changes
2727
world: UnsafeWorldCell<'w>,
2828
}
2929

@@ -157,7 +157,7 @@ impl<'w> DeferredWorld<'w> {
157157
if archetype.has_replace_observer() {
158158
self.trigger_observers(
159159
ON_REPLACE,
160-
entity,
160+
Some(entity),
161161
[component_id].into_iter(),
162162
MaybeLocation::caller(),
163163
);
@@ -197,7 +197,7 @@ impl<'w> DeferredWorld<'w> {
197197
if archetype.has_insert_observer() {
198198
self.trigger_observers(
199199
ON_INSERT,
200-
entity,
200+
Some(entity),
201201
[component_id].into_iter(),
202202
MaybeLocation::caller(),
203203
);
@@ -738,7 +738,7 @@ impl<'w> DeferredWorld<'w> {
738738
pub(crate) unsafe fn trigger_observers(
739739
&mut self,
740740
event: ComponentId,
741-
target: Entity,
741+
target: Option<Entity>,
742742
components: impl Iterator<Item = ComponentId> + Clone,
743743
caller: MaybeLocation,
744744
) {
@@ -761,26 +761,28 @@ impl<'w> DeferredWorld<'w> {
761761
pub(crate) unsafe fn trigger_observers_with_data<E, T>(
762762
&mut self,
763763
event: ComponentId,
764-
mut target: Entity,
764+
target: Option<Entity>,
765765
components: impl Iterator<Item = ComponentId> + Clone,
766766
data: &mut E,
767767
mut propagate: bool,
768768
caller: MaybeLocation,
769769
) where
770770
T: Traversal<E>,
771771
{
772+
Observers::invoke::<_>(
773+
self.reborrow(),
774+
event,
775+
target,
776+
components.clone(),
777+
data,
778+
&mut propagate,
779+
caller,
780+
);
781+
let Some(mut target) = target else { return };
782+
772783
loop {
773-
Observers::invoke::<_>(
774-
self.reborrow(),
775-
event,
776-
target,
777-
components.clone(),
778-
data,
779-
&mut propagate,
780-
caller,
781-
);
782784
if !propagate {
783-
break;
785+
return;
784786
}
785787
if let Some(traverse_to) = self
786788
.get_entity(target)
@@ -792,6 +794,15 @@ impl<'w> DeferredWorld<'w> {
792794
} else {
793795
break;
794796
}
797+
Observers::invoke::<_>(
798+
self.reborrow(),
799+
event,
800+
Some(target),
801+
components.clone(),
802+
data,
803+
&mut propagate,
804+
caller,
805+
);
795806
}
796807
}
797808

‎crates/bevy_ecs/src/world/entity_ref.rs

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -2371,7 +2371,7 @@ impl<'w> EntityWorldMut<'w> {
23712371
if archetype.has_despawn_observer() {
23722372
deferred_world.trigger_observers(
23732373
ON_DESPAWN,
2374-
self.entity,
2374+
Some(self.entity),
23752375
archetype.components(),
23762376
caller,
23772377
);
@@ -2385,7 +2385,7 @@ impl<'w> EntityWorldMut<'w> {
23852385
if archetype.has_replace_observer() {
23862386
deferred_world.trigger_observers(
23872387
ON_REPLACE,
2388-
self.entity,
2388+
Some(self.entity),
23892389
archetype.components(),
23902390
caller,
23912391
);
@@ -2400,7 +2400,7 @@ impl<'w> EntityWorldMut<'w> {
24002400
if archetype.has_remove_observer() {
24012401
deferred_world.trigger_observers(
24022402
ON_REMOVE,
2403-
self.entity,
2403+
Some(self.entity),
24042404
archetype.components(),
24052405
caller,
24062406
);
@@ -5749,7 +5749,9 @@ mod tests {
57495749
let entity = world
57505750
.spawn_empty()
57515751
.observe(|trigger: Trigger<TestEvent>, mut commands: Commands| {
5752-
commands.entity(trigger.target()).insert(TestComponent(0));
5752+
commands
5753+
.entity(trigger.target().unwrap())
5754+
.insert(TestComponent(0));
57535755
})
57545756
.id();
57555757

@@ -5769,7 +5771,7 @@ mod tests {
57695771
let mut world = World::new();
57705772
world.add_observer(
57715773
|trigger: Trigger<OnAdd, TestComponent>, mut commands: Commands| {
5772-
commands.entity(trigger.target()).despawn();
5774+
commands.entity(trigger.target().unwrap()).despawn();
57735775
},
57745776
);
57755777
let entity = world.spawn_empty().id();

‎crates/bevy_input_focus/src/lib.rs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -394,7 +394,7 @@ mod tests {
394394
trigger: Trigger<FocusedInput<KeyboardInput>>,
395395
mut query: Query<&mut GatherKeyboardEvents>,
396396
) {
397-
if let Ok(mut gather) = query.get_mut(trigger.target()) {
397+
if let Ok(mut gather) = query.get_mut(trigger.target().unwrap()) {
398398
if let Key::Character(c) = &trigger.input.logical_key {
399399
gather.0.push_str(c.as_str());
400400
}

0 commit comments

Comments
 (0)