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

Added new method remove to PluginGroup, also adds unit tests for PluginGroup #2171

Closed
wants to merge 2 commits into from

Conversation

blaind
Copy link
Contributor

@blaind blaind commented May 15, 2021

As requested in #2166

@blaind blaind mentioned this pull request May 15, 2021
@blaind blaind changed the title Added new method remove to PluginGroup + tests Added new method remove to PluginGroup, also adds unit tests for PluginGroup May 15, 2021
@mockersf
Copy link
Member

why not use the disable method?

@blaind
Copy link
Contributor Author

blaind commented May 15, 2021

why not use the disable method?

The root use case here is to reconfigure RenderPlugin with other config (needed, because some initialization is done in build method that depends on the configuration). disable won't work for that (agreed, the remove -approach is not the best either)

pub fn add_plugins_fn(group: &mut PluginGroupBuilder) -> &mut PluginGroupBuilder {
    group.remove::<bevy_render::RenderPlugin>();
    group.add_after::<bevy_scene::ScenePlugin, _>(bevy_render::RenderPlugin {
        base_render_graph_config: Some(bevy_render::render_graph::base::BaseRenderGraphConfig {
            add_2d_camera: false,
            add_3d_camera: false,
            add_xr_camera: true,
            ..Default::default()
        }),
    });

    group
}

@mockersf
Copy link
Member

mockersf commented May 15, 2021

the bug on disabling a plugin and re-adding it with another config is fixed in #2039 👍

@blaind
Copy link
Contributor Author

blaind commented May 15, 2021

That's great! I guess renders this PR as obsolete. Seems to contain tests as well

@alice-i-cecile
Copy link
Member

Are we good to close this PR then? :)

@blaind
Copy link
Contributor Author

blaind commented May 16, 2021

Are we good to close this PR then? :)

Ok for me, if there's no other use cases for the remove method

@mockersf mockersf added A-Core Common functionality for all bevy apps C-Enhancement A new feature labels May 16, 2021
@alice-i-cecile
Copy link
Member

I don't see any other immediate application of this functionality so I'm going to close this for now. This can be revived if a use case comes up :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Core Common functionality for all bevy apps C-Enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants