AppExit should return from App::run without quitting the process

[jakerr]

It seems a bit counter intuitive that when default plugins are added, App::run() is effectively a diverging function that never returns. This means that any code after the call to App::run() is silently ignored.

With add_default_plugins

use bevy::prelude::*;

fn main() {
    println!("Before");
    App::build()
        .add_default_plugins()
        .add_startup_system(setup.system())
        .run();
    println!("After");
}

prints:

Before

When the window is closed, the process exits and App::run() never returns.

Without add_default_plugins

In the hellow_world.rs example that doesn't add default plugins this behaves more as I would expect:

    println!("Before");
    App::build().add_system(hello_world_system.system()).run();
    println!("After");

prints:

Before
hello world
After

Conclusion

In my opinion the most intuitive behavior would be if the AppExit event just caused the App::run() function call happening on the main thread to return control (as it does in the hello_world example).

[Ratysz]

This might be unresolvable, due to how winit works - it has to take over your main thread forever; one of the default resources, as far as I know, is that.

[soruh]

In that case it might make sense to have run always diverge ( -> !) so that the compile can tell you that code after it is unreachable.

[cart]

This is a tough one. On the one hand, I like knowing when code is unreachable. On the other hand, for apps that dont need winit, I'm not sure I want to prevent them from being used within a larger context.

Ideally we can find a way to make winit return. If not I'm inclined to leave the current behavior the way it is, despite the slightly confusing divergent behavior. I'm happy to discuss this more though.

[cart]

Ex: allowing Apps to return makes it much easier to write unit tests

[Ratysz]

For what it's worth, it's possible to have it return on desktop targets (where most unit testing should/will happen, I think), but there are caveats, as outlined in the link. Perhaps a variant of the plugin, for testing and that one inevitable odd setup that cannot live without it?

[jakerr]

@Ratysz Thank you for that link. I was digging in the winit code a bit trying to see if it was configurable but that was the piece I was missing.

I took a look at the macos implementation of run to understand the difference between run and run_return and learned that besides the closure type that they take (as mentioned in the documentation linked), run really does just call run_return followed by process.exit(0). So at least there aren't a lot of implementation details that might complicate our options.

[jakerr]

Here's a similar closed issue in winit that goes into some detail about the design decision there. One caveat that I think we should be aware of as consumers of winit is that if there is anything in scope that is not captured by the closure passed in to run, no destructors for items will be called. As long as everything is moved into the closure, it will be destructed between the run_return and exit calls inside the run implementation.

[jakerr]

So I've given this a bit of thought and I think it'd be great to add an extension on to App supporting the same configurations supported by EventLoopExtDesktop which would allow us to configure App to return from run.

This would let us use it in tests as well as let clients of Bevy make an informed decision for their projects. I'd write similar caveats into the documentation for the new API linking to the caveats in the dependency.

@Ratysz, @cart what do you think? If that sounds good I'd be happy to take the issue.

[cart]

I think that shouldn't be tacked on to App, but rather passed into winit via a configuration Resource of some kind (ex: WinitConfig { run_return: bool })

I'm cool with giving people choices. It seems pretty clear (given the winit docs) that the default should remain as it is though.

So yeah that sounds good to me!

[oh-wind]

Yes it is necessary