Skip to content

Fix App::get_added_plugins not working inside finish and cleanup #20506

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
merged 3 commits into from
Aug 12, 2025
Merged

Fix App::get_added_plugins not working inside finish and cleanup #20506

merged 3 commits into from
Aug 12, 2025
+103 −26

Conversation

atlv24
Copy link
Contributor

@atlv24 atlv24 commented Aug 11, 2025 •
edited
Loading

Objective

This assertion fails when called inside finish or cleanup:

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.

@atlv24 atlv24 added C-Bug An unexpected or incorrect behavior A-App Bevy apps and plugins S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 11, 2025
@atlv24 atlv24 added this to the 0.17 milestone Aug 11, 2025
@atlv24 atlv24 mentioned this pull request Aug 11, 2025
Merged
tychedelia
tychedelia approved these changes Aug 11, 2025
View reviewed changes
Copy link
Member

@tychedelia tychedelia left a comment

Choose a reason for hiding this comment

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

Oof! Good find.

janhohenheim
janhohenheim reviewed Aug 11, 2025
View reviewed changes
Copy link
Member

@janhohenheim janhohenheim left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Can't you just iterate over the plugins without mem taking them? Why remove and reinsert them? Does Plugin::finish require the current plugin to not be in the plugin registry?
Asking because this seems to me like it's lowering the code quality to preserve this invariant and I don't understand why.

@atlv24
Copy link
Contributor Author

atlv24 commented Aug 11, 2025 •
edited
Loading

Plugin::finish/cleanup takes a &mut App, and the plugin is stored in App::main().plugin_registry. You can't hold an immutable reference and a mutable reference to the same object at the same time

janhohenheim
janhohenheim approved these changes Aug 11, 2025
View reviewed changes
Copy link
Member

@janhohenheim janhohenheim left a comment

Choose a reason for hiding this comment

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

makes sense, thx :)

@janhohenheim janhohenheim added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Aug 11, 2025
james7132
james7132 previously requested changes Aug 11, 2025
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.

I'd like to see a regression test added, in this PR, to ensure we don't reintroduce this bug.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Aug 11, 2025
@atlv24 atlv24 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 11, 2025
@atlv24 atlv24 requested a review from james7132 August 11, 2025 18:03
@andriyDev
Copy link
Contributor

Does this work correctly when adding plugins in finish/cleanup? I wonder if it'll swap the correct thing if we're adding plugins

@atlv24
Copy link
Contributor Author

atlv24 commented Aug 11, 2025

Thats a really good point

@atlv24
Copy link
Contributor Author

atlv24 commented Aug 11, 2025

I think it didnt work before. Seems the plugin registry would just get overwritten lol, so this is actually gonna fix two bugs.

@atlv24
Copy link
Contributor Author

atlv24 commented Aug 11, 2025

What does it even mean for a plugin to get added during finish or cleanup though? should finish/cleanup run on those plugins?

@atlv24
Copy link
Contributor Author

atlv24 commented Aug 11, 2025

Okay i fixed all the things, assigned to @alice-i-cecile because hokey pokey :)

atlv24
atlv24 commented Aug 11, 2025
View reviewed changes
@andriyDev
Copy link
Contributor

What does it even mean for a plugin to get added during finish or cleanup though? should finish/cleanup run on those plugins?

You're totally correct that it's poorly defined. However it's something we've used in practice. I stumbled upon such a usage during my RenderStartup refactors. I can dig up that case if need be. In that context, it didn't use finish or cleanup so it was fine, but obviously not an ideal situation.

@alice-i-cecile alice-i-cecile dismissed james7132’s stale review August 11, 2025 22:36

Regression test has been added.

james7132
james7132 approved these changes Aug 11, 2025
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.

Regression test LGTM, though adding new Plugins mid-finish is mighty cursed. Left a comment regarding the HokeyPokey implementation that needs to be addressed before merging though.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Aug 11, 2025
@atlv24 atlv24 added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 11, 2025
@atlv24
Copy link
Contributor Author

atlv24 commented Aug 11, 2025

It doesn't allocate, its ZST

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 12, 2025
Merged via the queue into bevyengine:main with commit 1224ba7 Aug 12, 2025
36 checks passed
gwafotapa pushed a commit to gwafotapa/bevy that referenced this pull request Aug 12, 2025
@atlv24 @gwafotapa
Fix App::get_added_plugins not working inside finish and cleanup (bev…
4ee4144 
…yengine#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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@james7132 james7132 james7132 approved these changes

@janhohenheim janhohenheim janhohenheim approved these changes

@tychedelia tychedelia tychedelia approved these changes

Assignees

@alice-i-cecile alice-i-cecile

Labels
A-App Bevy apps and plugins 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
Milestone
0.17
Development

Successfully merging this pull request may close these issues.
6 participants
@atlv24 @andriyDev @james7132 @janhohenheim @tychedelia @alice-i-cecile