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

Prevent black frames during startup #9826

Merged
merged 6 commits into from
Oct 18, 2023

Conversation

arendjr
Copy link
Contributor

@arendjr arendjr commented Sep 16, 2023

Objective

This PR addresses the issue where Bevy displays one or several black frames before the scene is first rendered. This is particularly noticeable on iOS, where the black frames disrupt the transition from the launch screen to the game UI. I have written about my search to solve this issue on the Bevy discord: https://discord.com/channels/691052431525675048/1151047604520632352

While I can attest this PR works on both iOS and Linux/Wayland (and even seems to resolve a slight flicker during startup with the latter as well), I'm not familiar enough with Bevy to judge the full implications of these changes. I hope a reviewer or tester can help me confirm whether this is the right approach, or what might be a cleaner solution to resolve this issue.

Solution

I have moved the "startup phase" as well as the plugin finalization into the app.run() function so those things finish synchronously before the "main schedule" starts. I even move one frame forward as well, using app.update(), to make sure the rendering has caught up with the state of the finalized plugins as well.

I admit that part of this was achieved through trial-and-error, since not doing the "startup phase" before app.finish() resulted in panics, while not calling an extra app.update() didn't fully resolve the issue.

What I can say, is that the iOS launch screen animation works in such a way that the OS initiates the transition once the framework's didFinishLaunching() returns, meaning app developers must finish setting up their UI before that function returns. This is what basically led me on the path to try to "finish stuff earlier" :)

Changelog

Changed

  • The startup phase and the first frame are rendered synchronously when calling app.run(), before the "main schedule" is started. This fixes black frames during the iOS launch transition and possible flickering on other platforms, but may affect initialization order in your application.

Migration Guide

Because of this change, the timing of the first few frames might have changed, and I think it could be that some things one may expect to be initialized in a system may no longer be. To be honest, I feel out of my depth to judge the exact impact here.

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Windowing Platform-agnostic interface layer to run your app in labels Sep 16, 2023
@alice-i-cecile
Copy link
Member

This looks like it's solving the problem that #9692 supplied a work-around for, but in a more elegant way. If it does, can you revert that change as part of this PR?

@arendjr
Copy link
Contributor Author

arendjr commented Sep 16, 2023

@alice-i-cecile Oh yes, someone pointed me to the fix from #9692 on Discord, although it did not solve the iOS issue. I don't think it resolved the flicker I witnessed on Wayland either, but maybe it was trying to? I can revert it and see if I notice anything off, but I don't have a Windows machine to test on, so in that case maybe @hymm could verify if it still works correctly there (I see they tested the previous fix on Win 11)? Just let me know how you would like me to proceed :)

@arendjr
Copy link
Contributor Author

arendjr commented Sep 16, 2023

