Skip to content

Add observer ordering via ObserverSet and topo-sorted dispatch#24328

Open
caniko wants to merge 18 commits into
bevyengine:mainfrom
caniko:feat/observer-ordering
Open

Add observer ordering via ObserverSet and topo-sorted dispatch#24328
caniko wants to merge 18 commits into
bevyengine:mainfrom
caniko:feat/observer-ordering

Conversation

@caniko
Copy link
Copy Markdown

@caniko caniko commented May 17, 2026

Add observer ordering via ObserverSet and topo-sorted dispatch

Resolves #14890.

What this changes

Observers can be ordered against each other using a SystemSet-style API:

app.add_observer(score.in_set(WinCheck));
app.add_observer(announce.after(WinCheck));

Cross-bucket ordering works natively: a global observer can be ordered against a per-entity or per-component observer. The implementation uses one topo sort per event key, then reuses the sorted order across all dispatch sites. The existing archetype-flag fast skip is preserved.

Approach

  • ObserverSet trait + derive, mirroring SystemSet. Identities are stored as Interned<dyn ObserverSet>.
  • CachedObservers replaces the old EntityHashMap buckets with one node table, one topo-sorted order, and sorted inverted indices per bucket.
  • Topo sort reuses bevy_ecs::schedule::graph::DiGraph, so observers and systems share the same ordering kernel.
  • Dispatch in event/trigger.rs goes through one run_ordered helper that performs a k-way merge over sorted NodeId streams. No per-dispatch allocation.
  • Dynamic trigger helpers are routed through the same ordered dispatch path.

Breaking changes

  1. ObserverMap type alias removed. Replaced by indexed-Vec storage. Downstream code reading world.observers() to enumerate observers needs to use a new iterator API. There are very few such users in the ecosystem (mostly debug tools).
  2. CachedObservers / CachedComponentObservers field shapes change. The fields were already private; only the accessors (global_observers(), etc.) were public. The new accessors return sorted slices of NodeId plus a nodes() table.
  3. Iteration order becomes deterministic even for callers who never touch ObserverSet. Strictly stronger than today's contract; nothing correct can break, but anything that relied on undefined order (test asserts, frame replays) will need updates. Call out in the PR.
  4. Dispatch sites in event/trigger.rs change shape. Internal, no Trigger trait signature changes, but custom Trigger impls in the wild (Bevy picking is the largest) need a quick port to the new run_ordered helper. Bevy maintainers can drive this in the same PR.

What's NOT in this PR

  • Parallel observer execution. This needs SystemParam access analysis per observer to build a conflict graph.
  • Propagation hop reordering. Hop order is owned by Traversal; per-hop dispatch now uses the sorted observer order.
  • ambiguous_with for observers. Only meaningful once parallel observer execution exists.

Design Notes

Design summary

The key design choice is one event-local observer graph, not four separately sorted buckets. This enables edges across dispatch buckets while avoiding four topo sorts per event. Each registered observer/event pair becomes an ObserverNode with a stable insertion sort key. CachedObservers stores a dense node table, an event-wide sorted order, and bucket indices containing NodeIds sorted by that event-wide order.

Dispatch sites pass the relevant sorted streams into run_ordered. For one stream, dispatch is a direct dense slice walk. For multiple streams without ordering edges, dispatch preserves existing bucket order without allocation. If ordering edges exist, run_ordered walks the event-wide order and runs nodes present in the active streams, which gives cross-bucket ordering without allocating a merged list.

Archetype flags remain the fast skip for lifecycle component observers. Register/unregister returns the same component-observer deltas needed to update those flags, now backed by the new component bucket indices.

Test coverage

New and updated bevy_ecs observer coverage includes:

  • ObserverSet derive and set membership.
  • before/after ordering by observer entity and by set.
  • multiple-set membership and empty-set targets.
  • cycle detection behavior.
  • unregister-preserves-order.
  • cross-bucket ordering.
  • archetype-flag preservation.
  • propagation using sorted dispatch per hop.
  • nested sets.
  • dispatch_order_for accessor coverage.
  • dynamic trigger helpers routed through ordered dispatch.
  • existing lifecycle order tests kept intact and ported to the current Discard lifecycle terminology.

Local validation

  • cargo check --workspace: passed.
  • cargo test -p bevy_ecs --doc: passed.
  • cargo clippy -p bevy_ecs -- -D warnings: passed.
  • cargo test -p bevy_ecs: observer tests pass, but the full suite is blocked locally by error::bevy_error::tests::filtered_backtrace_test. I verified the same test fails on untouched upstream main (370be1b02) under Rust 1.95.0 with the same assertion, so this is not caused by this branch.

