-
-
Notifications
You must be signed in to change notification settings - Fork 4.2k
Pull Schedules::ignored_scheduling_ambiguities into a separate resource and avoid unnecessary allocation
#21929
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
base: main
Are you sure you want to change the base?
Conversation
06f7c47 to
3982de5
Compare
|
|
||
| /// List of [`ComponentId`]s to ignore when reporting system order ambiguity conflicts. | ||
| #[derive(Resource, Default)] | ||
| pub struct IgnoredAmbiguities(pub HashSet<ComponentId>); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each schedule is going to produce it's own ambiguities. Shouldn't this be like HashMap<ScheduleLabel, IndexSet<ComponentId>>? or maybe a component might be better. Since you can store schedules outside of the Schedules resource and this would keep them from overwriting if they share a schedule label.
- also change it to an IndexSet because we want a stable iteration order.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Each schedule is going to produce it's own ambiguities. Shouldn't this be like
HashMap<ScheduleLabel, IndexSet<ComponentId>>? or maybe a component might be better. Since you can store schedules outside of theSchedulesresource and this would keep them from overwriting if they share a schedule label.
ScheduleGraph::ambiguous_with_all and ScheduleGraph::ambiguous_with are the per-Schedule ambiguity stores, this is the global one. I'm just moving the global one outside of the Schedules resource. I can add a flag to Schedule for whether it should use the global storage or not?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We could also do a two-tier system where each Schedule also has their own HashSet<ComponentId>, and have a flag to also fetch from the global set.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW, before and after this PR as-is, all Schedules have to adhere to the global ambiguity set. So adding an opt-out would be a new thing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
* also change it to an IndexSet because we want a stable iteration order.
The engine itself doesn't iterate the set, and I can't find any examples on GitHub (including bevy_mod_debugdump) of people iterating the set, so I don't think we need to make it an IndexSet.
8e773b2 to
e135f22
Compare
Objective
While it is related to the scheduling infrastructure, there is no need for ignored scheduling ambiguities to be directly co-located in
Schedules. We can also avoid acloneby storing them separately.Solution
Schedules::ignored_scheduling_ambiguitiesinto its ownResource:IgnoredAmbiguities.HashSetinstead of aBTreeSet; we don't have a need for ordering.Testing
Reusing current tests.