Skip to content
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

Closed
wants to merge 5 commits into from
Closed

Conversation

hxYuki
Copy link
Contributor

@hxYuki hxYuki commented Nov 9, 2023

Objective

Solution

  • Add a new method that runs systems directly and keeps their states indexed by names.

Changelog

  • Added World::run_system_singleton and corresponding command Command::run_system_singleton.

Copy link
Contributor

github-actions bot commented Nov 9, 2023

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

crates/bevy_ecs/src/system/system_registry.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_registry.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_registry.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_registry.rs Outdated Show resolved Hide resolved
@SkiFire13
Copy link
Contributor

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 Locals. However the very first system is the one cached in the SystemMap and the one which gets repeatedly called, meaning the state of new systems gets ignored.

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 0 0 instead of 0 1 due the make_sys(0) being cached and called instead of the make_sys(1).

Preventing this method from being called with closures like it was before won't completly fix the issue since the closure could be piped/mapped, in which case it would not be distinguishable from other systems (and also you could manually implement a system as if it was a closure). Requiring std::mem::size_of::<T>() == 0 should work, but it may also rule out legit usecases.

@alice-i-cecile
Copy link
Member

@Trashtalk217, review please? <3

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events D-Complex Quite challenging from either a design or technical perspective. Ask for help! C-Usability A targeted quality-of-life change that makes Bevy easier to use labels Nov 9, 2023
@alice-i-cecile alice-i-cecile added this to the 0.13 milestone Nov 9, 2023
@alice-i-cecile
Copy link
Member

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.

Requiring std::mem::size_of::() == 0 should work, but it may also rule out legit usecases

I'd be fine with a info! level warning for this?

@hxYuki
Copy link
Contributor Author

hxYuki commented Nov 10, 2023

Currently the method will give a info! warning for closure with captures.

Copy link
Member

@alice-i-cecile alice-i-cecile left a 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:

  1. Cover this in the one-shot systems example.
  2. Clean up the language on all of the docs for clarity.
  3. 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.

Copy link
Contributor

@SkiFire13 SkiFire13 left a 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

crates/bevy_ecs/src/system/system_registry.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system_registry.rs Outdated Show resolved Hide resolved
@Trashtalk217
Copy link
Contributor

Trashtalk217 commented Nov 10, 2023

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 run_system methods that all behave slightly differently and have different shortcomings.

I'm afraid that this is confusing to users, and when we add input/output (as in #10380), there should probably also be a run_system_singleton_with_input and overall, I don't like that API very much; where you have 6 or so slightly different methods that look the same but are in some subtle way slightly different.

Also, and this is something actually actionable, can you double-check if you can nest run_system_singleton? As I look at the code, I'm pretty sure that'll break something (maybe spawn two SystemMap resources?).

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Nov 10, 2023
@hxYuki
Copy link
Contributor Author

hxYuki commented Nov 12, 2023

Also, and this is something actually actionable, can you double-check if you can nest run_system_singleton? As I look at the code, I'm pretty sure that'll break something (maybe spawn two SystemMap resources?).

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).

@hxYuki
Copy link
Contributor Author

hxYuki commented Nov 12, 2023

@alice-i-cecile, I wrote more details on World::run_system_singleton. No sure how much should it be on Commands::run_system_singleton.

@alice-i-cecile alice-i-cecile removed this from the 0.13 milestone Jan 24, 2024
@janhohenheim
Copy link
Member

Is this still needed once #14920 lands? I feel like the linked PR would solve anything that this API could help with.

@alice-i-cecile
Copy link
Member

Agreed, closing.

github-merge-queue bot pushed a commit that referenced this pull request Sep 23, 2024
# 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.
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-Usability A targeted quality-of-life change that makes Bevy easier to use D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add one default SystemId registration for one-shot systems
5 participants