bevy icon indicating copy to clipboard operation
bevy copied to clipboard

Minimal Bubbling Observers

Open NthTensor opened this issue 1 year ago • 10 comments

Objective

Add basic bubbling to observers, modeled off bevy_eventlistener.

Solution

  • Introduce a new Traversal trait for components which point to other entities.
  • Provide a default TraverseNone: Traversal component which cannot be constructed.
  • Implement Traversal for Parent.
  • The Event trait now has an associated Traversal which defaults to TraverseNone.
  • Added a field bubbling: &mut bool to Trigger which can be used to instruct the runner to bubble the event to the entity specified by the event's traversal type.
  • Added an associated constant SHOULD_BUBBLE to Event which configures the default bubbling state.
  • Added logic to wire this all up correctly.

Introducing the new associated information directly on Event (instead of a new BubblingEvent trait) lets us dispatch both bubbling and non-bubbling events through the same api.

Testing

I have added several unit tests to cover the common bugs I identified during development. Running the unit tests should be enough to validate correctness. The changes effect unsafe portions of the code, but should not change any of the safety assertions.

Changelog

Observers can now bubble up the entity hierarchy! To create a bubbling event, change your Derive(Event) to something like the following:

#[derive(Component)]
struct MyEvent;

impl Event for MyEvent {
    type Traverse = Parent; // This event will propagate up from child to parent.
    const AUTO_PROPAGATE: bool = true; // This event will propagate by default.
}

You can dispatch a bubbling event using the normal world.trigger_targets(MyEvent, entity).

Halting an event mid-bubble can be done using trigger.propagate(false). Events with AUTO_PROPAGATE = false will not propagate by default, but you can enable it using trigger.propagate(true).

If there are multiple observers attached to a target, they will all be triggered by bubbling. They all share a bubbling state, which can be accessed mutably using trigger.propagation_mut() (trigger.propagate is just sugar for this).

You can choose to implement Traversal for your own types, if you want to bubble along a different structure than provided by bevy_hierarchy. Implementers must be careful never to produce loops, because this will cause bevy to hang.

Migration Guide

  • Manual implementations of Event should add associated type Traverse = TraverseNone and associated constant AUTO_PROPAGATE = false;
  • Trigger::new has new field propagation: &mut Propagation which provides the bubbling state.
  • ObserverRunner now takes the same &mut Propagation as a final parameter.

NthTensor avatar Jun 23 '24 14:06 NthTensor

I would like advice on how best to document this, since it doesn't really expose anything publicly by default, except for perhaps the new Traversal trait (which I am writing better docs for).

The docs entry-point for observers seems to be the Observer component. Does it make more sense to talk about this there, on Event or both?

NthTensor avatar Jun 24 '24 02:06 NthTensor

My preference is to primarily cover this on Traversal, but link to that trait's docs with a one sentence explanation of how / why this might be used in both Event and Observer's docs.

alice-i-cecile avatar Jun 24 '24 14:06 alice-i-cecile

I resolved all the issues identified in the reviews and I was in the process of porting over the examples from bevy_eventlistener when I identified a rather large footgun with how global observers (ones who don't target a specific list of entities) interact with bubbling; eg, they don't.

I'll be rewriting this a bit over the next week to fix this issue.

NthTensor avatar Jun 30 '24 04:06 NthTensor

Completely non-blocking - have you done any sort of performance profiling? I'm curious how this scales.

aevyrie avatar Jun 30 '24 09:06 aevyrie

I have not yet profiled. I am expecting it to be somewhat slower. Will port over the eventlistener stress test once I fix the footguns.

NthTensor avatar Jun 30 '24 11:06 NthTensor

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

github-actions[bot] avatar Jun 30 '24 18:06 github-actions[bot]

Your PR increases Bevy Minimum Supported Rust Version. Please update the rust-version field in the root Cargo.toml file.

github-actions[bot] avatar Jun 30 '24 18:06 github-actions[bot]

Alright, resolved feedback, improved docs, changed how things worked a bit to work with global observers, simplified things a bit, added an example. Should be ready for reviews.

I intended to investigate porting some of the hooks stuff over to use slices instead of iterators as future work.

NthTensor avatar Jul 01 '24 00:07 NthTensor

Since the trigger entity method typically returns the target entity of the explicit user API trigger targets I think it would be worth to add that propagation changes this implicitly when propagation is occurring. Imo a sentence about that in the entity method would be good

torsteingrindvik avatar Jul 05 '24 19:07 torsteingrindvik

Great, thank you! I prefer the bool as well, that's how I had it implemented before the first round of feedback.

I'll get this cleaned up, benchmarked, and ready for merge by next monday then.

When this is done, I'll spin up some issues for follow up work that we uncovered:

  • Traversal should be changed to use QueryData as described here.
  • The observers tests should be updated to use a hash-map of counters probably.
  • Consider moving trigger_observers over to slices rather than iterators, to match the new version of trigger_observers_with_data.

NthTensor avatar Jul 09 '24 09:07 NthTensor

Addressed review comments, added a benchmark, cleaned things up a bit.

Preliminary profiling indicates this is a regression compared to the best case of bevy_eventlisterner but (a) its unlikely to cause problems in production and (b) we expect to be able to improve perf at a later stage.

If this passes tests I think it's good for merge.

NthTensor avatar Jul 15 '24 00:07 NthTensor

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to https://github.com/bevyengine/bevy-website/issues/1657 if you'd like to help out.

alice-i-cecile avatar Oct 20 '24 14:10 alice-i-cecile