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

Add subgroup #5022

Closed
wants to merge 7 commits into from
Closed

Add subgroup #5022

wants to merge 7 commits into from

Conversation

BD103
Copy link
Member

@BD103 BD103 commented Jun 15, 2022

Objective

Sometimes a PluginGroup requires another PluginGroup, like DefaultPlugins. The previous solution to this was:

use bevy::prelude::*;
use bevy::app::PluginGroupBuilder;

struct GamePlugins;

impl PluginGroup for GamePlugins {
    fn build(&mut self, group: &mut PluginGroupBuilder) {
        DefaultPlugins.build(group);

        // Add more plugins here...
    }
}

This feels backwards, even though it is correct.

Solution

Add a quality-of-life function that merges another group into the current one.

use bevy::prelude::*;
use bevy::app::PluginGroupBuilder;

struct GamePlugins;

impl PluginGroup for GamePlugins {
    fn build(&mut self, group: &mut PluginGroupBuilder) {
        group.add_subgroup(DefaultPlugins);

        // Add more plugins here...
    }
}

Changelog

Added PlugingGroupBuilder::add_subgroup().

Reasoning

My game is structured in a set of plugins. All you need to do is call App::new().add_plugins(GamePlugins).run(). This ensures that all the required plugins are registered. If main.rs has to also register DefaultPlugins, but doesn't, then my game would break. To fix this, I attach the default plugins as seen in the first example. I was dissatisfied with how that broke the flow of the code, so I wrote this function to fix it.

@BD103
Copy link
Member Author

BD103 commented Jun 15, 2022

I've fixed the doc example, though now it simulates DefaultPlugins instead of actually using them. The build is failing because of a ron deprecation, but #5021 should fix that.

If requested, I can add this to the plugin group example.

Also, please look over the unit test and make sure I did it correctly. This is my first time contributing to Bevy, so I may have done something wrong.

@mockersf mockersf added C-Usability A targeted quality-of-life change that makes Bevy easier to use A-App Bevy apps and plugins labels Jun 15, 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.

I like this API; I think that this helps make the logic clearer when working with plugin groups.

Great work with the doc tests, and the PR writeup did a great job motivating this change.

I think this would be worth adding to the plugin group example, but I won't block on it.

@BD103
Copy link
Member Author

BD103 commented Jun 22, 2022

Thanks for the feedback! I'll look into adding an example.

@BD103
Copy link
Member Author

BD103 commented Jun 23, 2022

I have updated the example to show using add_subgroup. As long as that looks good, this PR is ready to merge.

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.

Example looks good. This needs one more review from the community before I can merge it; perhaps ask in #engine-dev on Discord?

@BD103
Copy link
Member Author

BD103 commented Jun 23, 2022

I'm sorry, but I don't use Discord. Do you have any other suggestions?

@alice-i-cecile
Copy link
Member

I'll ask for you then :) No worries!

@BD103
Copy link
Member Author

BD103 commented Jun 23, 2022

Thanks! :D

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.

This current implementation rebuilds the entire plugin group it depends on. If done from multiple plugin groups declaring them as dependencies, you'll reregister everything under that dependency N times. This is clearly not desirable in the general case. We need explicit deduplication before this should be considered.

@alice-i-cecile alice-i-cecile added the S-Blocked This cannot move forward until something else changes label Jun 23, 2022
@alice-i-cecile
Copy link
Member

Blocked on #69 then.

@BD103
Copy link
Member Author

BD103 commented Sep 14, 2022

It's been a while since there's been activity here. Should I close the PR or leave it open?

@alice-i-cecile
Copy link
Member

Let's close this out per the comment from James; we can always reopen in the future.

@BD103
Copy link
Member Author

BD103 commented Sep 14, 2022

Ok, I won't delete my fork though until this gets resolved.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Blocked This cannot move forward until something else changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants