-
-
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
Add run_system_singleton
#10469
Add run_system_singleton
#10469
Conversation
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
Co-authored-by: Giacomo Stevanato <[email protected]>
Thinking more about the closure thing, I see a problem: closures can have arbitrary internal state (i.e. the values captured in a closure) other than I haven't tested this, but I suspect that this code: let mut world = World::new();
let make_sys = |n| move || println!("{n}");
let _ = world.run_system_singleton(make_sys(0));
let _ = world.run_system_singleton(make_sys(1)); would print Preventing this method from being called with closures like it was before won't completly fix the issue since the closure could be |
@Trashtalk217, review please? <3 |
Very close to my original API and implementation :) I think this is a reasonable set of trade-offs to present to users, but we need to make sure the docs are very clear about the distinction.
I'd be fine with a |
Currently the method will give a |
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.
Feedback at this point is mostly docs related.
IMO we should:
- Cover this in the one-shot systems example.
- Clean up the language on all of the docs for clarity.
- Move the compare/contrast and limitations for all one-shot systems to module level docs, probably under system_registry.rs.
I would also like to see a fair number of tests, covering various type system edge cases.
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 don't forget to run cargo fmt --all
or CI will get angry
I'm skeptical whether or not this is a good addition. While it's easiest to call a system simply by name, it introduces some shortcomings and that leads us to the rather peculiar situation where we have three I'm afraid that this is confusing to users, and when we add input/output (as in #10380), there should probably also be a Also, and this is something actually actionable, can you double-check if you can nest |
I've tested with this: fn count(mut commands: Commands, counter: ResMut<Counter>) {
println!("{}", counter.0);
let _ = commands.run_system_singleton(count_up);
}
fn count_up(mut counter: ResMut<Counter>) {
println!("updated");
counter.0 += 1;
}
let mut world = World::default();
world.insert_resource(Counter(0));
let _ = world.run_system_singleton(count);
let _ = world.run_system_singleton(count);
let _ = world.run_system_singleton(count);
// It prints:
// Creating system map
// 0
// updated
// 1
// updated
// 2
// updated It seems that nothing goes wrong (Not sure if this is enough). |
@alice-i-cecile, I wrote more details on |
9539daf
to
5e8737e
Compare
Is this still needed once #14920 lands? I feel like the linked PR would solve anything that this API could help with. |
Agreed, closing. |
# Objective Working with `World` is painful due to lifetime issues and a lack of ergonomics, so you may want to delegate to the system API. Your current options are: - `world.run_system_once`, which initializes the system each time it's called (performance cost) and doesn't support `Local`. The docs recommend users not use this method outside of diagnostic use cases like unit tests. - `world.run_system`, which requires you to register the system and store the `SystemId` somewhere (made easier by implementing `FromWorld` for a newtyped `Local`, unless you're in e.g. a custom `Command` impl). These options work, but you're choosing between a performance cost and an ergonomic challenge. ## Solution Provide a cached `run_system` API that accepts an `S: IntoSystem` and checks for a `CachedSystemId<S::System>(SystemId)` resource. If it doesn't exist, it will register the system and save its `SystemId` in that resource. In other words, it hides the "save the `SystemId` in a `Local` or `Resource`" pattern as an implementation detail. Prior work: #10469. ## Testing This approach worked in a proof-of-concept: https://github.com/benfrankel/bevy_jam_template/blob/b34ee29531e3ceae287a9cc44ec6c520e83a4cdd/src/util/patch/run_system_cached.rs#L35. A new unit test was added and it passes in CI.
Objective
SystemId
registration for one-shot systems #10382Solution
Changelog
World::run_system_singleton
and corresponding commandCommand::run_system_singleton
.