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

[Merged by Bors] - fix re-adding a plugin to a plugin group #2039

Closed
wants to merge 4 commits into from

Conversation

mockersf
Copy link
Member

In a PluginGroupBuilder, when adding a plugin that was already in the group (potentially disabled), it was then added twice to the app builder when calling finish. As the plugin is kept in an HashMap, it is not possible to have the same plugins twice with different configuration.

This PR updates the order of the plugin group so that each plugin is present only once.

@NathanSWard
Copy link
Contributor

NathanSWard commented Apr 28, 2021

I'm curious, is this the best behavior here?
With this implementation we silently succeed by simply removing the duplicated plugin, however, I would argue that having a duplicate plugin is incorrect and we should panic! and notify the user that they are attempting to add in an already existing plugin.

Edit: I could be wrong, and there could be a valid use case, I just can't personally think of one. 😅

@mockersf
Copy link
Member Author

mockersf commented Apr 29, 2021

The error can happen when re-adding a plugin with a new configuration.

To do that you would do:

App::build()
    .add_plugin_group_with(PluginGroup, |group| {
        group.disable::<PluginA>()
             .add_after::< PluginA, _>(PluginA {
                // custom values here
            })
    })

but that didn't work before this PR: it marked PluginA as disabled, then added a new entry in the ordered list and re-enabled it with the new values in the hash map, making it added twice when building the app. This could be worked around by adding a remove::<T: Plugin>() method but then you wouldn't be able to add your new plugin immediately after the previous one in the ordered list.

@mtsr
Copy link
Contributor

mtsr commented Apr 29, 2021

Maybe it could issue a warning when replacing a pluging that is not disabled?

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Core Common functionality for all bevy apps labels Apr 29, 2021
@alice-i-cecile
Copy link
Member

Maybe it could issue a warning when replacing a pluging that is not disabled?

I'm in favor of this behavior if possible.

@mockersf
Copy link
Member Author

added a warn log

@blaind blaind mentioned this pull request May 16, 2021
@cart cart added the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 23, 2021
@mockersf mockersf removed the S-Pre-Relicense This PR was made before Bevy added the Apache license. Cannot be merged or used for other work label Jul 24, 2021
@mockersf mockersf force-pushed the plugin-disable-readd branch 2 times, most recently from 5c6ab13 to 9cd8d0f Compare July 27, 2021 20:59
@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 May 3, 2022
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.

bors r+

bors bot pushed a commit that referenced this pull request May 9, 2022
In a `PluginGroupBuilder`, when adding a plugin that was already in the group (potentially disabled), it was then added twice to the app builder when calling `finish`. As the plugin is kept in an `HashMap`, it is not possible to have the same plugins twice with different configuration.

This PR updates the order of the plugin group so that each plugin is present only once.

Co-authored-by: François <8672791+mockersf@users.noreply.github.com>
@bors bors bot changed the title fix re-adding a plugin to a plugin group [Merged by Bors] - fix re-adding a plugin to a plugin group May 9, 2022
@bors bors bot closed this May 9, 2022
robtfm pushed a commit to robtfm/bevy that referenced this pull request May 10, 2022
In a `PluginGroupBuilder`, when adding a plugin that was already in the group (potentially disabled), it was then added twice to the app builder when calling `finish`. As the plugin is kept in an `HashMap`, it is not possible to have the same plugins twice with different configuration.

This PR updates the order of the plugin group so that each plugin is present only once.

Co-authored-by: François <8672791+mockersf@users.noreply.github.com>
exjam pushed a commit to exjam/bevy that referenced this pull request May 22, 2022
In a `PluginGroupBuilder`, when adding a plugin that was already in the group (potentially disabled), it was then added twice to the app builder when calling `finish`. As the plugin is kept in an `HashMap`, it is not possible to have the same plugins twice with different configuration.

This PR updates the order of the plugin group so that each plugin is present only once.

Co-authored-by: François <8672791+mockersf@users.noreply.github.com>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
In a `PluginGroupBuilder`, when adding a plugin that was already in the group (potentially disabled), it was then added twice to the app builder when calling `finish`. As the plugin is kept in an `HashMap`, it is not possible to have the same plugins twice with different configuration.

This PR updates the order of the plugin group so that each plugin is present only once.

Co-authored-by: François <8672791+mockersf@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Bug An unexpected or incorrect behavior 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
Development

Successfully merging this pull request may close these issues.

6 participants