Skip to content

make it easier to change app to single threaded stages #5966

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

Closed
wants to merge 9 commits into from

Conversation

hymm
Copy link
Contributor

@hymm hymm commented Sep 12, 2022

Objective

Solution

  • change add_default_stages to take an option to pick threading model of the default stages
  • added constructors on App, App::single_threaded and App::multithreaded that allow users to construct an app with default stages as single threaded or multi threaded.

Migration Guide

add_default_stages now takes an option.

Before

App::empty().add_default_stages();

After

App::empty().add_default_stages(AppThreading::Multi);

Notes

  • Not sure if App::new_with_threading_options should just replace App::new(). It would mean we'd need to change all the examples to use App::default() instead of new added separate constructors with the option set instead

Copy link
Contributor

@nicopap nicopap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think get_stage should be defined somewhere else). It could very well replace App::new, since, after all, it's in general used only once in a game, which I don't think is too burdensome to update. Though a deprecation warning period + a migration guide pointing out App::default would be necessary.

}

app
App::new_with_threading_option(AppThreading::Multi)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Idea: What about adding a Default to AppThreading and calling AppThreading::default() here? Just a suggestion

@nicopap
Copy link
Contributor

nicopap commented Sep 12, 2022

Also I had no idea add_default_stages was a thing. :D

@IceSentry IceSentry added A-App Bevy apps and plugins M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Sep 12, 2022
@hymm
Copy link
Contributor Author

hymm commented Sep 13, 2022

I added constructors for App::single_threaded() and App::multi_threaded() and made App::new_with_threading_options private, instead of adding an option to new(). Not sure about the naming here, since you can still use query.par_for_each and the task pools to do multi threading.

@@ -114,6 +119,33 @@ impl App {
}
}

/// Creates a new [`App`] with some default structure with all default stages set to use single threaded system executor.
pub fn single_threaded() -> App {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think ideally this is an "app setting", which we use for all app stages (by default). Ex: replace instances of add_stage(Label, stage_reference) with add_default_stage(Label). And with stageless on the horizon, this would need to be a global app setting anyway (ex: your only options are to use a single threaded app executor or a multithreaded app executor).
What are your thoughts on waiting for stageless before defining this api vs building this api (with stageless in mind)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm fine with closing this out until after stageless.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

And with stageless on the horizon, this would need to be a global app setting anyway

This could be a per schedule setting in stageless.

@hymm hymm closed this Oct 3, 2022
@hymm hymm deleted the single-threaded-app branch October 5, 2023 17:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants