Use SlotMaps to store systems and system sets in Schedules
#19352
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ItsDoot
added
A-ECS
|
Your PR increases Bevy Minimum Supported Rust Version. Please update the |
ItsDoot
added
S-Needs-Review
urben1680
approved these changes
Jul 2, 2025
chescock
approved these changes
Jul 2, 2025
ItsDoot
added
S-Ready-For-Final-Review
|
Good stuff :) I'm looking forward to progress on dynamic system scheduling after all these years <3 |
github-merge-queue bot
pushed a commit
that referenced
this pull request
Jul 8, 2025
# Objective Make the schedule graph code more understandable, and replace some panics with `Result`s. I found the `check_edges` and `check_hierarchy` functions [a little confusing](#19352 (comment)), as they combined two concerns: Initializing graph nodes for system sets, and checking for self-edges on system sets. It was hard to understand the self-edge checks because it wasn't clear what `NodeId` was being checked against! So, let's separate those concerns, and move them to more appropriate locations. Fix a bug where `schedule.configure_sets((SameSet, SameSet).chain());` would panic with an unhelpful message: `assertion failed: index_a < index_b`. ## Solution Remove the `check_edges` and `check_hierarchy` functions, separating the initialization behavior and the checking behavior and moving them where they are easier to understand. For initializing graph nodes, do this on-demand using the `entry` API by replacing later `self.system_set_ids[&set]` calls with a `self.system_sets.get_or_add_set(set)` method. This should avoid the need for an extra pass over the graph and an extra lookup. Unfortunately, implementing that method directly on `ScheduleGraph` leads to borrowing errors as it borrows the entire `struct`. So, split out the collections managing system sets into a separate `struct`. For checking self-edges, move this check later so that it can be reported by returning a `Result` from `Schedule::initialize` instead of having to panic in `configure_set_inner`. The issue was that `iter_sccs` does not report self-edges as cycles, since the resulting components only have one node, but that later code assumes all edges point forward. So, check for self-edges directly, immediately before calling `iter_sccs`. This also ensures we catch *every* way that self-edges can be added. The previous code missed an edge case where `chain()`ing a set to itself would create a self-edge and would trigger a `debug_assert`.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Solution
Makes the necessary internal data structure changes in order to allow system removal to be added in a future PR:
Vecs storing systems and system sets inScheduleGraphhave been replaced withSlotMaps.See the included migration guide for the required changes.
Testing
Internal changes only and no new features should mean no new tests are requried.