Skip to content

Commit

Permalink
Move some structs that impl Command to methods on World and `Enti…
Browse files Browse the repository at this point in the history
…tyWorldMut` (bevyengine#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](bevyengine#8895 (comment))
and decided to stick with the trait, but we could revisit it here if so
desired.
  • Loading branch information
JaySpruce authored Dec 29, 2024
1 parent 291cb31 commit 0f2b2de
Show file tree
Hide file tree
Showing 6 changed files with 390 additions and 549 deletions.
194 changes: 178 additions & 16 deletions crates/bevy_ecs/src/observer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -2,11 +2,9 @@
mod entity_observer;
mod runner;
mod trigger_event;

pub use entity_observer::CloneEntityWithObserversExt;
pub use runner::*;
pub use trigger_event::*;

use crate::{
archetype::ArchetypeFlags,
Expand Down Expand Up @@ -168,6 +166,98 @@ impl<'w, E, B: Bundle> DerefMut for Trigger<'w, E, B> {
}
}

/// Represents a collection of targets for a specific [`Trigger`] of an [`Event`]. Targets can be of type [`Entity`] or [`ComponentId`].
///
/// When a trigger occurs for a given event and [`TriggerTargets`], any [`Observer`] that watches for that specific event-target combination
/// will run.
pub trait TriggerTargets {
/// The components the trigger should target.
fn components(&self) -> &[ComponentId];

/// The entities the trigger should target.
fn entities(&self) -> &[Entity];
}

impl TriggerTargets for () {
fn components(&self) -> &[ComponentId] {
&[]
}

fn entities(&self) -> &[Entity] {
&[]
}
}

impl TriggerTargets for Entity {
fn components(&self) -> &[ComponentId] {
&[]
}

fn entities(&self) -> &[Entity] {
core::slice::from_ref(self)
}
}

impl TriggerTargets for Vec<Entity> {
fn components(&self) -> &[ComponentId] {
&[]
}

fn entities(&self) -> &[Entity] {
self.as_slice()
}
}

impl<const N: usize> TriggerTargets for [Entity; N] {
fn components(&self) -> &[ComponentId] {
&[]
}

fn entities(&self) -> &[Entity] {
self.as_slice()
}
}

impl TriggerTargets for ComponentId {
fn components(&self) -> &[ComponentId] {
core::slice::from_ref(self)
}

fn entities(&self) -> &[Entity] {
&[]
}
}

impl TriggerTargets for Vec<ComponentId> {
fn components(&self) -> &[ComponentId] {
self.as_slice()
}

fn entities(&self) -> &[Entity] {
&[]
}
}

impl<const N: usize> TriggerTargets for [ComponentId; N] {
fn components(&self) -> &[ComponentId] {
self.as_slice()
}

fn entities(&self) -> &[Entity] {
&[]
}
}

impl TriggerTargets for &Vec<Entity> {
fn components(&self) -> &[ComponentId] {
&[]
}

fn entities(&self) -> &[Entity] {
self.as_slice()
}
}

/// A description of what an [`Observer`] observes.
#[derive(Default, Clone)]
pub struct ObserverDescriptor {
Expand Down Expand Up @@ -431,34 +521,106 @@ impl World {
/// While event types commonly implement [`Copy`],
/// those that don't will be consumed and will no longer be accessible.
/// If you need to use the event after triggering it, use [`World::trigger_ref`] instead.
pub fn trigger(&mut self, event: impl Event) {
TriggerEvent { event, targets: () }.trigger(self);
pub fn trigger<E: Event>(&mut self, mut event: E) {
let event_id = self.register_component::<E>();
// SAFETY: We just registered `event_id` with the type of `event`
unsafe { self.trigger_targets_dynamic_ref(event_id, &mut event, ()) };
}

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

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

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

/// Triggers the given [`Event`] for the given `targets`, which will run any [`Observer`]s watching for it.
///
/// While event types commonly implement [`Copy`],
/// those that don't will be consumed and will no longer be accessible.
/// If you need to use the event after triggering it, use [`World::trigger_targets_dynamic_ref`] instead.
///
/// # Safety
///
/// Caller must ensure that `event_data` is accessible as the type represented by `event_id`.
pub unsafe fn trigger_targets_dynamic<E: Event, Targets: TriggerTargets>(
&mut self,
event_id: ComponentId,
mut event_data: E,
targets: Targets,
) {
// SAFETY: `event_data` is accessible as the type represented by `event_id`
unsafe {
self.trigger_targets_dynamic_ref(event_id, &mut event_data, targets);
};
}

/// Triggers the given [`Event`] as a mutable reference for the given `targets`,
/// which will run any [`Observer`]s watching for it.
///
/// Compared to [`World::trigger_targets_dynamic`], this method is most useful when it's necessary to check
/// or use the event after it has been modified by observers.
///
/// # Safety
///
/// Caller must ensure that `event_data` is accessible as the type represented by `event_id`.
pub unsafe fn trigger_targets_dynamic_ref<E: Event, Targets: TriggerTargets>(
&mut self,
event_id: ComponentId,
event_data: &mut E,
targets: Targets,
) {
let mut world = DeferredWorld::from(self);
if targets.entities().is_empty() {
// SAFETY: `event_data` is accessible as the type represented by `event_id`
unsafe {
world.trigger_observers_with_data::<_, E::Traversal>(
event_id,
Entity::PLACEHOLDER,
targets.components(),
event_data,
false,
);
};
} else {
for target in targets.entities() {
// SAFETY: `event_data` is accessible as the type represented by `event_id`
unsafe {
world.trigger_observers_with_data::<_, E::Traversal>(
event_id,
*target,
targets.components(),
event_data,
E::AUTO_PROPAGATE,
);
};
}
}
}

/// Register an observer to the cache, called when an observer is created
Expand Down Expand Up @@ -590,7 +752,7 @@ mod tests {
use crate as bevy_ecs;
use crate::component::ComponentId;
use crate::{
observer::{EmitDynamicTrigger, Observer, ObserverDescriptor, ObserverState, OnReplace},
observer::{Observer, ObserverDescriptor, ObserverState, OnReplace},
prelude::*,
traversal::Traversal,
};
Expand Down Expand Up @@ -996,18 +1158,18 @@ mod tests {
let event_a = world.register_component::<EventA>();

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

world.commands().queue(
// SAFETY: we registered `event_a` above and it matches the type of TriggerA
unsafe { EmitDynamicTrigger::new_with_id(event_a, EventA, ()) },
);
world.commands().queue(move |world: &mut World| {
// SAFETY: we registered `event_a` above and it matches the type of EventA
unsafe { world.trigger_targets_dynamic(event_a, EventA, ()) };
});
world.flush();
assert_eq!(vec!["event_a"], world.resource::<Order>().0);
}
Expand Down
Loading

0 comments on commit 0f2b2de

Please sign in to comment.