-
-
Notifications
You must be signed in to change notification settings - Fork 4k
fix: added plugin registry #13400
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
base: main
Are you sure you want to change the base?
fix: added plugin registry #13400
Changes from 26 commits
7694270
d4b2fad
4994fd6
06a7c9f
d0d2fc2
2f928b5
fa6c448
5ed6fb6
97ff405
a2f36e6
1c1dd7e
e581bee
94f3b18
0ad4cfb
acd6503
569a658
6bc60c8
a0fd7d2
ad4234f
ea35d86
686c9ad
c86af5e
02d7d91
f0e78f0
f04d557
8ae903a
290e218
12c9981
0db7783
a5f03a3
997ea33
bd95b32
0133487
cb37cd7
1c2631c
4aa6ee5
4ea8965
7ab7dab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Large diffs are not rendered by default.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,8 +1,41 @@ | ||
use downcast_rs::{impl_downcast, Downcast}; | ||
|
||
use crate::App; | ||
use crate::{App, InternedAppLabel}; | ||
use std::any::Any; | ||
|
||
/// Plugin state in the application | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More docs please! Information about how to access this data in the form of a doc test would be particularly valuable, but we should also have a note about how this is the expected lifecycle that plugins flow through. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'll try to add a full example, that maybe explains also what each state purpose is. I left this part out until there's a general agreement for the PR, if that makes sense. |
||
#[derive(Clone, Copy, Debug, Default, PartialEq, Eq, PartialOrd, Ord)] | ||
pub enum PluginState { | ||
/// Plugin is not initialized. | ||
#[default] | ||
Idle, | ||
/// Plugin is initialized. | ||
Init, | ||
/// Plugin is being built. | ||
Building, | ||
/// Plugin is being configured. | ||
Configuring, | ||
/// Plugin configuration is finishing. | ||
Finalizing, | ||
/// Plugin configuration is completed. | ||
Done, | ||
/// Plugin resources are cleaned up. | ||
Cleaned, | ||
} | ||
|
||
impl PluginState { | ||
pub(crate) fn next(self) -> Self { | ||
match self { | ||
Self::Idle => Self::Init, | ||
Self::Init => Self::Building, | ||
Self::Building => Self::Configuring, | ||
Self::Configuring => Self::Finalizing, | ||
Self::Finalizing => Self::Done, | ||
s => unreachable!("Cannot handle {:?} state", s), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Can we return an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is one of those cases that should really not happen, or it would be a bug in the plugin registry management. Would still an option be a better choice? |
||
} | ||
} | ||
} | ||
|
||
/// A collection of Bevy app logic and configuration. | ||
/// | ||
/// Plugins configure an [`App`]. When an [`App`] registers a plugin, | ||
|
@@ -19,7 +52,7 @@ use std::any::Any; | |
/// When adding a plugin to an [`App`]: | ||
/// * the app calls [`Plugin::build`] immediately, and register the plugin | ||
/// * once the app started, it will wait for all registered [`Plugin::ready`] to return `true` | ||
/// * it will then call all registered [`Plugin::finish`] | ||
/// * it will then call all registered [`Plugin::finalize`] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we need to call out the other life cycle steps added in this PR in these docs? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 I'll fix all pending docs comments after the general code review, if that's ok (but open to do otherwise if you feel this is better) |
||
/// * and call all registered [`Plugin::cleanup`] | ||
/// | ||
/// ## Defining a plugin. | ||
|
@@ -56,19 +89,46 @@ use std::any::Any; | |
/// # fn damp_flickering() {} | ||
/// ```` | ||
pub trait Plugin: Downcast + Any + Send + Sync { | ||
/// Configures the [`App`] to which this plugin is added. | ||
fn build(&self, app: &mut App); | ||
/// Returns required sub apps before finalizing this plugin. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More docs here about why this is useful to users please. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we keep this, a doc test demonstrating how to require the rendering subapp would be really valuable. This will be by far the most common pattern. |
||
fn require_sub_apps(&self) -> Vec<InternedAppLabel> { | ||
pietrosophya marked this conversation as resolved.
Show resolved
Hide resolved
|
||
Vec::new() | ||
} | ||
|
||
/// Has the plugin finished its setup? This can be useful for plugins that need something | ||
/// asynchronous to happen before they can finish their setup, like the initialization of a renderer. | ||
/// Once the plugin is ready, [`finish`](Plugin::finish) should be called. | ||
fn ready(&self, _app: &App) -> bool { | ||
/// Pre-configures the [`App`] to which this plugin is added. This is usually executed before | ||
pietrosophya marked this conversation as resolved.
Show resolved
Hide resolved
|
||
/// the event loop is started. | ||
fn init(&self, _app: &mut App) { | ||
// do nothing | ||
} | ||
|
||
/// Is the plugin ready to be built? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More information about how this might be used would be good. |
||
fn ready_to_build(&self, _app: &mut App) -> bool { | ||
alice-i-cecile marked this conversation as resolved.
Show resolved
Hide resolved
|
||
true | ||
} | ||
|
||
/// Finish adding this plugin to the [`App`], once all plugins registered are ready. This can | ||
/// Builds the [`Plugin`] resources. This is usually executed inside an event loop. | ||
pietrosophya marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fn build(&self, _app: &mut App) { | ||
pietrosophya marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// do nothing | ||
} | ||
|
||
/// Is the plugin ready to be configured? | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. More docs. |
||
fn ready_to_configure(&self, _app: &mut App) -> bool { | ||
true | ||
} | ||
|
||
/// Configures the [`App`] to which this plugin is added. This can | ||
/// be useful for plugins that needs completing asynchronous configuration. | ||
fn configure(&self, _app: &mut App) { | ||
// do nothing | ||
} | ||
|
||
/// Is the plugin ready to be finalized? | ||
fn ready_to_finalize(&self, _app: &mut App) -> bool { | ||
true | ||
} | ||
|
||
/// Finalizes this plugin to the [`App`]. This can | ||
/// be useful for plugins that depends on another plugin asynchronous setup, like the renderer. | ||
fn finish(&self, _app: &mut App) { | ||
fn finalize(&self, _app: &mut App) { | ||
pietrosophya marked this conversation as resolved.
Show resolved
Hide resolved
|
||
// do nothing | ||
} | ||
|
||
|
@@ -90,6 +150,35 @@ pub trait Plugin: Downcast + Any + Send + Sync { | |
fn is_unique(&self) -> bool { | ||
true | ||
} | ||
|
||
/// Updates the plugin to a desired [`PluginState`]. | ||
pietrosophya marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fn update(&mut self, app: &mut App, state: PluginState) { | ||
match state { | ||
PluginState::Init => self.init(app), | ||
PluginState::Building => self.build(app), | ||
PluginState::Configuring => self.configure(app), | ||
PluginState::Finalizing => self.finalize(app), | ||
PluginState::Done => {} | ||
s => panic!("Cannot handle {s:?} state"), | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We can just remove this arm, correct? Exhaustive matching is good! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. There is the extra cleanup, that should not be executed as part of the update. But it could be just added to the match and the error described better. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Idle and cleanup are the two states that cannot be processed. Is it better to add both to the match? |
||
} | ||
} | ||
|
||
/// Updates if the plugin is ready to progress to the desired next [`PluginState`]. | ||
fn ready(&self, app: &mut App, next_state: PluginState) -> bool { | ||
match next_state { | ||
PluginState::Building => self.ready_to_build(app), | ||
PluginState::Configuring => self.ready_to_configure(app), | ||
PluginState::Finalizing => self.ready_to_finalize(app), | ||
_ => true, | ||
} | ||
} | ||
|
||
/// Checks all required [`SubApp`]]s. | ||
pietrosophya marked this conversation as resolved.
Show resolved
Hide resolved
|
||
fn check_required_sub_apps(&mut self, app: &App) -> bool { | ||
self.require_sub_apps() | ||
.iter() | ||
.all(|s| app.contains_sub_app(*s)) | ||
} | ||
} | ||
|
||
impl_downcast!(Plugin); | ||
|
@@ -100,26 +189,6 @@ impl<T: Fn(&mut App) + Send + Sync + 'static> Plugin for T { | |
} | ||
} | ||
|
||
/// Plugins state in the application | ||
#[derive(PartialEq, Eq, Debug, Clone, Copy, PartialOrd, Ord)] | ||
pub enum PluginsState { | ||
/// Plugins are being added. | ||
Adding, | ||
/// All plugins already added are ready. | ||
Ready, | ||
/// Finish has been executed for all plugins added. | ||
Finished, | ||
/// Cleanup has been executed for all plugins added. | ||
Cleaned, | ||
} | ||
|
||
/// A dummy plugin that's to temporarily occupy an entry in an app's plugin registry. | ||
pub(crate) struct PlaceholderPlugin; | ||
|
||
impl Plugin for PlaceholderPlugin { | ||
fn build(&self, _app: &mut App) {} | ||
} | ||
|
||
/// A type representing an unsafe function that returns a mutable pointer to a [`Plugin`]. | ||
/// It is used for dynamically loading plugins. | ||
/// | ||
|
Uh oh!
There was an error while loading. Please reload this page.