Yikes, those failing DX12 examples does seem to be an issue with this branch :(

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Sep 16, 2023
@mockersf
Copy link
Member

This should be tested on the other platforms that are peculiar about setup, mostly android and wasm/WebGPU

@IceSentry
Copy link
Contributor

Doesn't work at all for me in windows 10.

    Finished dev [unoptimized + debuginfo] target(s) in 0.51s
     Running `target\debug\examples\3d_scene.exe`
2023-09-16T18:18:07.189173Z  INFO bevy_winit::system: Creating new window "App" (0v0)
2023-09-16T18:18:07.644783Z  INFO bevy_diagnostic::system_information_diagnostics_plugin::internal: SystemInfo { os: "Windows 10 Pro", kernel: "19045", cpu: "AMD Ryzen 7 5800X 8-Core Processor", core_count: "8", memory: "63.9 GiB" }
2023-09-16T18:18:08.024971Z  INFO bevy_render::renderer: AdapterInfo { name: "NVIDIA GeForce RTX 4080", vendor: 4318, device: 9988, device_type: DiscreteGpu, driver: "NVIDIA", driver_info: "537.34", backend: Vulkan }
thread 'Compute Task Pool (6)' panicked at 'called `Option::unwrap()` on a `None` value', crates\bevy_render\src\pipelined_rendering.rs:144:84
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
error: process didn't exit successfully: `target\debug\examples\3d_scene.exe` (exit code: 101)

@arendjr
Copy link
Contributor Author

arendjr commented Sep 16, 2023

Ah, that looks very similar to the CI failure for DX12. I don’t have Windows, but maybe the location of the panic provides a hint.

@arendjr
Copy link
Contributor Author

arendjr commented Sep 16, 2023

So it would appear the RenderToMainAppReceiver resource is not added yet:

let receiver = world.get_resource::<RenderToMainAppReceiver>().unwrap();

Need to figure out why though…

@hymm
Copy link
Contributor

hymm commented Sep 16, 2023

You can't run the schedule before finish.

// Force plugins to finish their setup and advance by one frame to make sure everything is
// setup up. This is important to prevent black frames during the launch animation on iOS,
// but it seems to also solve a brief flicker during startup on Linux/Wayland.
app.finish();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can't call app.finish() until app.ready() will return true

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I may have misunderstood then. I thought finish() would run the plugin initialization to completion so that ready() will return true afterwards.

Then what would be the recommended way to wait for ready() to return true?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bevy waits for ready() by starting the winit event loop and running it until it's true, which is probably what you're trying to work around on iOS

Copy link
Contributor Author

@arendjr arendjr Sep 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see. You’re right I must work around that though, because once control is handed over to the event loop it means iOS’ didFinishLaunching returns and the transition starts. That’s not good, since everything must be ready synchronously or I’m in trouble and the black frames are back.

It makes me wonder though, does that mean some plugins need an event loop in order to become ready? If yes, is it just a few? I suppose, since I didn’t run into issues on iOS and Linux. And would it be possible to adjust those plugins?

Or if no, why start the event loop at all that before plugins are ready?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is mandatory for webgpu support, for renderer initialization, and was done on all platforms to keep it the same.

In wgpu, the renderer init is async on all platforms, but the actual duration varies. I guess on iOS and Linux (and maybe then, not on all devices) it can be fast enough

Copy link
Member

@mockersf mockersf Sep 17, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you try #9830?

this PR should make ready return true directly on native, so you'll be able to do something like

if app.ready() {
  .. your code changes
}

you may still need to run startup even when not ready, or reverting that part of your changes

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that's very helpful! I will give it a shot tonight!

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, that patch works fine with this PR! I've cherry-picked it into this one too to see if those suggestions together will also fix the CI.

Copy link
Contributor Author

@arendjr arendjr Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CI is still red, but I just tested it, and it appears that with your change it's enough to only call .finish() and .update() and I don't need to force startup() anymore, so I'll indeed revert that part.

@arendjr
Copy link
Contributor Author

arendjr commented Sep 18, 2023

@hymm I've reverted that part of PR, but unfortunately it's still breaking on Windows. Would you maybe have another suggestion?

@arendjr
Copy link
Contributor Author

arendjr commented Sep 18, 2023

I suppose worst case I could just put a #[cfg(platform = "ios")] guard around my change. It would make sure this doesn't affect anything else, but it's a bummer since it might introduce other platform-dependent behaviors in people's applications, I'm afraid.

Comment on lines 295 to 296
app.finish();
app.update();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you need to call app.cleanup() between those two call, but that can be called only once and it's already called in winit run loop, so you'll have to handle that

@arendjr
Copy link
Contributor Author

arendjr commented Sep 18, 2023

All green now. Thanks for all the help, @mockersf !

@arendjr
Copy link
Contributor Author

arendjr commented Sep 19, 2023

Btw, I was wondering. Maybe I should take the finished_and_setup_done boolean from the winit plugin and let it be passed as an argument to the runner instead. It would save a call to app.ready(), but more importantly it would make the contract a bit more explicit and would provide a place to document what behavior is expected from the runners with regards to the setup.

@mockersf
Copy link
Member

mockersf commented Oct 8, 2023

looks good! could you solve conflicts with main?

@arendjr
Copy link
Contributor Author

arendjr commented Oct 9, 2023

@mockersf I've rebased on top of main. Should be all good now.

@alice-i-cecile
Copy link
Member

@mockersf, do you think this change should be controversial now?

Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tested this on windows with vulkan and DX12 and it doesn't remove any empty frames but other than that it runs without any errors. So assuming it actually works as intended on iOS LGTM

@mockersf
Copy link
Member

mockersf commented Oct 9, 2023

@mockersf I've rebased on top of main. Should be all good now.

Thanks!

@mockersf, do you think this change should be controversial now?

In my opinion no. It's now just a fasttrack on native platforms. I want to check it works fine on WebGL2/WebGPU/iOS and Android, but I don't see a reason it shouldn't.

In theory there's a small risk that app.ready() returns a different result between the two calls. I'll maybe open a followup PR after to make me feel better.

@alice-i-cecile alice-i-cecile removed the X-Controversial There is active debate or serious implications around merging this PR label Oct 9, 2023
@mockersf mockersf added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Oct 18, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Oct 18, 2023
Merged via the queue into bevyengine:main with commit 5d110eb Oct 18, 2023
22 checks passed
@mockersf mockersf mentioned this pull request Oct 19, 2023
github-merge-queue bot pushed a commit that referenced this pull request Oct 23, 2023
# Objective

- After #9826, there are issues on "run once runners"
- example `without_winit` crashes:
```
2023-10-19T22:06:01.810019Z  INFO bevy_render::renderer: AdapterInfo { name: "llvmpipe (LLVM 15.0.7, 256 bits)", vendor: 65541, device: 0, device_type: Cpu, driver: "llvmpipe", driver_info: "Mesa 23.2.1 - kisak-mesa PPA (LLVM 15.0.7)", backend: Vulkan }
2023-10-19T22:06:02.860331Z  WARN bevy_audio::audio_output: No audio device found.
2023-10-19T22:06:03.215154Z  INFO bevy_diagnostic::system_information_diagnostics_plugin::internal: SystemInfo { os: "Linux 22.04 Ubuntu", kernel: "6.2.0-1014-azure", cpu: "Intel(R) Xeon(R) CPU E5-2673 v3 @ 2.40GHz", core_count: "2", memory: "6.8 GiB" }
thread 'main' panicked at crates/bevy_render/src/pipelined_rendering.rs:91:14:
Unable to get RenderApp. Another plugin may have removed the RenderApp before PipelinedRenderingPlugin
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```
- example `headless` runs the app twice with the `run_once` schedule

## Solution

- Expose a more complex state of an app than just "ready"
- Also block adding plugins to an app after it has finished or cleaned
up its plugins as that wouldn't work anyway

## Migration Guide

* `app.ready()` has been replaced by `app.plugins_state()` which will
return more details on the current state of plugins in the app
tim-blackbird added a commit to tim-blackbird/bevy that referenced this pull request Nov 5, 2023
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
# Objective

This PR addresses the issue where Bevy displays one or several black
frames before the scene is first rendered. This is particularly
noticeable on iOS, where the black frames disrupt the transition from
the launch screen to the game UI. I have written about my search to
solve this issue on the Bevy discord:
https://discord.com/channels/691052431525675048/1151047604520632352

While I can attest this PR works on both iOS and Linux/Wayland (and even
seems to resolve a slight flicker during startup with the latter as
well), I'm not familiar enough with Bevy to judge the full implications
of these changes. I hope a reviewer or tester can help me confirm
whether this is the right approach, or what might be a cleaner solution
to resolve this issue.

## Solution

I have moved the "startup phase" as well as the plugin finalization into
the `app.run()` function so those things finish synchronously before the
"main schedule" starts. I even move one frame forward as well, using
`app.update()`, to make sure the rendering has caught up with the state
of the finalized plugins as well.

I admit that part of this was achieved through trial-and-error, since
not doing the "startup phase" *before* `app.finish()` resulted in
panics, while not calling an extra `app.update()` didn't fully resolve
the issue.

What I *can* say, is that the iOS launch screen animation works in such
a way that the OS initiates the transition once the framework's
[`didFinishLaunching()`](https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1622921-application)
returns, meaning app developers **must** finish setting up their UI
before that function returns. This is what basically led me on the path
to try to "finish stuff earlier" :)

