Refactor App and SubApp internals for better separation

[maniwani]

Objective

This is a necessary precursor to #9122 (this was split from that PR to reduce the amount of code to review all at once).

Moving !Send resource ownership to App will make it unambiguously !Send. SubApp must be Send, so it can't wrap App.

Solution

Refactor App and SubApp to not have a recursive relationship. Since SubApp no longer wraps App, once !Send resources are moved out of World and into App, SubApp will become unambiguously Send.

There could be less code duplication between App and SubApp, but that would break App method chaining.

Changelog

• SubApp no longer wraps App.
• App fields are no longer publicly accessible.
• App can no longer be converted into a SubApp.
• Various methods now return references to a SubApp instead of an App.

Migration Guide

• To construct a sub-app, use SubApp::new(). App can no longer convert into SubApp.
• If you implemented a trait for App, you may want to implement it for SubApp as well.
• If you're accessing app.world directly, you now have to use app.world() and app.world_mut().
• App::sub_app now returns &SubApp.
• App::sub_app_mut now returns &mut SubApp.
• App::get_sub_app now returns Option<&SubApp>.
• App::get_sub_app_mut now returns Option<&mut SubApp>.

[github-actions]

Example alien_cake_addict failed to run, please try running it locally and check the result.

[github-actions]

Example alien_cake_addict failed to run, please try running it locally and check the result.

[github-actions]

Example alien_cake_addict failed to run, please try running it locally and check the result.

[joseph-gio]

After scanning through this a couple of times, the code checks out, and it seems well-motivated.

[joseph-gio]

What's the SubApps struct for? How come these fields aren't just stored directly under App?

[maniwani]

See the App::into_parts method in #9122. I was asked to split this part of the changes into a separate PR. In #9122, updating an App involves splitting it into its Send and !Send halves (App is !Send). SubApps is the Send half.

[joseph-gio]

Yeah I understand the motivation behind this PR. I'm specifically asking about these two fields.

Why is it defined like this

struct App {
    sub_apps: SubApps,
    ...
}
struct SubApps {
    main: SubApp,
    sub_apps: HashMap<AppLabelId, SubApp>,
}

instead of like this?

struct App {
    main: SubApp,
    sub_apps: HashMap<AppLabelId, SubApp>,
    ...
}

[maniwani]

It's so you don't have to write the contents of SubApps::update by hand in each runner (it replaces App::update since runners basically can't use that after #9122). If you'd like to get rid of the sub_apps.sub_apps, please suggest a different composition that still avoids boilerplate.

[joseph-gio]

Oh that makes sense. If SubApps is meant to be used externally, then there should probably be a way of accessing it.

[maniwani]

Would this be better?

struct App {
    main: SubApp,
    sub_apps: SubApps,
    ...
}

#[derive(Deref, DerefMut)]
struct SubApps(HashMap<AppLabelId, SubApp>);

impl SubApps {
   fn extract(&mut self, main: &mut SubApp) {
       ...
   }
   
   ...
}

[joseph-gio]

Excluding the main app from the sub-apps collection does make more sense to me, yeah.

[hymm]

What about pushing things in the other direction and try to make the main app less special?

struct App { 
    active_app_label: AppLabel, 
    sub_apps: SubApps, ... 
}

impl SubApps {
    fn extract(&mut self, main_label: AppLabel) {
        let main_app = self.0.get(main_label);
        ...
    }
}

[hymm]

Generally in favor of the direction of this PR. Definitely moves in the right direction to polish up the sub app abstraction.

Note: I skipped over doc comments for this review pass, but looked at everything else.

[hymm]

The logic for a lot of these feels like it should be inverted. i.e. We should just be passing SubApp into the plugin methods. But rename SubApp -> App and rename the super app into something else like Bevy::new(). That would get rid of most if not all of the uses of run_as_app.

edit: actually on second thought this is might be controversial enough that it should be pushed off to a separate pr. Render plugins will need to be aware of which app they're being added to. Which will require some major changes. But the data flow is definitely a little wonky now since SubApps own plugins, but I'm fine with fixing in another pr and merging as is to unblock fixing nonsend resources.

[maniwani]

That's a bigger change, yeah. Plugins can currently see everything and the various render-related plugins do currently make use of that to add things to both the main app and the rendering sub-app.

[lee-orr]

Could that be achieved with an API call? something like:

impl Plugin for MyPlugin {
  fn build(&mut sub_app) {
    let render_app = sub_app.get_sibling(RenderApp);
  }
}

[hymm]

What about pushing things in the other direction and try to make the main app less special?

struct App { 
    active_app_label: AppLabel, 
    sub_apps: SubApps, ... 
}

impl SubApps {
    fn extract(&mut self, main_label: AppLabel) {
        let main_app = self.0.get(main_label);
        ...
    }
}

[hymm]

did a review pass over the docs. Seems like there still needs to be some decision on where to put the main app?

[hymm]

Willing to merge this as is to unblock #9122. There's an open question about how and where main app should be stored, but we can adjust that in follow up PRs as it probably won't effect the public apis.

[alice-i-cecile]

I like this architecture a lot better, and the code / doc quality is solid.

In particular, no longer storing App nested indefinitely in itself is really important for architectural (and UX) clarity. I agree with Hymm, moving this forward is important and this is definitely a step in the right direction.

[james7132]

Sorry for taking so long to review this. LGTM. Most of the outstanding comments here can be addressed in a followup PR if need be.

[joseph-gio]

LGTM still. Re-approved