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

Conversation

NthTensor
Copy link
Contributor

@NthTensor NthTensor commented Jun 23, 2024

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 NthTensor added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events A-Hierarchy Parent-child entity hierarchies S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 23, 2024
@alice-i-cecile alice-i-cecile requested a review from maniwani June 23, 2024 14:38
@alice-i-cecile alice-i-cecile added this to the 0.15 milestone Jun 23, 2024
@alice-i-cecile alice-i-cecile requested a review from cart June 23, 2024 14:39
@alice-i-cecile alice-i-cecile added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jun 23, 2024
Copy link
Member

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Just a few typos on some internal test items (awesome tests btw!)

crates/bevy_ecs/src/observer/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/observer/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/observer/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/observer/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/observer/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/observer/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/observer/mod.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

I'm not too familiar with the observer code (or ECS details in general), but I didn't spot any obvious issues based on a quick look. I mainly went through the docs and how this works at a high level.

The core implementation looks solid, and will definitely be highly useful for picking and UI, as well as many general patterns. I like that you can implement custom traversal: for example in bevy_xpbd, I have a ColliderParent component, and I could use it to propagate collision events from child colliders directly to rigid body entities.

I do agree with @MrGVSV that the current terminology is potentially misleading. Bubbling in my mind always goes up in the hierarchy, which is not necessarily the case for us if we allow custom traversal.

Overall I'd like to see some improvements to the documentation here: What is event bubbling? What could it be useful for? How do I stop propagation? What is Event::Traverse actually used for? How can I implement my own traversal? These are largely answered in the PR description, but I'd like it in the actual documentation.

crates/bevy_ecs/src/event/base.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/event/base.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/event/base.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/observer/runner.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/traversal.rs Outdated Show resolved Hide resolved
@NthTensor
Copy link
Contributor Author

NthTensor commented Jun 24, 2024

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?

@alice-i-cecile
Copy link
Member

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.

crates/bevy_ecs/src/observer/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/observer/mod.rs Outdated Show resolved Hide resolved
@NthTensor
Copy link
Contributor Author

NthTensor commented Jun 30, 2024

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.

@aevyrie
Copy link
Member

aevyrie commented Jun 30, 2024

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

@NthTensor
Copy link
Contributor Author

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.

Copy link
Contributor

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

Copy link
Contributor

@MiniaczQ MiniaczQ left a comment

Choose a reason for hiding this comment

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

Very elegant :)

crates/bevy_ecs/src/traversal.rs Outdated Show resolved Hide resolved
@@ -662,4 +725,210 @@ 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.

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?

crates/bevy_ecs/src/traversal.rs Outdated Show resolved Hide resolved
@NthTensor
Copy link
Contributor Author

NthTensor commented Jul 9, 2024

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 NthTensor added X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed X-Contentious There are nontrivial implications that should be thought through S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 9, 2024
@NthTensor
Copy link
Contributor Author

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.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jul 15, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 15, 2024
Merged via the queue into bevyengine:main with commit ed2b8e0 Jul 15, 2024
32 checks passed
@alice-i-cecile
Copy link
Member

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 bevyengine/bevy-website#1657 if you'd like to help out.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events A-Hierarchy Parent-child entity hierarchies A-Picking Pointing at and selecting objects of all sorts C-Feature A new feature, making something new possible M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Blessed Has a large architectural impact or tradeoffs, but the design has been endorsed by decision makers
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.