## Changelog

### Changed

- The startup phase and the first frame are rendered synchronously when
calling `app.run()`, before the "main schedule" is started. This fixes
black frames during the iOS launch transition and possible flickering on
other platforms, but may affect initialization order in your
application.

## Migration Guide

Because of this change, the timing of the first few frames might have
changed, and I think it could be that some things one may expect to be
initialized in a system may no longer be. To be honest, I feel out of my
depth to judge the exact impact here.
ameknite pushed a commit to ameknite/bevy that referenced this pull request Nov 6, 2023
# Objective

- After bevyengine#9826, there are issues on "run once runners"
- example `without_winit` crashes:
```
2023-10-19T22:06:01.810019Z  INFO bevy_render::renderer: AdapterInfo { name: "llvmpipe (LLVM 15.0.7, 256 bits)", vendor: 65541, device: 0, device_type: Cpu, driver: "llvmpipe", driver_info: "Mesa 23.2.1 - kisak-mesa PPA (LLVM 15.0.7)", backend: Vulkan }
2023-10-19T22:06:02.860331Z  WARN bevy_audio::audio_output: No audio device found.
2023-10-19T22:06:03.215154Z  INFO bevy_diagnostic::system_information_diagnostics_plugin::internal: SystemInfo { os: "Linux 22.04 Ubuntu", kernel: "6.2.0-1014-azure", cpu: "Intel(R) Xeon(R) CPU E5-2673 v3 @ 2.40GHz", core_count: "2", memory: "6.8 GiB" }
thread 'main' panicked at crates/bevy_render/src/pipelined_rendering.rs:91:14:
Unable to get RenderApp. Another plugin may have removed the RenderApp before PipelinedRenderingPlugin
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```
- example `headless` runs the app twice with the `run_once` schedule

