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 and single plugins in add_plugins, deprecate add_plugin #8097

Merged
merged 3 commits into from
Jun 21, 2023

Conversation

geieredgar
Copy link
Contributor

@geieredgar geieredgar commented Mar 15, 2023

Objective

  • Better consistency with add_systems.
  • Deprecating add_plugin in favor of a more powerful add_plugins.
  • Allow passing Plugin to add_plugins.
  • Allow passing tuples to add_plugins.

Solution

  • App::add_plugins now takes an impl Plugins parameter.
  • App::add_plugin is deprecated.
  • Plugins is a new sealed trait that is only implemented for Plugin, PluginGroup and tuples over Plugins.
  • All examples, benchmarks and tests are changed to use add_plugins, using tuples where appropriate.

Changelog

Changed

  • App::add_plugins now accepts all types that implement Plugins, which is implemented for:
    • Types that implement Plugin.
    • Types that implement PluginGroup.
    • Tuples (up to 16 elements) over types that implement Plugins.
  • Deprecated App::add_plugin in favor of App::add_plugins.

Migration Guide

  • Replace app.add_plugin(plugin) calls with app.add_plugins(plugin).

@alice-i-cecile alice-i-cecile added A-App Bevy apps and plugins X-Controversial There is active debate or serious implications around merging this PR labels Mar 15, 2023
@alice-i-cecile alice-i-cecile added this to the 0.11 milestone Mar 15, 2023
@geieredgar geieredgar force-pushed the add_plugins branch 2 times, most recently from ea208f9 to c100d07 Compare March 18, 2023 12:52
@geieredgar
Copy link
Contributor Author

I've rebased this on main after #8079 was merged. I think this is ready for final reviews.

@geieredgar geieredgar changed the title Allow tuples and single plugins in add_plugins, remove add_plugin Allow tuples and single plugins in add_plugins, deprecate add_plugin Mar 18, 2023
@geieredgar
Copy link
Contributor Author

To be consistent with #8079, I've changed this PR to only deprecate add_plugin instead of removing it.

@alice-i-cecile
Copy link
Member

@geieredgar I like this PR's direction and I think we should make a call for 0.11. Can you rebase please?

@geieredgar
Copy link
Contributor Author

@alice-i-cecile I've rebased the PR. I also renamed the marker types and type parameters to be more consistent with similar traits like Condition or IntoSystemConfigs.

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.

Well-made, and I think there's an argument for this. I don't have strong feelings either way on the change, so I'll defer to Cart et al on that.

@B-Reif
Copy link
Contributor

B-Reif commented Jun 20, 2023

I’m lightly opposed to this sort of thing for the same reasons I brought up in the PR for add_resources, #8126.

To reiterate:

  • the add_systems API is pluralized because, in that context, the tuples can specify additional behavior.
  • A tuple-of-plugins (or resources) doesn’t add functionality.
  • At worst, making other builder methods analogous to add_systems suggests some grouping functionality that doesn’t exist.

@cart
Copy link
Member

cart commented Jun 20, 2023

@B-Reif

the add_systems API is pluralized because, in that context, the tuples can specify additional behavior.
A tuple-of-plugins (or resources) doesn’t add functionality.

One notable difference. Unlike resources, which do not currently have a plural version of the api and are generally thought of as a classic rust "key->value" API, Plugins already have the add_plugins API. I see supporting tuples there as a natural extension of the api. I also consider it Bevy-like ... given that we like using tuples for groups of things elsewhere (ex: Bundles and Systems). I think about plugin tuples as "anonymous PluginGroups" that save boilerplate.

From my perspective the only controversial part of this is deprecating add_plugin. And imo at that point the consistency with add_systems and the removal of redundant paths is "worth it".

At worst, making other builder methods analogous to add_systems suggests some grouping functionality that doesn’t exist.

I'll argue that most of the instances of "merged plugin adds" in this PR do have "groupings" of functionality. They are either called from inside of a parent Plugin, where the "grouping scope" is "Plugins used by the Parent Plugin to accomplish its goal". Or they are called in the context of a main function, where the grouping scope is "plugins used by this Bevy App".

In cases where logical groups are desired, devs can still make multiple calls. Or if they prefer, they can use comments within the tuple to denote organizational sections.

@cart
Copy link
Member

cart commented Jun 20, 2023

I'll leave this open for a day-ish to leave room for a bit more discussion. But I'm inclined to merge.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 20, 2023
@inodentry
Copy link
Contributor

I am in favor. I agree with cart.

I was initially in favor for resources too, but I changed my mind on that one, because resources are consistently treated as singular standalone items semantically everywhere in bevy. Their intention is to be standalone pieces of data. Like cart said, the "key->value" feel. But plugins are not.

We clearly need a way to add multiple plugins at once (add_plugins has existed forever, with good reason). Currently, the only way to do that is by creating these special PluginGroup things, which IMO makes the API inaccessible, as a lot of people wouldn't bother with the boilerplate. It's like if we only supported Bundle structs, but not anonymous tuples. It feels like an omission in UX.

Also reducing the number of variants (unique methods) in the App API is a good thing. Having both add_plugin and add_plugins feels redundant.

@B-Reif
Copy link
Contributor

B-Reif commented Jun 20, 2023

I'll argue that most of the instances of "merged plugin adds" in this PR do have "groupings" of functionality.

I was talking about the tuple itself having some observable difference from multiple builder calls (like the traits implemented on system tuples).

I hadn’t considered we already need this in the form of PluginGroup or other abstractions, and this obviously feels better than that so I’m in favor now.

@JoJoJet JoJoJet added the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Jun 21, 2023
@cart cart added this pull request to the merge queue Jun 21, 2023
Merged via the queue into bevyengine:main with commit f18f288 Jun 21, 2023
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it 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.

7 participants