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

feat(bevy_app): expose an API to perform updates for a specific sub-app. #14009

Merged

Conversation

Soulghost
Copy link
Contributor

@Soulghost Soulghost commented Jun 25, 2024

Objective

Solution

  • Expose an API to perform updates for a specific sub-app, so we can avoid mutable borrow the app twice.

Testing

  • I have tested the API by modifying the code in the many_lights example with the following changes:
impl Plugin for LogVisibleLights {
    fn build(&self, app: &mut App) {
        let Some(render_app) = app.get_sub_app_mut(RenderApp) else {
            return;
        };

        render_app.add_systems(Render, print_visible_light_count.in_set(RenderSet::Prepare));
    }

    fn finish(&self, app: &mut App) {
        app.update_sub_app_by_label(RenderApp);
    }
}

Changelog

  • add the update_sub_app_by_label API to App and SubApps.

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.

Added some suggestions for making the documentation more in line with the existing methods

crates/bevy_app/src/app.rs Outdated Show resolved Hide resolved
crates/bevy_app/src/sub_app.rs Outdated Show resolved Hide resolved
@janhohenheim janhohenheim added D-Trivial Nice and easy! A great choice to get started with Bevy A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 25, 2024
@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Jun 25, 2024
@alice-i-cecile
Copy link
Member

@anlumo does this resolve your concerns?

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.

Agreed on the doc nits, but this is a fundamentally reasonable method to exist.

@anlumo
Copy link
Contributor

anlumo commented Jun 25, 2024

Looks good to me!

Soulghost and others added 3 commits June 25, 2024 20:59
Co-authored-by: Jan Hohenheim <jan@hohenheim.ch>
Co-authored-by: Jan Hohenheim <jan@hohenheim.ch>
@alice-i-cecile alice-i-cecile 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 Jun 25, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 25, 2024
Merged via the queue into bevyengine:main with commit f51a306 Jun 25, 2024
28 checks passed
mockersf pushed a commit that referenced this pull request Jun 25, 2024
…pp. (#14009)

# Objective

- Fixes #14003

## Solution

- Expose an API to perform updates for a specific sub-app, so we can
avoid mutable borrow the app twice.

## Testing

- I have tested the API by modifying the code in the `many_lights`
example with the following changes:
```rust
impl Plugin for LogVisibleLights {
    fn build(&self, app: &mut App) {
        let Some(render_app) = app.get_sub_app_mut(RenderApp) else {
            return;
        };

        render_app.add_systems(Render, print_visible_light_count.in_set(RenderSet::Prepare));
    }

    fn finish(&self, app: &mut App) {
        app.update_sub_app_by_label(RenderApp);
    }
}
```

---

## Changelog
- add the `update_sub_app_by_label` API to `App` and `SubApps`.

---------

Co-authored-by: Jan Hohenheim <jan@hohenheim.ch>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Trivial Nice and easy! A great choice to get started with Bevy 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
Development

Successfully merging this pull request may close these issues.

SubApp::extract cannot be called due to borrow checker
4 participants