## Solution

- Expose a more complex state of an app than just "ready"
- Also block adding plugins to an app after it has finished or cleaned
up its plugins as that wouldn't work anyway

## Migration Guide

* `app.ready()` has been replaced by `app.plugins_state()` which will
return more details on the current state of plugins in the app
github-merge-queue bot pushed a commit that referenced this pull request 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`.
github-merge-queue bot pushed a commit that referenced this pull request Nov 26, 2023
# Objective

- Window size, scale and position are not correct on the first execution
of the systems
- Fixes #10407,  fixes #10642

## Solution

- Don't run `update` before we get a chance to create the window in
winit
- Finish reverting #9826 after #10389
cart pushed a commit that referenced this pull request 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`.
cart pushed a commit that referenced this pull request Nov 30, 2023
# Objective

- Window size, scale and position are not correct on the first execution
of the systems
- Fixes #10407,  fixes #10642

## Solution

- Don't run `update` before we get a chance to create the window in
winit
- Finish reverting #9826 after #10389
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

This PR addresses the issue where Bevy displays one or several black
frames before the scene is first rendered. This is particularly
noticeable on iOS, where the black frames disrupt the transition from
the launch screen to the game UI. I have written about my search to
solve this issue on the Bevy discord:
https://discord.com/channels/691052431525675048/1151047604520632352

While I can attest this PR works on both iOS and Linux/Wayland (and even
seems to resolve a slight flicker during startup with the latter as
well), I'm not familiar enough with Bevy to judge the full implications
of these changes. I hope a reviewer or tester can help me confirm
whether this is the right approach, or what might be a cleaner solution
to resolve this issue.

## Solution

I have moved the "startup phase" as well as the plugin finalization into
the `app.run()` function so those things finish synchronously before the
"main schedule" starts. I even move one frame forward as well, using
`app.update()`, to make sure the rendering has caught up with the state
of the finalized plugins as well.

I admit that part of this was achieved through trial-and-error, since
not doing the "startup phase" *before* `app.finish()` resulted in
panics, while not calling an extra `app.update()` didn't fully resolve
the issue.

What I *can* say, is that the iOS launch screen animation works in such
a way that the OS initiates the transition once the framework's
[`didFinishLaunching()`](https://developer.apple.com/documentation/uikit/uiapplicationdelegate/1622921-application)
returns, meaning app developers **must** finish setting up their UI
before that function returns. This is what basically led me on the path
to try to "finish stuff earlier" :)

## Changelog

### Changed

- The startup phase and the first frame are rendered synchronously when
calling `app.run()`, before the "main schedule" is started. This fixes
black frames during the iOS launch transition and possible flickering on
other platforms, but may affect initialization order in your
application.

## Migration Guide

Because of this change, the timing of the first few frames might have
changed, and I think it could be that some things one may expect to be
initialized in a system may no longer be. To be honest, I feel out of my
depth to judge the exact impact here.
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request Jan 9, 2024
# Objective

- After bevyengine#9826, there are issues on "run once runners"
- example `without_winit` crashes:
```
2023-10-19T22:06:01.810019Z  INFO bevy_render::renderer: AdapterInfo { name: "llvmpipe (LLVM 15.0.7, 256 bits)", vendor: 65541, device: 0, device_type: Cpu, driver: "llvmpipe", driver_info: "Mesa 23.2.1 - kisak-mesa PPA (LLVM 15.0.7)", backend: Vulkan }
2023-10-19T22:06:02.860331Z  WARN bevy_audio::audio_output: No audio device found.
2023-10-19T22:06:03.215154Z  INFO bevy_diagnostic::system_information_diagnostics_plugin::internal: SystemInfo { os: "Linux 22.04 Ubuntu", kernel: "6.2.0-1014-azure", cpu: "Intel(R) Xeon(R) CPU E5-2673 v3 @ 2.40GHz", core_count: "2", memory: "6.8 GiB" }
thread 'main' panicked at crates/bevy_render/src/pipelined_rendering.rs:91:14:
Unable to get RenderApp. Another plugin may have removed the RenderApp before PipelinedRenderingPlugin
note: run with `RUST_BACKTRACE=1` environment variable to display a backtrace
```
- example `headless` runs the app twice with the `run_once` schedule

## Solution

- Expose a more complex state of an app than just "ready"
- Also block adding plugins to an app after it has finished or cleaned
up its plugins as that wouldn't work anyway

## Migration Guide

* `app.ready()` has been replaced by `app.plugins_state()` which will
return more details on the current state of plugins in the app
rdrpenguin04 pushed a commit to rdrpenguin04/bevy that referenced this pull request 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`.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior 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.

5 participants