-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
run_if
for SystemConfigs
via anonymous system sets
#7676
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
Oh very clever. I'll do a full review and add this to the milestone. Welcome and thank you! |
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.
Generally very impressed. I'd like to see some basic tests for hashing and equality added, based on the behavior in the PR description.
Haven't done a full review yet, but I do think this is probably the right path forward. But I'm very curious what others think (especially @maniwani, given their work in this area). First some pros:
Some cons:
|
I'd like to consolidate this in the future, and make all instances of run conditions with the same name get checked exactly once. Users who want different timing can newtype. I think that the way that each
Agreed, I'm feeling a bit iffy about the naming questions here.
Agreed. Now that I'm thinking more about how this works, I think this should be fixed here to avoid making a complete mess of scheduling viz. |
Example |
1 similar comment
Example |
Okay, I adjusted
I agree that for schedule visualizations detailed names aren't necessary because they are redundant if you can already see the members in the visualization.
I have switched to using an incremented atomic counter for the implicit sets. I already have thought about that, but was worried about the error messages. But now I know that we can simply reconstruct the detailed set name when an error occurs. What do you think about adding an |
Summarization of my latest changes:
With this, I think every mentioned concern excluding the behavioral difference between |
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.
Should add some documentation to App::add_systems and Schedule::add_systems that using run_if, after, etc adds a anonymous system set.
Hmmm not (yet) sold on that. Having conditions be "assigned" to specific groups of systems, and having them run right before the first system in the group is definitely implicit. But it ties the condition to specific contexts / places in the schedule, which gives them a good chance of being evaluated at the "right time". And the behavior is predictable (and roughly controllable), if you know the rules. Evaluating them globally removes a lot of "locational context". Definitely a hard problem / one worth discussing. Something we could consider: add "dataflow" to the graph to make these things more clear. Allow scheduling systems that produce outputs (ex: |
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.
The latest changes resolve the concerns I listed. I'm on board!
I've commited all requested changes (except for the |
From a first pass, I do not like the change from "distributive" to "transitive".
Yeah, I think this behavior is too subtle. I'd be onboard if
IMO these aren't pros. The act of putting things in a set can give benefits, but we do not want to group systems by accident.
This doesn't really make sense. Take
(The "just before" is literal, but the actual invariant is that any data accessed by the conditions is not mutated in between the conditions being evaluated and the (first of the) guarded system(s) running.)
AFAIK dependencies can't (yet) express "before and after + no writer in between". That realization is what led to the current impl. It's the same problem as "before and after + sync point in between". Even if we could do those, having conditions in the graph would lengthen schedule build times (and make visualizations noiser unless they were filtered out).
|
@maniwani How about adding a This is probably what you meant by |
Yep, I would be satisfied with that. Though we still wouldn't have a method that does copy a condition to all systems like a hypothetical |
I extracted the |
# Objective - Fixes #7659. ## Solution - This PR extracted the `distributive_run_if` part of #7676, because it does not require the controversial introduction of anonymous system sets. - The distinctive name should make the user aware about the differences between `IntoSystemConfig::run_if` and `IntoSystemConfigs::distributive_run_if`. - The documentation explains in detail the consequences of using the API and possible pit falls when using it. - A test demonstrates the possibility of changing the condition result, resulting in some of the systems not being run. --- ## Changelog ### Added - Add `distributive_run_if` to `IntoSystemConfigs` to enable adding a run condition to each system when using `add_systems`.
# Objective - Fixes bevyengine#7659. ## Solution - This PR extracted the `distributive_run_if` part of bevyengine#7676, because it does not require the controversial introduction of anonymous system sets. - The distinctive name should make the user aware about the differences between `IntoSystemConfig::run_if` and `IntoSystemConfigs::distributive_run_if`. - The documentation explains in detail the consequences of using the API and possible pit falls when using it. - A test demonstrates the possibility of changing the condition result, resulting in some of the systems not being run. --- ## Changelog ### Added - Add `distributive_run_if` to `IntoSystemConfigs` to enable adding a run condition to each system when using `add_systems`.
Looking forward to this. Following the discussions I thought I'd drop another idea // Regular
pub fn add_system<M>(&mut self, system: impl IntoSystemAppConfig<M>)
// Applies all .run_if(...), .before(...), .after(...) in a distributive fashion to all systems, so basically clone the conditions to every system
pub fn add_systems<M>(&mut self, systems: impl IntoSystemAppConfigs<M>)
// Create an anonymous set (sys_a, sys_b) such that run_if(...), before(...), .after(...) applies to the anonymous set
pub fn add_system_set<M>(&mut self, systems: impl IntoSystemSetAppConfig<M>) Then I see some weirdness with app.add_systems((a,b,c).chain())
// vs
app.add_system_set((a,b,c).ordered()) It seems like a good example of future cases where there might be markers such as P.S. I found |
@geieredgar, can you resolve the conflicts or rebase? Once that's done, this should be good to merge. |
3599a45
to
cc5b79c
Compare
I've rebased the PR, but I am not sure if it would be better to just change |
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.
I think this is the right general approach, although the implementation doesn't actually work atm (see comments).
I have fixes prepared that I'll push now.
# Objective First of all, this PR took heavy inspiration from #7760 and #5715. It intends to also fix #5569, but with a slightly different approach. This also fixes #9335 by reexporting `DynEq`. ## Solution The advantage of this API is that we can intern a value without allocating for zero-sized-types and for enum variants that have no fields. This PR does this automatically in the `SystemSet` and `ScheduleLabel` derive macros for unit structs and fieldless enum variants. So this should cover many internal and external use cases of `SystemSet` and `ScheduleLabel`. In these optimal use cases, no memory will be allocated. - The interning returns a `Interned<dyn SystemSet>`, which is just a wrapper around a `&'static dyn SystemSet`. - `Hash` and `Eq` are implemented in terms of the pointer value of the reference, similar to my first approach of anonymous system sets in #7676. - Therefore, `Interned<T>` does not implement `Borrow<T>`, only `Deref`. - The debug output of `Interned<T>` is the same as the interned value. Edit: - `AppLabel` is now also interned and the old `derive_label`/`define_label` macros were replaced with the new interning implementation. - Anonymous set ids are reused for different `Schedule`s, reducing the amount of leaked memory. ### Pros - `InternedSystemSet` and `InternedScheduleLabel` behave very similar to the current `BoxedSystemSet` and `BoxedScheduleLabel`, but can be copied without an allocation. - Many use cases don't allocate at all. - Very fast lookups and comparisons when using `InternedSystemSet` and `InternedScheduleLabel`. - The `intern` module might be usable in other areas. - `Interned{ScheduleLabel, SystemSet, AppLabel}` does implement `{ScheduleLabel, SystemSet, AppLabel}`, increasing ergonomics. ### Cons - Implementors of `SystemSet` and `ScheduleLabel` still need to implement `Hash` and `Eq` (and `Clone`) for it to work. ## Changelog ### Added - Added `intern` module to `bevy_utils`. - Added reexports of `DynEq` to `bevy_ecs` and `bevy_app`. ### Changed - Replaced `BoxedSystemSet` and `BoxedScheduleLabel` with `InternedSystemSet` and `InternedScheduleLabel`. - Replaced `impl AsRef<dyn ScheduleLabel>` with `impl ScheduleLabel`. - Replaced `AppLabelId` with `InternedAppLabel`. - Changed `AppLabel` to use `Debug` for error messages. - Changed `AppLabel` to use interning. - Changed `define_label`/`derive_label` to use interning. - Replaced `define_boxed_label`/`derive_boxed_label` with `define_label`/`derive_label`. - Changed anonymous set ids to be only unique inside a schedule, not globally. - Made interned label types implement their label trait. ### Removed - Removed `define_boxed_label` and `derive_boxed_label`. ## Migration guide - Replace `BoxedScheduleLabel` and `Box<dyn ScheduleLabel>` with `InternedScheduleLabel` or `Interned<dyn ScheduleLabel>`. - Replace `BoxedSystemSet` and `Box<dyn SystemSet>` with `InternedSystemSet` or `Interned<dyn SystemSet>`. - Replace `AppLabelId` with `InternedAppLabel` or `Interned<dyn AppLabel>`. - Types manually implementing `ScheduleLabel`, `AppLabel` or `SystemSet` need to implement: - `dyn_hash` directly instead of implementing `DynHash` - `as_dyn_eq` - Pass labels to `World::try_schedule_scope`, `World::schedule_scope`, `World::try_run_schedule`. `World::run_schedule`, `Schedules::remove`, `Schedules::remove_entry`, `Schedules::contains`, `Schedules::get` and `Schedules::get_mut` by value instead of by reference. --------- Co-authored-by: Joseph <21144246+JoJoJet@users.noreply.github.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective First of all, this PR took heavy inspiration from bevyengine#7760 and bevyengine#5715. It intends to also fix bevyengine#5569, but with a slightly different approach. This also fixes bevyengine#9335 by reexporting `DynEq`. ## Solution The advantage of this API is that we can intern a value without allocating for zero-sized-types and for enum variants that have no fields. This PR does this automatically in the `SystemSet` and `ScheduleLabel` derive macros for unit structs and fieldless enum variants. So this should cover many internal and external use cases of `SystemSet` and `ScheduleLabel`. In these optimal use cases, no memory will be allocated. - The interning returns a `Interned<dyn SystemSet>`, which is just a wrapper around a `&'static dyn SystemSet`. - `Hash` and `Eq` are implemented in terms of the pointer value of the reference, similar to my first approach of anonymous system sets in bevyengine#7676. - Therefore, `Interned<T>` does not implement `Borrow<T>`, only `Deref`. - The debug output of `Interned<T>` is the same as the interned value. Edit: - `AppLabel` is now also interned and the old `derive_label`/`define_label` macros were replaced with the new interning implementation. - Anonymous set ids are reused for different `Schedule`s, reducing the amount of leaked memory. ### Pros - `InternedSystemSet` and `InternedScheduleLabel` behave very similar to the current `BoxedSystemSet` and `BoxedScheduleLabel`, but can be copied without an allocation. - Many use cases don't allocate at all. - Very fast lookups and comparisons when using `InternedSystemSet` and `InternedScheduleLabel`. - The `intern` module might be usable in other areas. - `Interned{ScheduleLabel, SystemSet, AppLabel}` does implement `{ScheduleLabel, SystemSet, AppLabel}`, increasing ergonomics. ### Cons - Implementors of `SystemSet` and `ScheduleLabel` still need to implement `Hash` and `Eq` (and `Clone`) for it to work. ## Changelog ### Added - Added `intern` module to `bevy_utils`. - Added reexports of `DynEq` to `bevy_ecs` and `bevy_app`. ### Changed - Replaced `BoxedSystemSet` and `BoxedScheduleLabel` with `InternedSystemSet` and `InternedScheduleLabel`. - Replaced `impl AsRef<dyn ScheduleLabel>` with `impl ScheduleLabel`. - Replaced `AppLabelId` with `InternedAppLabel`. - Changed `AppLabel` to use `Debug` for error messages. - Changed `AppLabel` to use interning. - Changed `define_label`/`derive_label` to use interning. - Replaced `define_boxed_label`/`derive_boxed_label` with `define_label`/`derive_label`. - Changed anonymous set ids to be only unique inside a schedule, not globally. - Made interned label types implement their label trait. ### Removed - Removed `define_boxed_label` and `derive_boxed_label`. ## Migration guide - Replace `BoxedScheduleLabel` and `Box<dyn ScheduleLabel>` with `InternedScheduleLabel` or `Interned<dyn ScheduleLabel>`. - Replace `BoxedSystemSet` and `Box<dyn SystemSet>` with `InternedSystemSet` or `Interned<dyn SystemSet>`. - Replace `AppLabelId` with `InternedAppLabel` or `Interned<dyn AppLabel>`. - Types manually implementing `ScheduleLabel`, `AppLabel` or `SystemSet` need to implement: - `dyn_hash` directly instead of implementing `DynHash` - `as_dyn_eq` - Pass labels to `World::try_schedule_scope`, `World::schedule_scope`, `World::try_run_schedule`. `World::run_schedule`, `Schedules::remove`, `Schedules::remove_entry`, `Schedules::contains`, `Schedules::get` and `Schedules::get_mut` by value instead of by reference. --------- Co-authored-by: Joseph <21144246+JoJoJet@users.noreply.github.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com>
# Objective First of all, this PR took heavy inspiration from bevyengine#7760 and bevyengine#5715. It intends to also fix bevyengine#5569, but with a slightly different approach. This also fixes bevyengine#9335 by reexporting `DynEq`. ## Solution The advantage of this API is that we can intern a value without allocating for zero-sized-types and for enum variants that have no fields. This PR does this automatically in the `SystemSet` and `ScheduleLabel` derive macros for unit structs and fieldless enum variants. So this should cover many internal and external use cases of `SystemSet` and `ScheduleLabel`. In these optimal use cases, no memory will be allocated. - The interning returns a `Interned<dyn SystemSet>`, which is just a wrapper around a `&'static dyn SystemSet`. - `Hash` and `Eq` are implemented in terms of the pointer value of the reference, similar to my first approach of anonymous system sets in bevyengine#7676. - Therefore, `Interned<T>` does not implement `Borrow<T>`, only `Deref`. - The debug output of `Interned<T>` is the same as the interned value. Edit: - `AppLabel` is now also interned and the old `derive_label`/`define_label` macros were replaced with the new interning implementation. - Anonymous set ids are reused for different `Schedule`s, reducing the amount of leaked memory. ### Pros - `InternedSystemSet` and `InternedScheduleLabel` behave very similar to the current `BoxedSystemSet` and `BoxedScheduleLabel`, but can be copied without an allocation. - Many use cases don't allocate at all. - Very fast lookups and comparisons when using `InternedSystemSet` and `InternedScheduleLabel`. - The `intern` module might be usable in other areas. - `Interned{ScheduleLabel, SystemSet, AppLabel}` does implement `{ScheduleLabel, SystemSet, AppLabel}`, increasing ergonomics. ### Cons - Implementors of `SystemSet` and `ScheduleLabel` still need to implement `Hash` and `Eq` (and `Clone`) for it to work. ## Changelog ### Added - Added `intern` module to `bevy_utils`. - Added reexports of `DynEq` to `bevy_ecs` and `bevy_app`. ### Changed - Replaced `BoxedSystemSet` and `BoxedScheduleLabel` with `InternedSystemSet` and `InternedScheduleLabel`. - Replaced `impl AsRef<dyn ScheduleLabel>` with `impl ScheduleLabel`. - Replaced `AppLabelId` with `InternedAppLabel`. - Changed `AppLabel` to use `Debug` for error messages. - Changed `AppLabel` to use interning. - Changed `define_label`/`derive_label` to use interning. - Replaced `define_boxed_label`/`derive_boxed_label` with `define_label`/`derive_label`. - Changed anonymous set ids to be only unique inside a schedule, not globally. - Made interned label types implement their label trait. ### Removed - Removed `define_boxed_label` and `derive_boxed_label`. ## Migration guide - Replace `BoxedScheduleLabel` and `Box<dyn ScheduleLabel>` with `InternedScheduleLabel` or `Interned<dyn ScheduleLabel>`. - Replace `BoxedSystemSet` and `Box<dyn SystemSet>` with `InternedSystemSet` or `Interned<dyn SystemSet>`. - Replace `AppLabelId` with `InternedAppLabel` or `Interned<dyn AppLabel>`. - Types manually implementing `ScheduleLabel`, `AppLabel` or `SystemSet` need to implement: - `dyn_hash` directly instead of implementing `DynHash` - `as_dyn_eq` - Pass labels to `World::try_schedule_scope`, `World::schedule_scope`, `World::try_run_schedule`. `World::run_schedule`, `Schedules::remove`, `Schedules::remove_entry`, `Schedules::contains`, `Schedules::get` and `Schedules::get_mut` by value instead of by reference. --------- Co-authored-by: Joseph <21144246+JoJoJet@users.noreply.github.com> Co-authored-by: Carter Anderson <mcanders1@gmail.com>
Objective
run_if
for SystemConfigs #7659Solution
The idea of anonymous system sets or "implicit hidden organizational sets" was briefly mentioned by @cart here: #7634 (comment).
Schedule::add_systems
creates an implicit, anonymous system set of all systems inSystemConfigs
.SystemConfigs
are now applied to the implicit system set, instead of being applied to each individual system. This should not change the behavior, AFAIU, becausebefore
,after
,run_if
andambiguous_with
are transitive properties from a set to its members.AnonymousSystemSet
stores the names of its members to provide better error messages.AnonymousSystemSet
.AnonymousSystemSet
are not equal, even if they have the same members / member names.add_systems
calls will produce two differentAnonymousSystemSet
s.AnonymousSystemSet
will be equal.Drawbacks
If my assumptions are correct, the observed behavior should stay the same. But the number of system sets in the
Schedule
will increase with eachadd_systems
call. If this has negative performance implications,add_systems
could be changed to only create the implicit system set if necessary / when a run condition was added.