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

set_runner doesn't work like it did in 0.11 after updating to 0.12 #10385

Closed
ilyvion opened this issue Nov 5, 2023 · 5 comments · Fixed by #10389
Closed

set_runner doesn't work like it did in 0.11 after updating to 0.12 #10385

ilyvion opened this issue Nov 5, 2023 · 5 comments · Fixed by #10389
Labels
A-App Bevy apps and plugins C-Bug An unexpected or incorrect behavior

Comments

@ilyvion
Copy link
Contributor

ilyvion commented Nov 5, 2023

Bevy version

0.12

What you did

I have a Bevy plugin crate for using Bevy (mainly the ECS, although I also make use of the Bevy App to orchestrate the whole thing) with another game engine.

In order to support this, I'm using a custom runner, which up until 0.12, would always run before anything else, and so I could do some very early initialization safely there.

if app.plugins_state() == PluginsState::Ready {
// If we're already ready, we finish up now and advance one frame.
// This prevents black frames during the launch transition on iOS.
app.finish();
app.cleanup();
app.update();
}

👆 Thanks to these lines added between 0.11 and 0.12, I'm now getting panics, because systems get to run (thanks to the call to app.update()) before my custom runner gets to do its required initializations.

What went wrong

This change was very unexpected to me. I thought the runner would be in charge of everything, including running systems at all, but now this privilege has been taken from me, and it breaks everything.

Is there a way to make it so I can turn this new code off in later Bevy releases? I don't care about iOS support and want to be in total control over calls to app.update().

Additional information

If you're interested, this is my custom runner:

https://github.com/alexschrod/bevy_doryen/blob/140d7eb7a84a1ef9b8f064918d83f39e1f4d9508/src/lib.rs#L348-L381

And the reason it's important that only I get to call app.update() is that this needs to happen before and after each such call:

https://github.com/alexschrod/bevy_doryen/blob/140d7eb7a84a1ef9b8f064918d83f39e1f4d9508/src/lib.rs#L258-L260

@ilyvion ilyvion added C-Bug An unexpected or incorrect behavior S-Needs-Triage This issue needs to be labelled labels Nov 5, 2023
@tim-blackbird
Copy link
Contributor

You can avoid calling app.set_runner() and app.run() entirely and just manually call your existing runner function.
You may want to call app.finish() and app.cleanup() once before that in case any of your plugins make use of those.

@ilyvion
Copy link
Contributor Author

ilyvion commented Nov 5, 2023

I don't know what you mean by "I can avoid" those. Users of my plugin are expected to do

App::new()
    .add_plugins(DoryenPlugin)
    // [and everything else they want to use]
    .run();

like they would any other Bevy project. That's how I want the user experience to be; it's just Bevy coupled with a different engine. And as I pointed out, this has been working just fine for a long time (since at least 0.4 and through to 0.11) and this change realy feels to me like it has "undermined" what runners should have the privilege of being in control over in this stack.

@tim-blackbird
Copy link
Contributor

Sorry about that. I see how it's used now.

It does feel like this change should have happened inside the runner, not outside of it.
I'll take a look at it.

@tim-blackbird
Copy link
Contributor

I've opened #10389 to revert the changes made to App::run(). I'm a bit disappointed that it was unnecessarily changed like that.

Now I'm fairly confident #10389 will be merged, but I'm unsure if it will make it into a 0.12 patch release :(

@ickk
Copy link
Member

ickk commented Nov 8, 2023

This also breaks me

@ickk ickk added A-App Bevy apps and plugins and removed S-Needs-Triage This issue needs to be labelled labels Nov 8, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 16, 2023
…app` (#10389)

# Objective
The way `bevy_app` works was changed unnecessarily in #9826 whose
changes should have been specific to `bevy_winit`.
I'm somewhat disappointed that happened and we can see in
#10195 that it made things more
complicated.

Even worse, in #10385 it's clear that this breaks the clean abstraction
over another engine someone built with Bevy!

Fixes #10385.

## Solution

- Move the changes made to `bevy_app` in #9826 to `bevy_winit`
- Revert the changes to `ScheduleRunnerPlugin` and the `run_once` runner
in #10195 as they're no longer necessary.

While this code is breaking relative to `0.12.0`, it reverts the
behavior of `bevy_app` back to how it was in `0.11`.
Due to the nature of the breakage relative to `0.11` I hope this will be
considered for `0.12.1`.
ickk added a commit to ickk/bevy that referenced this issue Nov 17, 2023
ickk added a commit to ickk/bevy that referenced this issue Nov 17, 2023
github-merge-queue bot pushed a commit that referenced this issue Nov 18, 2023
Bevy introduced unintentional breaking behaviour along with the v0.12.0
release regarding the `App::set_runner` API. See: #10385, #10389 for
details. We weren't able to catch this before release because this API
is only used internally in one or two places (the very places which
motivated the break).

This commit adds a regression test to help guarantee some expected
behaviour for custom runners, namely that `app::update` won't be called
before the runner has a chance to initialise state.
cart pushed a commit that referenced this issue Nov 30, 2023
…app` (#10389)

# Objective
The way `bevy_app` works was changed unnecessarily in #9826 whose
changes should have been specific to `bevy_winit`.
I'm somewhat disappointed that happened and we can see in
#10195 that it made things more
complicated.

Even worse, in #10385 it's clear that this breaks the clean abstraction
over another engine someone built with Bevy!

Fixes #10385.

## Solution

- Move the changes made to `bevy_app` in #9826 to `bevy_winit`
- Revert the changes to `ScheduleRunnerPlugin` and the `run_once` runner
in #10195 as they're no longer necessary.

While this code is breaking relative to `0.12.0`, it reverts the
behavior of `bevy_app` back to how it was in `0.11`.
Due to the nature of the breakage relative to `0.11` I hope this will be
considered for `0.12.1`.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this issue Jan 9, 2024
…app` (bevyengine#10389)

# Objective
The way `bevy_app` works was changed unnecessarily in bevyengine#9826 whose
changes should have been specific to `bevy_winit`.
I'm somewhat disappointed that happened and we can see in
bevyengine#10195 that it made things more
complicated.

Even worse, in bevyengine#10385 it's clear that this breaks the clean abstraction
over another engine someone built with Bevy!

Fixes bevyengine#10385.

## Solution

- Move the changes made to `bevy_app` in bevyengine#9826 to `bevy_winit`
- Revert the changes to `ScheduleRunnerPlugin` and the `run_once` runner
in bevyengine#10195 as they're no longer necessary.

While this code is breaking relative to `0.12.0`, it reverts the
behavior of `bevy_app` back to how it was in `0.11`.
Due to the nature of the breakage relative to `0.11` I hope this will be
considered for `0.12.1`.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this issue Jan 9, 2024
…#10609)

Bevy introduced unintentional breaking behaviour along with the v0.12.0
release regarding the `App::set_runner` API. See: bevyengine#10385, bevyengine#10389 for
details. We weren't able to catch this before release because this API
is only used internally in one or two places (the very places which
motivated the break).

This commit adds a regression test to help guarantee some expected
behaviour for custom runners, namely that `app::update` won't be called
before the runner has a chance to initialise state.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants