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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Unique plugin #6411

Closed
wants to merge 8 commits into from
Closed

[Merged by Bors] - Unique plugin #6411

wants to merge 8 commits into from

Conversation

mockersf
Copy link
Member

@mockersf mockersf commented Oct 29, 2022 •
edited

Objective

  • Make it impossible to add a plugin twice
  • This is going to be more a risk for plugins with configurations, to avoid things like App::new().add_plugins(DefaultPlugins).add_plugin(ImagePlugin::default_nearest())

Solution

  • Panic when a plugin is added twice
  • It's still possible to mark a plugin as not unique by overriding is_unique
  • Simpler version of can only add a plugin once #3988 (not simpler anymore because of how PluginGroupBuilder implements PluginGroup)

@alice-i-cecile alice-i-cecile added C-Usability A simple quality-of-life change that makes Bevy easier to use A-App Bevy apps and plugins labels Oct 29, 2022
alice-i-cecile
alice-i-cecile requested changes Oct 29, 2022
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment •
edited

Choose a reason for hiding this comment

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

Nice and simple, very useful. Definitely agree with the decision to make plugins unique by default.

I'd like:

  1. Docs about the behavior of generic plugins.
  2. A should_panic test for duplicate plugins.
  3. A test that allowing duplicates works.
  4. A test that adding generic plugins with different type parameters works.

Looks like #3988 has tests, so steal those :p

@alice-i-cecile alice-i-cecile mentioned this pull request Oct 29, 2022
Closed
alice-i-cecile
alice-i-cecile approved these changes Oct 29, 2022
View reviewed changes
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.

Left some docs for you :) I won't block on them, but do think they should be added.

james7132
james7132 requested changes Oct 29, 2022
View reviewed changes
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Looks great. Some minor changes regarding startup performance and usability.

crates/bevy_app/src/plugin.rs Show resolved Hide resolved
crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved
crates/bevy_app/src/plugin_group.rs Show resolved Hide resolved
@mockersf mockersf mentioned this pull request Oct 30, 2022
Closed
@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 Oct 30, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Oct 31, 2022
@mockersf
Unique plugin (#6411)
8cdd977 
# Objective

- Make it impossible to add a plugin twice
- This is going to be more a risk for plugins with configurations, to avoid things like `App::new().add_plugins(DefaultPlugins).add_plugin(ImagePlugin::default_nearest())`

## Solution

- Panic when a plugin is added twice
- It's still possible to mark a plugin as not unique by overriding `is_unique`
- ~~Simpler version of~~ #3988 (not simpler anymore because of how `PluginGroupBuilder` implements `PluginGroup`)
@bors
Copy link
Contributor

bors bot commented Oct 31, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Unique plugin [Merged by Bors] - Unique plugin Oct 31, 2022
@bors bors bot closed this Oct 31, 2022
@mockersf mockersf added the hacktoberfest-accepted A PR that was accepted for Hacktoberfest, an annual open source event label Oct 31, 2022
@alice-i-cecile alice-i-cecile mentioned this pull request Oct 31, 2022
Closed
@harudagondi harudagondi mentioned this pull request Nov 1, 2022
Closed
@LarsDu
Copy link
Contributor

LarsDu commented Nov 11, 2022 •
edited

This seems to have broken my latest builds:
https://github.com/LarsDu/StarRust/tree/bevy-0.9-unstable

Update:

  • The build broke due to two things:
    • I actually did load the WeaponPlugin twice by accident
    • One of my Plugins was named ScenePlugin which is identical to one of the plugins loaded in DefaultPlugins

Perhaps there should be a followup PR to make it so that you can't name two different plugins the same thing?

@ramirezmike
Copy link
Contributor

One of my Plugins was named ScenePlugin which is identical to one of the plugins loaded in DefaultPlugins

I saw you mention this in the discord and thought that was odd.. Like, if the code is using the type name to identify uniqueness, I would expect it would be the fully qualified name so that you wouldn't get collisions across crates.

It looks like this PR uses the Plugin trait's name function which uses std::any::type_name.. which does seem to output the fully qualified name, so @LarsDu I'm curious if it did actually panic because of ScenePlugin..

Additionally, @mockersf (and maybe @alice-i-cecile?):

  1. the Plugin trait's doc comment says /// Configures a name for the [Plugin] which is _primarily_ used for debugging. which is somewhat less true now, right?
  2. std::any::type_name has several warnings about the uniqueness guarantees of its output and also makes no guarantee that it will stay the same across versions

again, just saw this and thought 🤔 and wanted to make sure this was all OK

@alice-i-cecile
Copy link
Member

Yeah, I think that we should update those docs to reflect its use in this feature.

I'm fine using std::any::type_name for this though, especially since there's multiple escape hatches.

@irate-devil
Copy link
Contributor

I don't like the thought of Bevy or third party crates adding a new Plugin(even a private one!) breaking user code because the name overlaps.
Even worse, imagine 2 different third party crates adding a plugin with the same name, now you can't use them together without one of them changing their code and publishing a new version :(

How about identifying unique plugins by TypeId, or perhaps a combination of TypeId and the name?

@ramirezmike
Copy link
Contributor

@devil-ira

Based on the documentation of std::any::type_name, I don't think this happens. It looks like it outputs the fully qualified name (for now :eyes:). Your concern is exactly why I brought it up because @LarsDu said this happened when they had a plugin named ScenePlugin in their user code. I think we'll want to wait until they confirm that's actually what happened.

@mockersf
Copy link
Member Author

Bevy's own ScenePlugin is registered as bevy_scene::ScenePlugin

ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@mockersf @ItsDoot
Unique plugin (bevyengine#6411)
4bc3b9b 
# Objective

- Make it impossible to add a plugin twice
- This is going to be more a risk for plugins with configurations, to avoid things like `App::new().add_plugins(DefaultPlugins).add_plugin(ImagePlugin::default_nearest())`

## Solution

- Panic when a plugin is added twice
- It's still possible to mark a plugin as not unique by overriding `is_unique`
- ~~Simpler version of~~ bevyengine#3988 (not simpler anymore because of how `PluginGroupBuilder` implements `PluginGroup`)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@james7132 james7132 james7132 approved these changes

Assignees
No one assigned
Labels
A-App Bevy apps and plugins C-Usability A simple quality-of-life change that makes Bevy easier to use hacktoberfest-accepted A PR that was accepted for Hacktoberfest, an annual open source event S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@mockersf @alice-i-cecile @LarsDu @ramirezmike @irate-devil @james7132