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

Allow tuples in add_plugins. #7687

Closed
wants to merge 12 commits into from
Closed

Conversation

geieredgar
Copy link
Contributor

@geieredgar geieredgar commented Feb 15, 2023

Objective

This is just a small idea of mine about a possible unified API.
(see #7687 (comment))

Solution

(see #7687 (comment))
Add a single add method to which a Plugin, a PluginGroup or a FnOnce(&mut App) can be passed. Plugins that need resources to be inserted before they can be built (because they don't want to use an ugly hack like Arc<Mutex<Option<T>>>) can now provide an constructor that returns an FnOnce which takes care of that (see logger_example).

Pros:

  • Can pass tuples to add_plugins.
  • Added Flexibility: Application code can easily and ergonomically split up setup code into functions that take an &mut App parameter.
  • Users don't have to worry about the difference of add_plugin vs add_plugins
  • Shorter to write: app.add_plugins(DefaultPlugins) vs app.add(DefaultPlugins). The type names already make clear that Plugins are added.

Cons:

  • Redundancy on the API level. This can be solved by removing add_plugin and add_plugins or reducing their visibility.
  • Probably harder to write docs.

@alice-i-cecile alice-i-cecile added A-App Bevy apps and plugins C-Usability A simple quality-of-life change that makes Bevy easier to use X-Controversial There is active debate or serious implications around merging this PR labels Feb 15, 2023
@alice-i-cecile
Copy link
Member

Hmm. Very interesting, but I think ultimately a regression. It's a lot harder for users to understand exactly what they need to pass in, or what existing code is doing.

@geieredgar
Copy link
Contributor Author

Okay, I have changed the code to make add_plugin and add_plugins work similar to add_system / add_systems.

add_plugin now accepts an argument that implements the sealed IntoPlugin trait, which is implemented for Plugin and FnOnce(&mut App) -> Plugin.
add_plugins now accepts an argument that implements the sealed IntoPluginGroup trait, which is implemented for PluginGroup, FnOnce(&mut App) -> PluginGroup and tuples containing Plugins PluginGroups and FnOnces.

This does not any longer fix the problem in #7631, but would still help implementing #7682.

@geieredgar geieredgar changed the title Add add method to App ~~Add add method to App~~ Allow tuples in add_plugins. Feb 15, 2023
@geieredgar geieredgar changed the title ~~Add add method to App~~ Allow tuples in add_plugins. Allow tuples in add_plugins. [Was formerly: Add add method to App] Feb 15, 2023
@alice-i-cecile
Copy link
Member

Definitely like that better!

@geieredgar geieredgar changed the title Allow tuples in add_plugins. [Was formerly: Add add method to App] Allow tuples in add_plugins. Feb 15, 2023
@geieredgar
Copy link
Contributor Author

I have moved the traits into their corresponding module and added additional tests and documentations for the traits.

@geieredgar
Copy link
Contributor Author

Superseded by #8097.

@geieredgar geieredgar closed this Mar 15, 2023
@johanhelsing
Copy link
Contributor

As far as i understand, the other pr will not help with the ownership issues i ran into in #7682, though... Is there another solution for that?

@geieredgar
Copy link
Contributor Author

I didn't include it because it felt somewhat hacky to solve the problem in this way, I believe the better approach would be to change Plugin::build to take self instead of &self. I will open an PR for that.

@geieredgar
Copy link
Contributor Author

Making Plugin::build take self is unfortunately not possible without a lot of refactoring / breaking existing users, because of App::get_added_plugins. Therefore I've decided to only change Plugin::build to take &mut self, which would at least allow us to get rid of the Arc<Mutex<_>> part. PR: #8101.

@johanhelsing
Copy link
Contributor

Alright, thanks for looking into it :) Sounds like a decent compromise.

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 C-Usability A simple quality-of-life change that makes Bevy easier to use 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.

3 participants