Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Minimal Bubbling Observers #13991

Merged
merged 21 commits into from
Jul 15, 2024
Merged
Show file tree
Hide file tree
Changes from 15 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 12 additions & 1 deletion Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ keywords = ["game", "engine", "gamedev", "graphics", "bevy"]
license = "MIT OR Apache-2.0"
repository = "https://github.com/bevyengine/bevy"
documentation = "https://docs.rs/bevy"
rust-version = "1.78.0"
rust-version = "1.79.0"

[workspace]
exclude = [
Expand Down Expand Up @@ -2549,6 +2549,17 @@ description = "Demonstrates observers that react to events (both built-in life-c
category = "ECS (Entity Component System)"
wasm = true

[[example]]
name = "observer_propagation"
path = "examples/ecs/observer_propagation.rs"
doc-scrape-examples = true

[package.metadata.example.observer_propagation]
name = "Observer Propagation"
description = "Demonstrates event propagation with observers"
category = "ECS (Entity Component System)"
wasm = true

[[example]]
name = "3d_rotation"
path = "examples/transforms/3d_rotation.rs"
Expand Down
2 changes: 2 additions & 0 deletions crates/bevy_ecs/macros/src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,6 +17,8 @@ pub fn derive_event(input: TokenStream) -> TokenStream {

TokenStream::from(quote! {
impl #impl_generics #bevy_ecs_path::event::Event for #struct_name #type_generics #where_clause {
type Traversal = #bevy_ecs_path::traversal::TraverseNone;
const AUTO_PROPAGATE: bool = false;
}

impl #impl_generics #bevy_ecs_path::component::Component for #struct_name #type_generics #where_clause {
Expand Down
16 changes: 14 additions & 2 deletions crates/bevy_ecs/src/event/base.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use crate::component::Component;
use crate::{component::Component, traversal::Traversal};
#[cfg(feature = "bevy_reflect")]
use bevy_reflect::Reflect;
use std::{
Expand Down Expand Up @@ -31,7 +31,19 @@ use std::{
label = "invalid `Event`",
note = "consider annotating `{Self}` with `#[derive(Event)]`"
)]
pub trait Event: Component {}
pub trait Event: Component {
/// The component that describes which Entity to propagate this event to next, when [propagation] is enabled.
///
/// [propagation]: crate::observer::Trigger::propagate
type Traversal: Traversal;

/// When true, this event will always attempt to propagate when [triggered], without requiring a call
/// to [`Trigger::propagate`].
///
/// [triggered]: crate::system::Commands::trigger_targets
/// [`Trigger::propagate`]: crate::observer::Trigger::propagate
const AUTO_PROPAGATE: bool = false;
}

/// An `EventId` uniquely identifies an event stored in a specific [`World`].
///
Expand Down
1 change: 1 addition & 0 deletions crates/bevy_ecs/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ pub mod removal_detection;
pub mod schedule;
pub mod storage;
pub mod system;
pub mod traversal;
pub mod world;

pub use bevy_ptr as ptr;
Expand Down
254 changes: 250 additions & 4 deletions crates/bevy_ecs/src/observer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,18 +15,21 @@ use bevy_utils::{EntityHashMap, HashMap};
use std::marker::PhantomData;

/// Type containing triggered [`Event`] information for a given run of an [`Observer`]. This contains the
/// [`Event`] data itself. If it was triggered for a specific [`Entity`], it includes that as well.
/// [`Event`] data itself. If it was triggered for a specific [`Entity`], it includes that as well. It also
/// contains event propagation information. See [`Trigger::propagate`] for more information.
pub struct Trigger<'w, E, B: Bundle = ()> {
event: &'w mut E,
propagate: &'w mut bool,
trigger: ObserverTrigger,
_marker: PhantomData<B>,
}

impl<'w, E, B: Bundle> Trigger<'w, E, B> {
/// Creates a new trigger for the given event and observer information.
pub fn new(event: &'w mut E, trigger: ObserverTrigger) -> Self {
pub fn new(event: &'w mut E, propagate: &'w mut bool, trigger: ObserverTrigger) -> Self {
Self {
event,
propagate,
trigger,
_marker: PhantomData,
}
Expand Down Expand Up @@ -56,6 +59,27 @@ impl<'w, E, B: Bundle> Trigger<'w, E, B> {
pub fn entity(&self) -> Entity {
self.trigger.entity
}

/// Enables or disables event propagation, allowing the same event to trigger observers on a chain of different entities.
///
/// The path an event will propagate along is specified by its associated [`Traversal`] component. By default, events
/// use `TraverseNone` which ends the path immediately and prevents propagation.
///
/// To enable propagation, you must:
/// + Set [`Event::Traversal`] to the component you want to propagate along.
/// + Either call `propagate(true)` in the first observer or set [`Event::AUTO_PROPAGATE`] to `true`.
///
/// You can prevent an event from propagating further using `propagate(false)`.
///
/// [`Traversal`]: crate::traversal::Traversal
pub fn propagate(&mut self, should_propagate: bool) {
*self.propagate = should_propagate;
}

/// Returns the value of the flag that controls event propagation. See [`propagate`] for more information.
NthTensor marked this conversation as resolved.
Show resolved Hide resolved
pub fn get_propagate(&self) -> bool {
*self.propagate
}
}

/// A description of what an [`Observer`] observes.
Expand Down Expand Up @@ -174,6 +198,7 @@ impl Observers {
entity: Entity,
components: impl Iterator<Item = ComponentId>,
data: &mut T,
propagate: &mut bool,
) {
// SAFETY: You cannot get a mutable reference to `observers` from `DeferredWorld`
let (mut world, observers) = unsafe {
Expand All @@ -197,9 +222,9 @@ impl Observers {
entity,
},
data.into(),
propagate,
);
};

// Trigger observers listening for any kind of this trigger
observers.map.iter().for_each(&mut trigger_observer);

Expand Down Expand Up @@ -393,6 +418,7 @@ mod tests {
use crate as bevy_ecs;
use crate::observer::{EmitDynamicTrigger, Observer, ObserverDescriptor, ObserverState};
use crate::prelude::*;
use crate::traversal::Traversal;

#[derive(Component)]
struct A;
Expand Down Expand Up @@ -421,6 +447,24 @@ mod tests {
}
}

#[derive(Component)]
struct Parent(Entity);

impl Traversal for Parent {
fn next(&self) -> Option<Entity> {
Some(self.0)
}
}

#[derive(Component)]
struct EventPropagating;

impl Event for EventPropagating {
type Traversal = Parent;

const AUTO_PROPAGATE: bool = true;
}

#[test]
fn observer_order_spawn_despawn() {
let mut world = World::new();
Expand Down Expand Up @@ -649,7 +693,7 @@ mod tests {
world.spawn(ObserverState {
// SAFETY: we registered `event_a` above and it matches the type of TriggerA
descriptor: unsafe { ObserverDescriptor::default().with_events(vec![event_a]) },
runner: |mut world, _trigger, _ptr| {
runner: |mut world, _trigger, _ptr, _propagate| {
world.resource_mut::<R>().0 += 1;
},
..Default::default()
Expand All @@ -662,4 +706,206 @@ mod tests {
world.flush();
assert_eq!(1, world.resource::<R>().0);
}

#[test]
fn observer_propagating() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pedantic: I'm seeing many tests like this, not just for bubbling but observer in general. Tests using counters to determine if multiple observers ran or if a subset of observers ran. This isn't the best testing because the tests can pass when they shouldn't if an observer runs multiple times or if the wrong subset of observers run.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

simple fix is a Vec<str> where we can push observer "labels" when they execute and then verify with vec!["obs1", "obs2"]
it's only tests so it doesn't need to be "proper"

would that be alright?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That seems correct to me. I'd rather not make this change as part of this PR. Changing the existing tests is out of scope, and I'd like to keep consistency. But I agree these tests could be improved as follow-up work.

let mut world = World::new();
world.init_resource::<R>();

let parent = world
.spawn_empty()
.observe(|_: Trigger<EventPropagating>, mut res: ResMut<R>| res.0 += 1)
.id();

let child = world
.spawn(Parent(parent))
.observe(|_: Trigger<EventPropagating>, mut res: ResMut<R>| res.0 += 1)
.id();

// TODO: ideally this flush is not necessary, but right now observe() returns WorldEntityMut
// and therefore does not automatically flush.
world.flush();
world.trigger_targets(EventPropagating, child);
world.flush();
assert_eq!(2, world.resource::<R>().0);
}

#[test]
fn observer_propagating_redundant_dispatch() {
let mut world = World::new();
world.init_resource::<R>();

let parent = world
.spawn_empty()
.observe(|_: Trigger<EventPropagating>, mut res: ResMut<R>| res.0 += 1)
.id();

let child = world
.spawn(Parent(parent))
.observe(|_: Trigger<EventPropagating>, mut res: ResMut<R>| res.0 += 1)
.id();

// TODO: ideally this flush is not necessary, but right now observe() returns WorldEntityMut
// and therefore does not automatically flush.
world.flush();
world.trigger_targets(EventPropagating, [child, parent]);
NthTensor marked this conversation as resolved.
Show resolved Hide resolved
world.flush();
assert_eq!(3, world.resource::<R>().0);
}

#[test]
fn observer_propagating_halt() {
let mut world = World::new();
world.init_resource::<R>();

let parent = world
.spawn_empty()
.observe(|_: Trigger<EventPropagating>, mut res: ResMut<R>| res.0 += 1)
.id();

let child = world
.spawn(Parent(parent))
.observe(|trigger: Trigger<EventPropagating>, mut res: ResMut<R>| {
res.0 += 1;
*trigger.propagate = false;
NthTensor marked this conversation as resolved.
Show resolved Hide resolved
})
.id();

// TODO: ideally this flush is not necessary, but right now observe() returns WorldEntityMut
// and therefore does not automatically flush.
world.flush();
world.trigger_targets(EventPropagating, child);
world.flush();
assert_eq!(1, world.resource::<R>().0);
}

#[test]
fn observer_propagating_join() {
let mut world = World::new();
world.init_resource::<R>();

let parent = world
.spawn_empty()
.observe(|_: Trigger<EventPropagating>, mut res: ResMut<R>| res.0 += 1)
.id();

let child_a = world
.spawn(Parent(parent))
.observe(|_: Trigger<EventPropagating>, mut res: ResMut<R>| {
res.0 += 1;
})
.id();

let child_b = world
.spawn(Parent(parent))
.observe(|_: Trigger<EventPropagating>, mut res: ResMut<R>| {
res.0 += 1;
})
.id();

// TODO: ideally this flush is not necessary, but right now observe() returns WorldEntityMut
// and therefore does not automatically flush.
world.flush();
world.trigger_targets(EventPropagating, [child_a, child_b]);
world.flush();
assert_eq!(4, world.resource::<R>().0);
}

#[test]
fn observer_propagating_no_next() {
let mut world = World::new();
world.init_resource::<R>();

let entity = world
.spawn_empty()
.observe(|_: Trigger<EventPropagating>, mut res: ResMut<R>| res.0 += 1)
.id();

// TODO: ideally this flush is not necessary, but right now observe() returns WorldEntityMut
// and therefore does not automatically flush.
world.flush();
world.trigger_targets(EventPropagating, entity);
world.flush();
assert_eq!(1, world.resource::<R>().0);
}

#[test]
fn observer_propagating_parallel_propagation() {
let mut world = World::new();
world.init_resource::<R>();

let parent_a = world
.spawn_empty()
.observe(|_: Trigger<EventPropagating>, mut res: ResMut<R>| res.0 += 1)
.id();

let child_a = world
.spawn(Parent(parent_a))
.observe(|trigger: Trigger<EventPropagating>, mut res: ResMut<R>| {
res.0 += 1;
*trigger.propagate = false;
})
.id();

let parent_b = world
.spawn_empty()
.observe(|_: Trigger<EventPropagating>, mut res: ResMut<R>| res.0 += 1)
.id();

let child_b = world
.spawn(Parent(parent_b))
.observe(|_: Trigger<EventPropagating>, mut res: ResMut<R>| res.0 += 1)
.id();

// TODO: ideally this flush is not necessary, but right now observe() returns WorldEntityMut
// and therefore does not automatically flush.
world.flush();
world.trigger_targets(EventPropagating, [child_a, child_b]);
world.flush();
assert_eq!(3, world.resource::<R>().0);
}

#[test]
fn observer_propagating_world() {
let mut world = World::new();
world.init_resource::<R>();

world.observe(|_: Trigger<EventPropagating>, mut res: ResMut<R>| res.0 += 1);

let grandparent = world.spawn_empty().id();
let parent = world.spawn(Parent(grandparent)).id();
let child = world.spawn(Parent(parent)).id();

// TODO: ideally this flush is not necessary, but right now observe() returns WorldEntityMut
// and therefore does not automatically flush.
world.flush();
world.trigger_targets(EventPropagating, child);
world.flush();
assert_eq!(3, world.resource::<R>().0);
}

#[test]
fn observer_propagating_world_skipping() {
let mut world = World::new();
world.init_resource::<R>();

world.observe(
|trigger: Trigger<EventPropagating>, query: Query<&A>, mut res: ResMut<R>| {
if query.get(trigger.entity()).is_ok() {
res.0 += 1;
}
},
);

let grandparent = world.spawn(A).id();
let parent = world.spawn(Parent(grandparent)).id();
let child = world.spawn((A, Parent(parent))).id();

// TODO: ideally this flush is not necessary, but right now observe() returns WorldEntityMut
// and therefore does not automatically flush.
world.flush();
world.trigger_targets(EventPropagating, child);
world.flush();
assert_eq!(2, world.resource::<R>().0);
}
}
Loading
Loading