Skip to content

Commit 4ee4144

Browse files
atlv24gwafotapa
authored andcommitted
Fix App::get_added_plugins not working inside finish and cleanup (bevyengine#20506)
# Objective This assertion fails when called inside finish or cleanup: ```rs assert_eq!( app.is_plugin_added::<ImagePlugin>(), app.get_added_plugins::<ImagePlugin>().len() > 0 ); ``` ## Solution - Do the hokey pokey one plugin at a time - swap_remove to stay O(1), then push and swap back to preserve plugin order ## Testing - Someone should write unit tests for this and probably document that it hokey pokeys, although i dont think anyone will `get_added_plugins<Self>` because you have a `self` param in finish and cleanup anyways.
1 parent 41cfb9f commit 4ee4144

File tree

2 files changed

+103
-26
lines changed

2 files changed

+103
-26
lines changed

crates/bevy_app/src/app.rs

Lines changed: 85 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -256,27 +256,29 @@ impl App {
256256
/// plugins are ready, but can be useful for situations where you want to use [`App::update`].
257257
pub fn finish(&mut self) {
258258
// plugins installed to main should see all sub-apps
259-
let plugins = core::mem::take(&mut self.main_mut().plugin_registry);
260-
for plugin in &plugins {
261-
plugin.finish(self);
259+
// do hokey pokey with a boxed zst plugin (doesn't allocate)
260+
let mut hokeypokey: Box<dyn Plugin> = Box::new(HokeyPokey);
261+
for i in 0..self.main().plugin_registry.len() {
262+
core::mem::swap(&mut self.main_mut().plugin_registry[i], &mut hokeypokey);
263+
hokeypokey.finish(self);
264+
core::mem::swap(&mut self.main_mut().plugin_registry[i], &mut hokeypokey);
262265
}
263-
let main = self.main_mut();
264-
main.plugin_registry = plugins;
265-
main.plugins_state = PluginsState::Finished;
266+
self.main_mut().plugins_state = PluginsState::Finished;
266267
self.sub_apps.iter_mut().skip(1).for_each(SubApp::finish);
267268
}
268269

269270
/// Runs [`Plugin::cleanup`] for each plugin. This is usually called by the event loop after
270271
/// [`App::finish`], but can be useful for situations where you want to use [`App::update`].
271272
pub fn cleanup(&mut self) {
272273
// plugins installed to main should see all sub-apps
273-
let plugins = core::mem::take(&mut self.main_mut().plugin_registry);
274-
for plugin in &plugins {
275-
plugin.cleanup(self);
274+
// do hokey pokey with a boxed zst plugin (doesn't allocate)
275+
let mut hokeypokey: Box<dyn Plugin> = Box::new(HokeyPokey);
276+
for i in 0..self.main().plugin_registry.len() {
277+
core::mem::swap(&mut self.main_mut().plugin_registry[i], &mut hokeypokey);
278+
hokeypokey.cleanup(self);
279+
core::mem::swap(&mut self.main_mut().plugin_registry[i], &mut hokeypokey);
276280
}
277-
let main = self.main_mut();
278-
main.plugin_registry = plugins;
279-
main.plugins_state = PluginsState::Cleaned;
281+
self.main_mut().plugins_state = PluginsState::Cleaned;
280282
self.sub_apps.iter_mut().skip(1).for_each(SubApp::cleanup);
281283
}
282284

@@ -1390,6 +1392,12 @@ impl App {
13901392
}
13911393
}
13921394

1395+
// Used for doing hokey pokey in finish and cleanup
1396+
pub(crate) struct HokeyPokey;
1397+
impl Plugin for HokeyPokey {
1398+
fn build(&self, _: &mut App) {}
1399+
}
1400+
13931401
type RunnerFn = Box<dyn FnOnce(App) -> AppExit>;
13941402

13951403
fn run_once(mut app: App) -> AppExit {
@@ -1526,6 +1534,38 @@ mod tests {
15261534
}
15271535
}
15281536

1537+
struct PluginF;
1538+
1539+
impl Plugin for PluginF {
1540+
fn build(&self, _app: &mut App) {}
1541+
1542+
fn finish(&self, app: &mut App) {
1543+
// Ensure other plugins are available during finish
1544+
assert_eq!(
1545+
app.is_plugin_added::<PluginA>(),
1546+
!app.get_added_plugins::<PluginA>().is_empty(),
1547+
);
1548+
}
1549+
1550+
fn cleanup(&self, app: &mut App) {
1551+
// Ensure other plugins are available during finish
1552+
assert_eq!(
1553+
app.is_plugin_added::<PluginA>(),
1554+
!app.get_added_plugins::<PluginA>().is_empty(),
1555+
);
1556+
}
1557+
}
1558+
1559+
struct PluginG;
1560+
1561+
impl Plugin for PluginG {
1562+
fn build(&self, _app: &mut App) {}
1563+
1564+
fn finish(&self, app: &mut App) {
1565+
app.add_plugins(PluginB);
1566+
}
1567+
}
1568+
15291569
#[test]
15301570
fn can_add_two_plugins() {
15311571
App::new().add_plugins((PluginA, PluginB));
@@ -1595,6 +1635,39 @@ mod tests {
15951635
app.finish();
15961636
}
15971637

1638+
#[test]
1639+
fn test_get_added_plugins_works_during_finish_and_cleanup() {
1640+
let mut app = App::new();
1641+
app.add_plugins(PluginA);
1642+
app.add_plugins(PluginF);
1643+
app.finish();
1644+
}
1645+
1646+
#[test]
1647+
fn test_adding_plugin_works_during_finish() {
1648+
let mut app = App::new();
1649+
app.add_plugins(PluginA);
1650+
app.add_plugins(PluginG);
1651+
app.finish();
1652+
assert_eq!(
1653+
app.main().plugin_registry[0].name(),
1654+
"bevy_app::main_schedule::MainSchedulePlugin"
1655+
);
1656+
assert_eq!(
1657+
app.main().plugin_registry[1].name(),
1658+
"bevy_app::app::tests::PluginA"
1659+
);
1660+
assert_eq!(
1661+
app.main().plugin_registry[2].name(),
1662+
"bevy_app::app::tests::PluginG"
1663+
);
1664+
// PluginG adds PluginB during finish
1665+
assert_eq!(
1666+
app.main().plugin_registry[3].name(),
1667+
"bevy_app::app::tests::PluginB"
1668+
);
1669+
}
1670+
15981671
#[test]
15991672
fn test_derive_app_label() {
16001673
use super::AppLabel;

crates/bevy_app/src/sub_app.rs

Lines changed: 18 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -401,25 +401,29 @@ impl SubApp {
401401

402402
/// Runs [`Plugin::finish`] for each plugin.
403403
pub fn finish(&mut self) {
404-
let plugins = core::mem::take(&mut self.plugin_registry);
405-
self.run_as_app(|app| {
406-
for plugin in &plugins {
407-
plugin.finish(app);
408-
}
409-
});
410-
self.plugin_registry = plugins;
404+
// do hokey pokey with a boxed zst plugin (doesn't allocate)
405+
let mut hokeypokey: Box<dyn Plugin> = Box::new(crate::HokeyPokey);
406+
for i in 0..self.plugin_registry.len() {
407+
core::mem::swap(&mut self.plugin_registry[i], &mut hokeypokey);
408+
self.run_as_app(|app| {
409+
hokeypokey.finish(app);
410+
});
411+
core::mem::swap(&mut self.plugin_registry[i], &mut hokeypokey);
412+
}
411413
self.plugins_state = PluginsState::Finished;
412414
}
413415

414416
/// Runs [`Plugin::cleanup`] for each plugin.
415417
pub fn cleanup(&mut self) {
416-
let plugins = core::mem::take(&mut self.plugin_registry);
417-
self.run_as_app(|app| {
418-
for plugin in &plugins {
419-
plugin.cleanup(app);
420-
}
421-
});
422-
self.plugin_registry = plugins;
418+
// do hokey pokey with a boxed zst plugin (doesn't allocate)
419+
let mut hokeypokey: Box<dyn Plugin> = Box::new(crate::HokeyPokey);
420+
for i in 0..self.plugin_registry.len() {
421+
core::mem::swap(&mut self.plugin_registry[i], &mut hokeypokey);
422+
self.run_as_app(|app| {
423+
hokeypokey.cleanup(app);
424+
});
425+
core::mem::swap(&mut self.plugin_registry[i], &mut hokeypokey);
426+
}
423427
self.plugins_state = PluginsState::Cleaned;
424428
}
425429

0 commit comments

Comments
 (0)