Review feedback addressed

  • 46cf6f428 fixes the run_ordered single-stream fast path so observer-set ordering still applies when the dispatch degenerates to one active stream.
  • bf7bdb9ca merges component dispatch streams so cross-bucket ordering is preserved for component-targeted observers too.
  • 814294c57 covers observer-set chain ordering interaction with .run_if on a middle observer (per @alice-i-cecile's ask).

Follow-up cleanups

Two additional commits round out the API surface and tighten a few internals:

c4588791f — observer API completion + performance + ergonomics:

  • observe() on EntityWorldMut / EntityCommands / entity_command::observe, and add_observer() on World / Commands, now accept impl IntoObserverConfigs<M> so the same builder chain (.in_set(...), .before(...), .after(...), .chain()) works through every entry point. Entity-side sites attach watch_entity to every observer in the configs before spawning.
  • Replaces a stale ObserverSet docstring that still claimed observer set configuration was "not yet wired into dispatch".
  • The cycle-detection warning now includes observer names alongside entities, so the diagnostic is actionable for users who set Observer::with_name(...).
  • World::add_observers and World::configure_observer_sets defer per-cache resort() calls for the duration of the batch via a counter-based deferral on Observers, so a plugin that registers N observers runs one topological sort instead of N. Panic-safe via a Drop guard (no_std-clean).
  • Observer::watch_entity / watch_entities debug-assert when called after the descriptor has been frozen by registration; release builds preserve the documented silent no-op contract for back-compat.
  • Adds a regression test confirming (a, b, c).chain().in_set(MySet) produces deduplicated edges in the topological graph (relies on DiGraph::add_edge naturally deduplicating identical (from, to) pairs).
  • Factors out a shared build_ordering_graph helper so rebuild_order and the test-only graph view use the same implementation.

036c12513resolve_set_target uses a HashSet for visited tracking instead of Vec + linear contains(), and the dispatch_order_for_* family carries a doc note that it allocates per call and is intended for diagnostics, not hot paths.

caniko added 9 commits May 17, 2026 12:23
Foundation for observer ordering (bevyengine#14890). No behaviour change yet — descriptor edges are unread by storage and dispatch; that wiring lands in later commits.
All four dispatch sites in event/trigger.rs now route through a single ordered dispatch helper over sorted NodeId streams. Archetype-flag fast skip is preserved. Cross-bucket ordering is now observable at runtime when observer ordering edges exist.
@github-actions
Copy link
Copy Markdown
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide, as well as our policy regarding AI usage, and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile modified the milestones: 0.19, 0.20 May 17, 2026
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-ECS Entities, components, systems, and events M-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 17, 2026
@github-project-automation github-project-automation Bot moved this to Needs SME Triage in ECS May 17, 2026
@github-actions
Copy link
Copy Markdown
Contributor

It looks like your PR has been selected for a highlight in the next release blog post, but you didn't provide a release note.

Please review the instructions for writing release notes, then expand or revise the content in the release notes directory to showcase your changes.

@alice-i-cecile
Copy link
Copy Markdown
Member

run_if already works now :) Add a test for the interaction?

@caniko
Copy link
Copy Markdown
Author

caniko commented May 17, 2026

Added follow-up commits completing the public API surface to match SystemSet's shape:

  • add_observers((a, b, c)) plural entry point.
  • IntoObserverConfigs tuple trait via all_tuples!.
  • Tuple-level .in_set / .before / .after / .chain modifiers.
  • configure_observer_sets((A, B).chain()) for set-level ordering.
  • Observer::with_name(&str) for diagnostic labels.
  • dispatch_order_for_set / _for_target / _with_names accessor variants.

Storage and dispatch are unchanged from the previous commits; this is additive on the builder and diagnostics layer. The new observer tests cover tuple chaining, tuple-wide modifiers, set chaining, naming, and filtered dispatch-order accessors.

I also pushed a small follow-up formatting commit after CI caught stable clippy's semicolon_if_nothing_returned lint in the new tests.

@caniko
Copy link
Copy Markdown
Author

caniko commented May 17, 2026

The run_ordered fast path still needs to honor observer-set ordering when dispatch collapses to a single active stream:
Fixed in 6139a0c3e (ecs/observer: honor set ordering in run_ordered fast-path).

Component-targeted observers still dispatch as separate ordered passes, which can violate the event-local cross-bucket ordering this PR is meant to provide:
Fixed in bf7bdb9ca (ecs/observer: merge component dispatch streams).

@caniko
Copy link
Copy Markdown
Author

caniko commented May 17, 2026

Will test this in my game, and see if I find any further bugs before marking the PR as ready. Feel free to try out the PR in the meantime! Thanks all!

@caniko
Copy link
Copy Markdown
Author

caniko commented May 18, 2026

Follow-up cleanups force-pushed (commits re-signed, audit-fix message reworded):

  • API completion (IntoObserverConfigs across all observe/add_observer entry points), named cycle warnings, deferred-resort batch perf, post-spawn watch_entity debug-assert, chain/in_set edge dedup verification, and a shared graph-builder refactor: f1d1ea6eb
  • resolve_set_target HashSet + diagnostic-cost docs on dispatch_order_for_*: 729ff413e

The review-feedback commits (set ordering in the fast path, merged component dispatch) were also re-signed in the same rebase. SHAs in the PR description are now up to date.

@caniko caniko force-pushed the feat/observer-ordering branch 2 times, most recently from 729ff41 to 987f280 Compare May 19, 2026 07:41
caniko added 2 commits May 19, 2026 09:55
API completion + diagnostics:
- accept IntoObserverConfigs in observe()/add_observer() sugar
  across EntityWorldMut, EntityCommands, free entity_command::observe,
  World::add_observer, and Commands::add_observer; entity-side sites
  thread watch_entity + chain through configs.observers
- replace the stale "not yet wired into dispatch" docstring on
  ObserverSet with an accurate description of the live API
- include observer names alongside entities in the cycle warning

Performance:
- defer per-cache resort() during batch registration via counter-based
  bump_defer_resort / release_defer_resort on Observers (propagated
  to all caches; newly-created caches inherit the count); wrap
  World::add_observers and World::configure_observer_sets so the
  topological sort runs once per call instead of once per inner
  register_observer; panic-safe via catch_unwind + resume_unwind

Ergonomics:
- debug-assert on watch_entity / watch_entities called after the
  descriptor has been frozen by registration; release builds keep
  the silent no-op contract
- chain() + in_set() edges are verified to deduplicate in the topo
  graph via a regression test, relying on DiGraph::add_edge natively
  deduplicating identical (from, to) pairs

Internal:
- extract a shared build_ordering_graph helper used by both
  rebuild_order and the test-only ordering-graph view
@caniko caniko force-pushed the feat/observer-ordering branch from 987f280 to 036c125 Compare May 19, 2026 07:57
@caniko
Copy link
Copy Markdown
Author

caniko commented May 19, 2026

Re-pushed with a rustfmt fold-in and a no_std fix.

  • The IntoObserverConfigs / watch_entity audit had a few rustfmt diffs; folded back into the follow-up commit so cargo fmt --all -- --check passes.
  • World::add_observers / World::configure_observer_sets used std::panic::{catch_unwind, resume_unwind} for the batched-defer cleanup, which broke check-compiles-no-std. Replaced with a private Drop-guard struct so the deferred-resort counter releases on both normal return and unwind, without needing std. Same panic-safety semantics, no_std-clean.

Updated SHAs:

  • c4588791f — follow-up audit fixes (was f1d1ea6eb)
  • 036c12513resolve_set_target HashSet + dispatch_order_for_* doc note (was 729ff413e)

Review-feedback commits (46cf6f428, bf7bdb9ca) are unchanged. CI is now fully green.

@caniko
Copy link
Copy Markdown
Author

caniko commented May 19, 2026

Added the ordering × run_if interaction test you asked for, @alice-i-cecile: 62a485cb9. The test sets up a three-set chain (SetA, SetB, SetC).chain() and puts .run_if(|flag| flag.0) on the middle observer, then verifies that with the flag off the chain dispatches as [a, c] (middle skipped, neighbours still ordered) and with the flag on it dispatches as [a, b, c].

Also added the missing release-notes entry the bot flagged (_release-content/release-notes/observer_ordering.md) and dropped the stale ".run_if is NOT in this PR" bullet from the description, since run conditions for observers have been in tree since #22602.

Adds the test alice-i-cecile asked for: an ordered chain across three
observer sets where the middle observer carries a `.run_if(false)`
condition. Verifies the conditional observer is skipped without
breaking the order of its neighbours, and re-runs with the flag
flipped to confirm full chain dispatch.

Also adds the observer-ordering release note that the release-content
bot flagged as missing.
@caniko caniko force-pushed the feat/observer-ordering branch from 62a485c to 814294c Compare May 19, 2026 11:12
@caniko caniko marked this pull request as ready for review May 19, 2026 12:01
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 C-Feature A new feature, making something new possible M-Release-Note Work that should be called out in the blog due to impact S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged

Projects

Status: Needs SME Triage

Development

Successfully merging this pull request may close these issues.

Observer ordering/scheduling

2 participants