Plugin - setup resource #2988
Closed
Plugin - setup resource #2988
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mockersf
added
A-App
alice-i-cecile
added
S-Needs-Review
and removed
S-Needs-Triage
|
Improves the situation of #1255. |
|
I like it! The concept of a startup resource is pretty simple, and it pushes us further towards a safer, more structured plugin API. The duplicate detection is much more powerful than #2886; I feel pretty strongly that this should be merged instead. I left some code quality nits, but once those are cleaned up it LGTM. |
NiklasEi
reviewed
Oct 18, 2021
There was a problem hiding this comment.
Is "every plugin can only exist once" really something we want to enforce for all plugins? This would for example break bevy_asset_loader and bevy_loading. Both plugins need to be configured with two states on a per instance basis. I don't see a way to do that with generics (without having enum variants as types).
In the above examples I like the split of different loading states into different plugin instances much better than trying to put configuration for multiple states into one instance. It would be nice to have a way of opting out of the plugin uniqueness check. Maybe we could extend the plugin trait?
NiklasEi
reviewed
Oct 18, 2021
|
I renamed "startup" to "initialization" as the resources won't be available for startup systems, to avoid confusion.
No, but I wasn't aware of a use case where that would be needed, thanks for bringing it up! I added a way for the plugin author to say their plugin can be added several times This is also a more general fix for #2983 as it won't be possible to add |
IceSentry
reviewed
Oct 18, 2021
IceSentry
reviewed
Oct 18, 2021
IceSentry
approved these changes
Oct 18, 2021
alice-i-cecile
approved these changes
Oct 18, 2021
mirkoRainer
reviewed
Oct 18, 2021
|
☝️ I think this change is worth mentioning in the migration guide. Not only because it can cause panics in older Apps that add a plugin twice, but also because plugin authors should be aware of this change and potentially opt-out. |
NiklasEi
approved these changes
Oct 18, 2021
NiklasEi
added
S-Ready-For-Final-Review
mockersf
force-pushed
the
plugin-initialization
branch
from
ab701b3
to
09ffd79
Compare
mockersf
force-pushed
the
plugin-initialization
branch
from
392839b
to
0547a84
Compare
mockersf
force-pushed
the
plugin-initialization
branch
from
f5f44e1
to
8d9d44b
Compare
|
Can you update the name of this PR to match then? |
mockersf
changed the title
|
@mockersf can you rebase and add a changelog entry? |
mockersf
force-pushed
the
plugin-initialization
branch
from
665a748
to
9613157
Compare
alice-i-cecile
added
the
X-Controversial
Closed
|
I have left some comments, but I want to put a new one here: |
You don't, that's pretty much the point |
mockersf
force-pushed
the
plugin-initialization
branch
2 times, most recently
from
d5610ec
to
b151ec8
Compare
|
@alice-i-cecile I rebased this PR, it should be ready if you want to start the controversial timer |
|
Added |
mockersf
force-pushed
the
plugin-initialization
branch
from
f9e0aa2
to
8af2460
Compare
mockersf
force-pushed
the
plugin-initialization
branch
from
8af2460
to
0d8404d
Compare
bors bot
pushed a commit
that referenced
this pull request
Oct 24, 2022
# Objective Fixes #5884 #2879 Alternative to #2988 #5885 #2886 "Immutable" Plugin settings are currently represented as normal ECS resources, which are read as part of plugin init. This presents a number of problems: 1. If a user inserts the plugin settings resource after the plugin is initialized, it will be silently ignored (and use the defaults instead) 2. Users can modify the plugin settings resource after the plugin has been initialized. This creates a false sense of control over settings that can no longer be changed. (1) and (2) are especially problematic and confusing for the `WindowDescriptor` resource, but this is a general problem. ## Solution Immutable Plugin settings now live on each Plugin struct (ex: `WindowPlugin`). PluginGroups have been reworked to support overriding plugin values. This also removes the need for the `add_plugins_with` api, as the `add_plugins` api can use the builder pattern directly. Settings that can be used at runtime continue to be represented as ECS resources. Plugins are now configured like this: ```rust app.add_plugin(AssetPlugin { watch_for_changes: true, ..default() }) ``` PluginGroups are now configured like this: ```rust app.add_plugins(DefaultPlugins .set(AssetPlugin { watch_for_changes: true, ..default() }) ) ``` This is an alternative to #2988, which is similar. But I personally prefer this solution for a couple of reasons: * ~~#2988 doesn't solve (1)~~ #2988 does solve (1) and will panic in that case. I was wrong! * This PR directly ties plugin settings to Plugin types in a 1:1 relationship, rather than a loose "setup resource" <-> plugin coupling (where the setup resource is consumed by the first plugin that uses it). * I'm not a huge fan of overloading the ECS resource concept and implementation for something that has very different use cases and constraints.
bors bot
pushed a commit
that referenced
this pull request
Oct 24, 2022
# Objective Fixes #5884 #2879 Alternative to #2988 #5885 #2886 "Immutable" Plugin settings are currently represented as normal ECS resources, which are read as part of plugin init. This presents a number of problems: 1. If a user inserts the plugin settings resource after the plugin is initialized, it will be silently ignored (and use the defaults instead) 2. Users can modify the plugin settings resource after the plugin has been initialized. This creates a false sense of control over settings that can no longer be changed. (1) and (2) are especially problematic and confusing for the `WindowDescriptor` resource, but this is a general problem. ## Solution Immutable Plugin settings now live on each Plugin struct (ex: `WindowPlugin`). PluginGroups have been reworked to support overriding plugin values. This also removes the need for the `add_plugins_with` api, as the `add_plugins` api can use the builder pattern directly. Settings that can be used at runtime continue to be represented as ECS resources. Plugins are now configured like this: ```rust app.add_plugin(AssetPlugin { watch_for_changes: true, ..default() }) ``` PluginGroups are now configured like this: ```rust app.add_plugins(DefaultPlugins .set(AssetPlugin { watch_for_changes: true, ..default() }) ) ``` This is an alternative to #2988, which is similar. But I personally prefer this solution for a couple of reasons: * ~~#2988 doesn't solve (1)~~ #2988 does solve (1) and will panic in that case. I was wrong! * This PR directly ties plugin settings to Plugin types in a 1:1 relationship, rather than a loose "setup resource" <-> plugin coupling (where the setup resource is consumed by the first plugin that uses it). * I'm not a huge fan of overloading the ECS resource concept and implementation for something that has very different use cases and constraints. ## Changelog - PluginGroups can now be configured directly using the builder pattern. Individual plugin values can be overridden by using `plugin_group.set(SomePlugin {})`, which enables overriding default plugin values. - `WindowDescriptor` plugin settings have been moved to `WindowPlugin` and `AssetServerSettings` have been moved to `AssetPlugin` - `app.add_plugins_with` has been replaced by using `add_plugins` with the builder pattern. ## Migration Guide The `WindowDescriptor` settings have been moved from a resource to `WindowPlugin::window`: ```rust // Old (Bevy 0.8) app .insert_resource(WindowDescriptor { width: 400.0, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(WindowPlugin { window: WindowDescriptor { width: 400.0, ..default() }, ..default() })) ``` The `AssetServerSettings` resource has been removed in favor of direct `AssetPlugin` configuration: ```rust // Old (Bevy 0.8) app .insert_resource(AssetServerSettings { watch_for_changes: true, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(AssetPlugin { watch_for_changes: true, ..default() })) ``` `add_plugins_with` has been replaced by `add_plugins` in combination with the builder pattern: ```rust // Old (Bevy 0.8) app.add_plugins_with(DefaultPlugins, |group| group.disable::<AssetPlugin>()); // New (Bevy 0.9) app.add_plugins(DefaultPlugins.build().disable::<AssetPlugin>()); ```
bors bot
pushed a commit
that referenced
this pull request
Oct 24, 2022
# Objective Fixes #5884 #2879 Alternative to #2988 #5885 #2886 "Immutable" Plugin settings are currently represented as normal ECS resources, which are read as part of plugin init. This presents a number of problems: 1. If a user inserts the plugin settings resource after the plugin is initialized, it will be silently ignored (and use the defaults instead) 2. Users can modify the plugin settings resource after the plugin has been initialized. This creates a false sense of control over settings that can no longer be changed. (1) and (2) are especially problematic and confusing for the `WindowDescriptor` resource, but this is a general problem. ## Solution Immutable Plugin settings now live on each Plugin struct (ex: `WindowPlugin`). PluginGroups have been reworked to support overriding plugin values. This also removes the need for the `add_plugins_with` api, as the `add_plugins` api can use the builder pattern directly. Settings that can be used at runtime continue to be represented as ECS resources. Plugins are now configured like this: ```rust app.add_plugin(AssetPlugin { watch_for_changes: true, ..default() }) ``` PluginGroups are now configured like this: ```rust app.add_plugins(DefaultPlugins .set(AssetPlugin { watch_for_changes: true, ..default() }) ) ``` This is an alternative to #2988, which is similar. But I personally prefer this solution for a couple of reasons: * ~~#2988 doesn't solve (1)~~ #2988 does solve (1) and will panic in that case. I was wrong! * This PR directly ties plugin settings to Plugin types in a 1:1 relationship, rather than a loose "setup resource" <-> plugin coupling (where the setup resource is consumed by the first plugin that uses it). * I'm not a huge fan of overloading the ECS resource concept and implementation for something that has very different use cases and constraints. ## Changelog - PluginGroups can now be configured directly using the builder pattern. Individual plugin values can be overridden by using `plugin_group.set(SomePlugin {})`, which enables overriding default plugin values. - `WindowDescriptor` plugin settings have been moved to `WindowPlugin` and `AssetServerSettings` have been moved to `AssetPlugin` - `app.add_plugins_with` has been replaced by using `add_plugins` with the builder pattern. ## Migration Guide The `WindowDescriptor` settings have been moved from a resource to `WindowPlugin::window`: ```rust // Old (Bevy 0.8) app .insert_resource(WindowDescriptor { width: 400.0, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(WindowPlugin { window: WindowDescriptor { width: 400.0, ..default() }, ..default() })) ``` The `AssetServerSettings` resource has been removed in favor of direct `AssetPlugin` configuration: ```rust // Old (Bevy 0.8) app .insert_resource(AssetServerSettings { watch_for_changes: true, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(AssetPlugin { watch_for_changes: true, ..default() })) ``` `add_plugins_with` has been replaced by `add_plugins` in combination with the builder pattern: ```rust // Old (Bevy 0.8) app.add_plugins_with(DefaultPlugins, |group| group.disable::<AssetPlugin>()); // New (Bevy 0.9) app.add_plugins(DefaultPlugins.build().disable::<AssetPlugin>()); ```
|
Closing in favor of #6336 |
james7132
pushed a commit
to james7132/bevy
that referenced
this pull request
Oct 28, 2022
# Objective Fixes bevyengine#5884 bevyengine#2879 Alternative to bevyengine#2988 bevyengine#5885 bevyengine#2886 "Immutable" Plugin settings are currently represented as normal ECS resources, which are read as part of plugin init. This presents a number of problems: 1. If a user inserts the plugin settings resource after the plugin is initialized, it will be silently ignored (and use the defaults instead) 2. Users can modify the plugin settings resource after the plugin has been initialized. This creates a false sense of control over settings that can no longer be changed. (1) and (2) are especially problematic and confusing for the `WindowDescriptor` resource, but this is a general problem. ## Solution Immutable Plugin settings now live on each Plugin struct (ex: `WindowPlugin`). PluginGroups have been reworked to support overriding plugin values. This also removes the need for the `add_plugins_with` api, as the `add_plugins` api can use the builder pattern directly. Settings that can be used at runtime continue to be represented as ECS resources. Plugins are now configured like this: ```rust app.add_plugin(AssetPlugin { watch_for_changes: true, ..default() }) ``` PluginGroups are now configured like this: ```rust app.add_plugins(DefaultPlugins .set(AssetPlugin { watch_for_changes: true, ..default() }) ) ``` This is an alternative to bevyengine#2988, which is similar. But I personally prefer this solution for a couple of reasons: * ~~bevyengine#2988 doesn't solve (1)~~ bevyengine#2988 does solve (1) and will panic in that case. I was wrong! * This PR directly ties plugin settings to Plugin types in a 1:1 relationship, rather than a loose "setup resource" <-> plugin coupling (where the setup resource is consumed by the first plugin that uses it). * I'm not a huge fan of overloading the ECS resource concept and implementation for something that has very different use cases and constraints. ## Changelog - PluginGroups can now be configured directly using the builder pattern. Individual plugin values can be overridden by using `plugin_group.set(SomePlugin {})`, which enables overriding default plugin values. - `WindowDescriptor` plugin settings have been moved to `WindowPlugin` and `AssetServerSettings` have been moved to `AssetPlugin` - `app.add_plugins_with` has been replaced by using `add_plugins` with the builder pattern. ## Migration Guide The `WindowDescriptor` settings have been moved from a resource to `WindowPlugin::window`: ```rust // Old (Bevy 0.8) app .insert_resource(WindowDescriptor { width: 400.0, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(WindowPlugin { window: WindowDescriptor { width: 400.0, ..default() }, ..default() })) ``` The `AssetServerSettings` resource has been removed in favor of direct `AssetPlugin` configuration: ```rust // Old (Bevy 0.8) app .insert_resource(AssetServerSettings { watch_for_changes: true, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(AssetPlugin { watch_for_changes: true, ..default() })) ``` `add_plugins_with` has been replaced by `add_plugins` in combination with the builder pattern: ```rust // Old (Bevy 0.8) app.add_plugins_with(DefaultPlugins, |group| group.disable::<AssetPlugin>()); // New (Bevy 0.9) app.add_plugins(DefaultPlugins.build().disable::<AssetPlugin>()); ```
Pietrek14
pushed a commit
to Pietrek14/bevy
that referenced
this pull request
Dec 17, 2022
# Objective Fixes bevyengine#5884 bevyengine#2879 Alternative to bevyengine#2988 bevyengine#5885 bevyengine#2886 "Immutable" Plugin settings are currently represented as normal ECS resources, which are read as part of plugin init. This presents a number of problems: 1. If a user inserts the plugin settings resource after the plugin is initialized, it will be silently ignored (and use the defaults instead) 2. Users can modify the plugin settings resource after the plugin has been initialized. This creates a false sense of control over settings that can no longer be changed. (1) and (2) are especially problematic and confusing for the `WindowDescriptor` resource, but this is a general problem. ## Solution Immutable Plugin settings now live on each Plugin struct (ex: `WindowPlugin`). PluginGroups have been reworked to support overriding plugin values. This also removes the need for the `add_plugins_with` api, as the `add_plugins` api can use the builder pattern directly. Settings that can be used at runtime continue to be represented as ECS resources. Plugins are now configured like this: ```rust app.add_plugin(AssetPlugin { watch_for_changes: true, ..default() }) ``` PluginGroups are now configured like this: ```rust app.add_plugins(DefaultPlugins .set(AssetPlugin { watch_for_changes: true, ..default() }) ) ``` This is an alternative to bevyengine#2988, which is similar. But I personally prefer this solution for a couple of reasons: * ~~bevyengine#2988 doesn't solve (1)~~ bevyengine#2988 does solve (1) and will panic in that case. I was wrong! * This PR directly ties plugin settings to Plugin types in a 1:1 relationship, rather than a loose "setup resource" <-> plugin coupling (where the setup resource is consumed by the first plugin that uses it). * I'm not a huge fan of overloading the ECS resource concept and implementation for something that has very different use cases and constraints. ## Changelog - PluginGroups can now be configured directly using the builder pattern. Individual plugin values can be overridden by using `plugin_group.set(SomePlugin {})`, which enables overriding default plugin values. - `WindowDescriptor` plugin settings have been moved to `WindowPlugin` and `AssetServerSettings` have been moved to `AssetPlugin` - `app.add_plugins_with` has been replaced by using `add_plugins` with the builder pattern. ## Migration Guide The `WindowDescriptor` settings have been moved from a resource to `WindowPlugin::window`: ```rust // Old (Bevy 0.8) app .insert_resource(WindowDescriptor { width: 400.0, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(WindowPlugin { window: WindowDescriptor { width: 400.0, ..default() }, ..default() })) ``` The `AssetServerSettings` resource has been removed in favor of direct `AssetPlugin` configuration: ```rust // Old (Bevy 0.8) app .insert_resource(AssetServerSettings { watch_for_changes: true, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(AssetPlugin { watch_for_changes: true, ..default() })) ``` `add_plugins_with` has been replaced by `add_plugins` in combination with the builder pattern: ```rust // Old (Bevy 0.8) app.add_plugins_with(DefaultPlugins, |group| group.disable::<AssetPlugin>()); // New (Bevy 0.9) app.add_plugins(DefaultPlugins.build().disable::<AssetPlugin>()); ```
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
# Objective Fixes bevyengine#5884 bevyengine#2879 Alternative to bevyengine#2988 bevyengine#5885 bevyengine#2886 "Immutable" Plugin settings are currently represented as normal ECS resources, which are read as part of plugin init. This presents a number of problems: 1. If a user inserts the plugin settings resource after the plugin is initialized, it will be silently ignored (and use the defaults instead) 2. Users can modify the plugin settings resource after the plugin has been initialized. This creates a false sense of control over settings that can no longer be changed. (1) and (2) are especially problematic and confusing for the `WindowDescriptor` resource, but this is a general problem. ## Solution Immutable Plugin settings now live on each Plugin struct (ex: `WindowPlugin`). PluginGroups have been reworked to support overriding plugin values. This also removes the need for the `add_plugins_with` api, as the `add_plugins` api can use the builder pattern directly. Settings that can be used at runtime continue to be represented as ECS resources. Plugins are now configured like this: ```rust app.add_plugin(AssetPlugin { watch_for_changes: true, ..default() }) ``` PluginGroups are now configured like this: ```rust app.add_plugins(DefaultPlugins .set(AssetPlugin { watch_for_changes: true, ..default() }) ) ``` This is an alternative to bevyengine#2988, which is similar. But I personally prefer this solution for a couple of reasons: * ~~bevyengine#2988 doesn't solve (1)~~ bevyengine#2988 does solve (1) and will panic in that case. I was wrong! * This PR directly ties plugin settings to Plugin types in a 1:1 relationship, rather than a loose "setup resource" <-> plugin coupling (where the setup resource is consumed by the first plugin that uses it). * I'm not a huge fan of overloading the ECS resource concept and implementation for something that has very different use cases and constraints. ## Changelog - PluginGroups can now be configured directly using the builder pattern. Individual plugin values can be overridden by using `plugin_group.set(SomePlugin {})`, which enables overriding default plugin values. - `WindowDescriptor` plugin settings have been moved to `WindowPlugin` and `AssetServerSettings` have been moved to `AssetPlugin` - `app.add_plugins_with` has been replaced by using `add_plugins` with the builder pattern. ## Migration Guide The `WindowDescriptor` settings have been moved from a resource to `WindowPlugin::window`: ```rust // Old (Bevy 0.8) app .insert_resource(WindowDescriptor { width: 400.0, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(WindowPlugin { window: WindowDescriptor { width: 400.0, ..default() }, ..default() })) ``` The `AssetServerSettings` resource has been removed in favor of direct `AssetPlugin` configuration: ```rust // Old (Bevy 0.8) app .insert_resource(AssetServerSettings { watch_for_changes: true, ..default() }) .add_plugins(DefaultPlugins) // New (Bevy 0.9) app.add_plugins(DefaultPlugins.set(AssetPlugin { watch_for_changes: true, ..default() })) ``` `add_plugins_with` has been replaced by `add_plugins` in combination with the builder pattern: ```rust // Old (Bevy 0.8) app.add_plugins_with(DefaultPlugins, |group| group.disable::<AssetPlugin>()); // New (Bevy 0.9) app.add_plugins(DefaultPlugins.build().disable::<AssetPlugin>()); ```
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Objective
Solution
allow the same plugin to be added only once to an application (can be changed by the plugin author)moved to can only add a plugin once #3988LogSettings,WindowDescriptor,AssetServerSettings,DefaultTaskPoolOptions,ImageSettingsto setup resourcesfixes #2879 and replace #2886
Changelog
LogSettings,WindowDescriptor,AssetServerSettings,DefaultTaskPoolOptions,ImageSettingsare now setup resources, and need to be added withinsert_setup_resource.