Make AppExit more specific about exit reason.

[Brezak]

Objective

Closes #13017.

Solution

• Make AppExit a enum with a Success and Error variant.
• Make App::run() return a AppExit if it ever returns.
• Make app runners return a AppExit to signal if they encountered a error.

---

Changelog

Added

• App::should_exit
• AppExit to the bevy and bevy_app preludes,

Changed

• AppExit is now a enum with 2 variants (Success and Error).
• The app's runner function now has to return a AppExit.
• App::run() now also returns the AppExit produced by the runner function.

Migration Guide

• Replace all usages of AppExit with AppExit::Success or AppExit::Failure.
• Any custom app runners now need to return a AppExit. We suggest you return a AppExit::Error if any AppExit raised was a Error. You can use the new App::should_exit method.
• If not exiting from main any other way. You should return the AppExit from App::run() so the app correctly returns a error code if anything fails e.g.

fn main() -> AppExit {
    App::new()
        //Your setup here...
        .run()
}

[Brezak]

AppExit doesn't contain a ExitCode as was suggested as we can't check if they're an error.

[alice-i-cecile]

This design is really nice! I like the approach taken, and the integration with the std::ExitCode is a great idea.

[Brezak]

bevy/crates/bevy_app/src/app.rs

Lines 865 to 867 in 8db3d1d

	/// Several app runners in this crate keep their own [`ManualEventReader<AppExit>`].
	/// This exists to accommodate them.
	pub(crate) fn should_exit_manual(&self, reader: &mut ManualEventReader<AppExit>) -> Option<AppExit> {

Regarding this function. I don't know why for example the schedule runner uses it's own ManualEventReader to keep track of app exit events but I feel like it's important. If anybody could explain why I'd like to add it to the documentation and make should_exit_manual the preferred way to check for AppExit events.

[alice-i-cecile]

Manual event readers are required when working with exclusive world access, such as when App is involved. EventReader caches a Local, so it's trickier to initialize.

We could use one-shot systems these days, but I'm not sure it would be an improvement.

[Brezak]

Manual event readers are required when working with exclusive world access, such as when App is involved. EventReader caches a Local, so it's trickier to initialize.

We could use one-shot systems these days, but I'm not sure it would be an improvement.

The question is why do our app runners maintain event reader state at all. Both the witnit_runner and the schedule_runner maintain a ManualEventReader between updates. Couldn't we just construct a ManualEventReader in-situ. The only reason I see that one would maintain a ManualEventReader between updates is to skip already read events, but all our app runners exit immediately a AppExit happens in a update. There are no events for them to skip.

[BD103]

I'm a big fan of this change, thanks for working on it! I have a few suggestions for making some AppExit methods const, but they're non-blocking.

The only other thing I want to bring up are the two BEFORE_MERGE comments, just so they don't get skipped. :)

[alice-i-cecile]

The question is why do our app runners maintain event reader state at all. Both the witnit_runner and the schedule_runner maintain a ManualEventReader between updates. Couldn't we just construct a ManualEventReader in-situ. The only reason I see that one would maintain a ManualEventReader between updates is to skip already read events, but all our app runners exit immediately a AppExit happens in a update. There are no events for them to skip.

Yeah, I don't think that we need to maintain event reader state in this case. I think we should make that change in a different PR though: it'll make it easier to test and review.

[alice-i-cecile]

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1300 if you'd like to help out.