Skip to content

Commit

Permalink
Bubbling observers traversal should use query data (#15385)
Browse files Browse the repository at this point in the history
# Objective

Fixes #14331

## Solution

- Make `Traversal` a subtrait of `ReadOnlyQueryData`
- Update implementations and usages

## Testing

- Updated unit tests

## Migration Guide

Update implementations of `Traversal`.

---------

Co-authored-by: Christian Hughes <[email protected]>
  • Loading branch information
BenjaminBrienen and ItsDoot authored Sep 23, 2024
1 parent 83356b1 commit 27bea6a
Show file tree
Hide file tree
Showing 12 changed files with 37 additions and 49 deletions.
2 changes: 1 addition & 1 deletion benches/benches/bevy_ecs/events/iter.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@ impl<const SIZE: usize> Benchmark<SIZE> {
}

pub fn run(&mut self) {
let mut reader = self.0.get_reader();
let mut reader = self.0.get_cursor();
for evt in reader.read(&self.0) {
std::hint::black_box(evt);
}
Expand Down
10 changes: 5 additions & 5 deletions benches/benches/bevy_ecs/observers/propagation.rs
Original file line number Diff line number Diff line change
@@ -1,14 +1,14 @@
use bevy_ecs::{
component::Component,
entity::Entity,
event::{Event, EventWriter},
event::Event,
observer::Trigger,
world::World,
};
use bevy_hierarchy::{BuildChildren, Children, Parent};
use bevy_hierarchy::{BuildChildren, Parent};

use criterion::{black_box, criterion_group, criterion_main, Criterion};
use rand::{prelude::SliceRandom, SeedableRng};
use criterion::{black_box, Criterion};
use rand::SeedableRng;
use rand::{seq::IteratorRandom, Rng};
use rand_chacha::ChaCha8Rng;

Expand Down Expand Up @@ -71,7 +71,7 @@ pub fn event_propagation(criterion: &mut Criterion) {
struct TestEvent<const N: usize> {}

impl<const N: usize> Event for TestEvent<N> {
type Traversal = Parent;
type Traversal = &'static Parent;
const AUTO_PROPAGATE: bool = true;
}

Expand Down
2 changes: 1 addition & 1 deletion benches/benches/bevy_ecs/observers/simple.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
use bevy_ecs::{entity::Entity, event::Event, observer::Trigger, world::World};

use criterion::{black_box, criterion_group, criterion_main, Criterion};
use criterion::{black_box, Criterion};
use rand::{prelude::SliceRandom, SeedableRng};
use rand_chacha::ChaCha8Rng;
fn deterministic_rand() -> ChaCha8Rng {
Expand Down
2 changes: 1 addition & 1 deletion benches/benches/bevy_ecs/world/commands.rs
Original file line number Diff line number Diff line change
Expand Up @@ -43,7 +43,7 @@ pub fn spawn_commands(criterion: &mut Criterion) {
bencher.iter(|| {
let mut commands = Commands::new(&mut command_queue, &world);
for i in 0..entity_count {
let mut entity = commands
let entity = commands
.spawn_empty()
.insert_if(A, || black_box(i % 2 == 0))
.insert_if(B, || black_box(i % 3 == 0))
Expand Down
1 change: 0 additions & 1 deletion benches/benches/bevy_reflect/function.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,4 @@
use bevy_reflect::func::{ArgList, IntoFunction, IntoFunctionMut, TypedFunction};
use bevy_reflect::prelude::*;
use criterion::{criterion_group, criterion_main, BatchSize, Criterion};

criterion_group!(benches, typed, into, call, clone);
Expand Down
2 changes: 1 addition & 1 deletion crates/bevy_ecs/macros/src/component.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ 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;
type Traversal = ();
const AUTO_PROPAGATE: bool = false;
}

Expand Down
10 changes: 5 additions & 5 deletions crates/bevy_ecs/src/observer/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -91,7 +91,7 @@ impl<'w, E, B: Bundle> Trigger<'w, E, B> {
/// 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.
/// use `()` which ends the path immediately and prevents propagation.
///
/// To enable propagation, you must:
/// + Set [`Event::Traversal`] to the component you want to propagate along.
Expand Down Expand Up @@ -565,17 +565,17 @@ mod tests {
#[derive(Component)]
struct Parent(Entity);

impl Traversal for Parent {
fn traverse(&self) -> Option<Entity> {
Some(self.0)
impl Traversal for &'_ Parent {
fn traverse(item: Self::Item<'_>) -> Option<Entity> {
Some(item.0)
}
}

#[derive(Component)]
struct EventPropagating;

impl Event for EventPropagating {
type Traversal = Parent;
type Traversal = &'static Parent;

const AUTO_PROPAGATE: bool = true;
}
Expand Down
29 changes: 7 additions & 22 deletions crates/bevy_ecs/src/traversal.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,11 @@
//! A trait for components that let you traverse the ECS.
use crate::{
component::{Component, StorageType},
entity::Entity,
};
use crate::{entity::Entity, query::ReadOnlyQueryData};

/// A component that can point to another entity, and which can be used to define a path through the ECS.
///
/// Traversals are used to [specify the direction] of [event propagation] in [observers]. By default,
/// events use the [`TraverseNone`] placeholder component, which cannot actually be created or added to
/// an entity and so never causes traversal.
/// Traversals are used to [specify the direction] of [event propagation] in [observers].
/// The default query is `()`.
///
/// Infinite loops are possible, and are not checked for. While looping can be desirable in some contexts
/// (for example, an observer that triggers itself multiple times before stopping), following an infinite
Expand All @@ -20,24 +16,13 @@ use crate::{
/// [specify the direction]: crate::event::Event::Traversal
/// [event propagation]: crate::observer::Trigger::propagate
/// [observers]: crate::observer::Observer
pub trait Traversal: Component {
pub trait Traversal: ReadOnlyQueryData {
/// Returns the next entity to visit.
fn traverse(&self) -> Option<Entity>;
fn traverse(item: Self::Item<'_>) -> Option<Entity>;
}

/// A traversal component that doesn't traverse anything. Used to provide a default traversal
/// implementation for events.
///
/// It is not possible to actually construct an instance of this component.
pub enum TraverseNone {}

impl Traversal for TraverseNone {
#[inline(always)]
fn traverse(&self) -> Option<Entity> {
impl Traversal for () {
fn traverse(_: Self::Item<'_>) -> Option<Entity> {
None
}
}

impl Component for TraverseNone {
const STORAGE_TYPE: StorageType = StorageType::Table;
}
18 changes: 11 additions & 7 deletions crates/bevy_ecs/src/world/deferred_world.rs
Original file line number Diff line number Diff line change
Expand Up @@ -148,8 +148,8 @@ impl<'w> DeferredWorld<'w> {
match self.get_resource_mut() {
Some(x) => x,
None => panic!(
"Requested resource {} does not exist in the `World`.
Did you forget to add it using `app.insert_resource` / `app.init_resource`?
"Requested resource {} does not exist in the `World`.
Did you forget to add it using `app.insert_resource` / `app.init_resource`?
Resources are also implicitly added via `app.add_event`,
and can be added by plugins.",
std::any::type_name::<R>()
Expand Down Expand Up @@ -178,8 +178,8 @@ impl<'w> DeferredWorld<'w> {
match self.get_non_send_resource_mut() {
Some(x) => x,
None => panic!(
"Requested non-send resource {} does not exist in the `World`.
Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`?
"Requested non-send resource {} does not exist in the `World`.
Did you forget to add it using `app.insert_non_send_resource` / `app.init_non_send_resource`?
Non-send resources can also be added by plugins.",
std::any::type_name::<R>()
),
Expand Down Expand Up @@ -387,15 +387,15 @@ impl<'w> DeferredWorld<'w> {
/// # Safety
/// Caller must ensure `E` is accessible as the type represented by `event`
#[inline]
pub(crate) unsafe fn trigger_observers_with_data<E, C>(
pub(crate) unsafe fn trigger_observers_with_data<E, T>(
&mut self,
event: ComponentId,
mut entity: Entity,
components: &[ComponentId],
data: &mut E,
mut propagate: bool,
) where
C: Traversal,
T: Traversal,
{
loop {
Observers::invoke::<_>(
Expand All @@ -409,7 +409,11 @@ impl<'w> DeferredWorld<'w> {
if !propagate {
break;
}
if let Some(traverse_to) = self.get::<C>(entity).and_then(C::traverse) {
if let Some(traverse_to) = self
.get_entity(entity)
.and_then(|entity| entity.get_components::<T>())
.and_then(T::traverse)
{
entity = traverse_to;
} else {
break;
Expand Down
6 changes: 3 additions & 3 deletions crates/bevy_hierarchy/src/components/parent.rs
Original file line number Diff line number Diff line change
Expand Up @@ -79,8 +79,8 @@ impl Deref for Parent {
/// `Parent::traverse` will never form loops in properly-constructed hierarchies.
///
/// [event propagation]: bevy_ecs::observer::Trigger::propagate
impl Traversal for Parent {
fn traverse(&self) -> Option<Entity> {
Some(self.0)
impl Traversal for &Parent {
fn traverse(item: Self::Item<'_>) -> Option<Entity> {
Some(item.0)
}
}
2 changes: 1 addition & 1 deletion crates/bevy_picking/src/events.rs
Original file line number Diff line number Diff line change
Expand Up @@ -73,7 +73,7 @@ impl<E> Event for Pointer<E>
where
E: Debug + Clone + Reflect,
{
type Traversal = Parent;
type Traversal = &'static Parent;

const AUTO_PROPAGATE: bool = true;
}
Expand Down
2 changes: 1 addition & 1 deletion examples/ecs/observer_propagation.rs
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ impl Event for Attack {
// 1. Which component we want to propagate along. In this case, we want to "bubble" (meaning propagate
// from child to parent) so we use the `Parent` component for propagation. The component supplied
// must implement the `Traversal` trait.
type Traversal = Parent;
type Traversal = &'static Parent;
// 2. We can also choose whether or not this event will propagate by default when triggered. If this is
// false, it will only propagate following a call to `Trigger::propagate(true)`.
const AUTO_PROPAGATE: bool = true;
Expand Down

0 comments on commit 27bea6a

Please sign in to comment.