Skip to content

Commit 0f2b2de

Browse files
authored
Move some structs that impl Command to methods on World and EntityWorldMut (#16999)
## Objective Commands were previously limited to structs that implemented `Command`. Now there are blanket implementations for closures, which (in my opinion) are generally preferable. Internal commands within `commands/mod.rs` have been switched from structs to closures, but there are a number of internal commands in other areas of the engine that still use structs. I'd like to tidy these up by moving their implementations to methods on `World`/`EntityWorldMut` and changing `Commands` to use those methods through closures. This PR handles the following: - `TriggerEvent` and `EmitDynamicTrigger` double as commands and helper structs, and can just be moved to `World` methods. - Four structs that enabled insertion/removal of components via reflection. This functionality shouldn't be exclusive to commands, and can be added to `EntityWorldMut`. - Five structs that mostly just wrapped `World` methods, and can be replaced with closures that do the same thing. ## Solution - __Observer Triggers__ (`observer/trigger_event.rs` and `observer/mod.rs`) - Moved the internals of `TriggerEvent` to the `World` methods that used it. - Replaced `EmitDynamicTrigger` with two `World` methods: - `trigger_targets_dynamic` - `trigger_targets_dynamic_ref` - `TriggerTargets` was now the only thing in `observer/trigger_event.rs`, so it's been moved to `observer/mod.rs` and `trigger_event.rs` was deleted. - __Reflection Insert/Remove__ (`reflect/entity_commands.rs`) - Replaced the following `Command` impls with equivalent methods on `EntityWorldMut`: - `InsertReflect` -> `insert_reflect` - `InsertReflectWithRegistry` -> `insert_reflect_with_registry` - `RemoveReflect` -> `remove_reflect` - `RemoveReflectWithRegistry` -> `remove_reflect_with_registry` - __System Registration__ (`system/system_registry.rs`) - The following `Command` impls just wrapped a `World` method and have been replaced with closures: - `RunSystemWith` - `UnregisterSystem` - `RunSystemCachedWith` - `UnregisterSystemCached` - `RegisterSystem` called a helper function that basically worked as a constructor for `RegisteredSystem` and made sure it came with a marker component. That helper function has been replaced with `RegisteredSystem::new` and a `#[require]`. ## Possible Addition The extension trait that adds the reflection commands, `ReflectCommandExt`, isn't strictly necessary; we could just `impl EntityCommands`. We could even move them to the same files as the main impls and put it behind a `#[cfg]`. The PR that added it [had a similar conversation](#8895 (comment)) and decided to stick with the trait, but we could revisit it here if so desired.
1 parent 291cb31 commit 0f2b2de

File tree

6 files changed

+390
-549
lines changed

6 files changed

+390
-549
lines changed

crates/bevy_ecs/src/observer/mod.rs

Lines changed: 178 additions & 16 deletions
Original file line numberDiff line numberDiff line change
@@ -2,11 +2,9 @@
22
33
mod entity_observer;
44
mod runner;
5-
mod trigger_event;
65

76
pub use entity_observer::CloneEntityWithObserversExt;
87
pub use runner::*;
9-
pub use trigger_event::*;
108

119
use crate::{
1210
archetype::ArchetypeFlags,
@@ -168,6 +166,98 @@ impl<'w, E, B: Bundle> DerefMut for Trigger<'w, E, B> {
168166
}
169167
}
170168

169+
/// Represents a collection of targets for a specific [`Trigger`] of an [`Event`]. Targets can be of type [`Entity`] or [`ComponentId`].
170+
///
171+
/// When a trigger occurs for a given event and [`TriggerTargets`], any [`Observer`] that watches for that specific event-target combination
172+
/// will run.
173+
pub trait TriggerTargets {
174+
/// The components the trigger should target.
175+
fn components(&self) -> &[ComponentId];
176+
177+
/// The entities the trigger should target.
178+
fn entities(&self) -> &[Entity];
179+
}
180+
181+
impl TriggerTargets for () {
182+
fn components(&self) -> &[ComponentId] {
183+
&[]
184+
}
185+
186+
fn entities(&self) -> &[Entity] {
187+
&[]
188+
}
189+
}
190+
191+
impl TriggerTargets for Entity {
192+
fn components(&self) -> &[ComponentId] {
193+
&[]
194+
}
195+
196+
fn entities(&self) -> &[Entity] {
197+
core::slice::from_ref(self)
198+
}
199+
}
200+
201+
impl TriggerTargets for Vec<Entity> {
202+
fn components(&self) -> &[ComponentId] {
203+
&[]
204+
}
205+
206+
fn entities(&self) -> &[Entity] {
207+
self.as_slice()
208+
}
209+
}
210+
211+
impl<const N: usize> TriggerTargets for [Entity; N] {
212+
fn components(&self) -> &[ComponentId] {
213+
&[]
214+
}
215+
216+
fn entities(&self) -> &[Entity] {
217+
self.as_slice()
218+
}
219+
}
220+
221+
impl TriggerTargets for ComponentId {
222+
fn components(&self) -> &[ComponentId] {
223+
core::slice::from_ref(self)
224+
}
225+
226+
fn entities(&self) -> &[Entity] {
227+
&[]
228+
}
229+
}
230+
231+
impl TriggerTargets for Vec<ComponentId> {
232+
fn components(&self) -> &[ComponentId] {
233+
self.as_slice()
234+
}
235+
236+
fn entities(&self) -> &[Entity] {
237+
&[]
238+
}
239+
}
240+
241+
impl<const N: usize> TriggerTargets for [ComponentId; N] {
242+
fn components(&self) -> &[ComponentId] {
243+
self.as_slice()
244+
}
245+
246+
fn entities(&self) -> &[Entity] {
247+
&[]
248+
}
249+
}
250+
251+
impl TriggerTargets for &Vec<Entity> {
252+
fn components(&self) -> &[ComponentId] {
253+
&[]
254+
}
255+
256+
fn entities(&self) -> &[Entity] {
257+
self.as_slice()
258+
}
259+
}
260+
171261
/// A description of what an [`Observer`] observes.
172262
#[derive(Default, Clone)]
173263
pub struct ObserverDescriptor {
@@ -431,34 +521,106 @@ impl World {
431521
/// While event types commonly implement [`Copy`],
432522
/// those that don't will be consumed and will no longer be accessible.
433523
/// If you need to use the event after triggering it, use [`World::trigger_ref`] instead.
434-
pub fn trigger(&mut self, event: impl Event) {
435-
TriggerEvent { event, targets: () }.trigger(self);
524+
pub fn trigger<E: Event>(&mut self, mut event: E) {
525+
let event_id = self.register_component::<E>();
526+
// SAFETY: We just registered `event_id` with the type of `event`
527+
unsafe { self.trigger_targets_dynamic_ref(event_id, &mut event, ()) };
436528
}
437529

438530
/// Triggers the given [`Event`] as a mutable reference, which will run any [`Observer`]s watching for it.
439531
///
440532
/// Compared to [`World::trigger`], this method is most useful when it's necessary to check
441533
/// or use the event after it has been modified by observers.
442-
pub fn trigger_ref(&mut self, event: &mut impl Event) {
443-
TriggerEvent { event, targets: () }.trigger_ref(self);
534+
pub fn trigger_ref<E: Event>(&mut self, event: &mut E) {
535+
let event_id = self.register_component::<E>();
536+
// SAFETY: We just registered `event_id` with the type of `event`
537+
unsafe { self.trigger_targets_dynamic_ref(event_id, event, ()) };
444538
}
445539

446540
/// Triggers the given [`Event`] for the given `targets`, which will run any [`Observer`]s watching for it.
447541
///
448542
/// While event types commonly implement [`Copy`],
449543
/// those that don't will be consumed and will no longer be accessible.
450544
/// If you need to use the event after triggering it, use [`World::trigger_targets_ref`] instead.
451-
pub fn trigger_targets(&mut self, event: impl Event, targets: impl TriggerTargets) {
452-
TriggerEvent { event, targets }.trigger(self);
545+
pub fn trigger_targets<E: Event>(&mut self, mut event: E, targets: impl TriggerTargets) {
546+
let event_id = self.register_component::<E>();
547+
// SAFETY: We just registered `event_id` with the type of `event`
548+
unsafe { self.trigger_targets_dynamic_ref(event_id, &mut event, targets) };
453549
}
454550

455551
/// Triggers the given [`Event`] as a mutable reference for the given `targets`,
456552
/// which will run any [`Observer`]s watching for it.
457553
///
458554
/// Compared to [`World::trigger_targets`], this method is most useful when it's necessary to check
459555
/// or use the event after it has been modified by observers.
460-
pub fn trigger_targets_ref(&mut self, event: &mut impl Event, targets: impl TriggerTargets) {
461-
TriggerEvent { event, targets }.trigger_ref(self);
556+
pub fn trigger_targets_ref<E: Event>(&mut self, event: &mut E, targets: impl TriggerTargets) {
557+
let event_id = self.register_component::<E>();
558+
// SAFETY: We just registered `event_id` with the type of `event`
559+
unsafe { self.trigger_targets_dynamic_ref(event_id, event, targets) };
560+
}
561+
562+
/// Triggers the given [`Event`] for the given `targets`, which will run any [`Observer`]s watching for it.
563+
///
564+
/// While event types commonly implement [`Copy`],
565+
/// those that don't will be consumed and will no longer be accessible.
566+
/// If you need to use the event after triggering it, use [`World::trigger_targets_dynamic_ref`] instead.
567+
///
568+
/// # Safety
569+
///
570+
/// Caller must ensure that `event_data` is accessible as the type represented by `event_id`.
571+
pub unsafe fn trigger_targets_dynamic<E: Event, Targets: TriggerTargets>(
572+
&mut self,
573+
event_id: ComponentId,
574+
mut event_data: E,
575+
targets: Targets,
576+
) {
577+
// SAFETY: `event_data` is accessible as the type represented by `event_id`
578+
unsafe {
579+
self.trigger_targets_dynamic_ref(event_id, &mut event_data, targets);
580+
};
581+
}
582+
583+
/// Triggers the given [`Event`] as a mutable reference for the given `targets`,
584+
/// which will run any [`Observer`]s watching for it.
585+
///
586+
/// Compared to [`World::trigger_targets_dynamic`], this method is most useful when it's necessary to check
587+
/// or use the event after it has been modified by observers.
588+
///
589+
/// # Safety
590+
///
591+
/// Caller must ensure that `event_data` is accessible as the type represented by `event_id`.
592+
pub unsafe fn trigger_targets_dynamic_ref<E: Event, Targets: TriggerTargets>(
593+
&mut self,
594+
event_id: ComponentId,
595+
event_data: &mut E,
596+
targets: Targets,
597+
) {
598+
let mut world = DeferredWorld::from(self);
599+
if targets.entities().is_empty() {
600+
// SAFETY: `event_data` is accessible as the type represented by `event_id`
601+
unsafe {
602+
world.trigger_observers_with_data::<_, E::Traversal>(
603+
event_id,
604+
Entity::PLACEHOLDER,
605+
targets.components(),
606+
event_data,
607+
false,
608+
);
609+
};
610+
} else {
611+
for target in targets.entities() {
612+
// SAFETY: `event_data` is accessible as the type represented by `event_id`
613+
unsafe {
614+
world.trigger_observers_with_data::<_, E::Traversal>(
615+
event_id,
616+
*target,
617+
targets.components(),
618+
event_data,
619+
E::AUTO_PROPAGATE,
620+
);
621+
};
622+
}
623+
}
462624
}
463625

464626
/// Register an observer to the cache, called when an observer is created
@@ -590,7 +752,7 @@ mod tests {
590752
use crate as bevy_ecs;
591753
use crate::component::ComponentId;
592754
use crate::{
593-
observer::{EmitDynamicTrigger, Observer, ObserverDescriptor, ObserverState, OnReplace},
755+
observer::{Observer, ObserverDescriptor, ObserverState, OnReplace},
594756
prelude::*,
595757
traversal::Traversal,
596758
};
@@ -996,18 +1158,18 @@ mod tests {
9961158
let event_a = world.register_component::<EventA>();
9971159

9981160
world.spawn(ObserverState {
999-
// SAFETY: we registered `event_a` above and it matches the type of TriggerA
1161+
// SAFETY: we registered `event_a` above and it matches the type of EventA
10001162
descriptor: unsafe { ObserverDescriptor::default().with_events(vec![event_a]) },
10011163
runner: |mut world, _trigger, _ptr, _propagate| {
10021164
world.resource_mut::<Order>().observed("event_a");
10031165
},
10041166
..Default::default()
10051167
});
10061168

1007-
world.commands().queue(
1008-
// SAFETY: we registered `event_a` above and it matches the type of TriggerA
1009-
unsafe { EmitDynamicTrigger::new_with_id(event_a, EventA, ()) },
1010-
);
1169+
world.commands().queue(move |world: &mut World| {
1170+
// SAFETY: we registered `event_a` above and it matches the type of EventA
1171+
unsafe { world.trigger_targets_dynamic(event_a, EventA, ()) };
1172+
});
10111173
world.flush();
10121174
assert_eq!(vec!["event_a"], world.resource::<Order>().0);
10131175
}

0 commit comments

Comments
 (0)