Skip to content

Allow tuples in add_plugins.#7687

Closed
geieredgar wants to merge 12 commits intobevyengine:mainfrom
geieredgar:add
Closed

Allow tuples in add_plugins.#7687
geieredgar wants to merge 12 commits intobevyengine:mainfrom
geieredgar:add

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 targeted quality-of-life change that makes Bevy easier to use X-Needs-SME This type of work requires an SME to approve it. 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 targeted quality-of-life change that makes Bevy easier to use X-Needs-SME This type of work requires an SME to approve it.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants