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

fix: upgrade to winit v0.30 #13366

Open
wants to merge 59 commits into
base: main
Choose a base branch
from

Conversation

pietrosophya
Copy link
Contributor

@pietrosophya pietrosophya commented May 14, 2024

Objective

Solution

This is a rewrite/adaptation of the new trait system described and implemented in winit v0.30.

Migration Guide

The custom UserEvent is now renamed as WakeUp, used to wake up the loop if anything happens outside the app (a new custom_user_event shows this behavior.

The internal UpdateState has been removed and replaced internally by the AppLifecycle. When changed, the AppLifecycle is sent as an event.

The UpdateMode now accepts only two values: Continuous and Reactive, but the latter exposes 3 new properties to enable reactive to device, user or window events. The previous UpdateMode::Reactive is now equivalent to UpdateMode::reactive(), while UpdateMode::ReactiveLowPower to UpdateMode::reactive_low_power().

The ApplicationLifecycle has been renamed as AppLifecycle, and now contains the possible values of the application state inside the event loop:

  • Idle: the loop has not started yet
  • Running (previously called Started): the loop is running
  • WillSuspend: the loop is going to be suspended
  • Suspended: the loop is suspended
  • WillResume: the loop is going to be resumed

Note: the Resumed state has been removed since the resumed app is just running.

Finally, now that winit enables this, it extends the WinitPlugin to support custom events.

Test platforms

  • Windows
  • MacOs
  • Linux (x11)
  • Linux (Wayland)
  • Android
  • iOS
  • WASM/WebGPU
  • WASM/WebGL2

Outstanding issues / regressions

  • iOS: build failed in CI
    • blocking, but may just be flakiness
  • Cross-platform: when the window is maximised, changes in the scale factor don't apply, to make them apply one has to make the window smaller again. (Re-maximising keeps the updated scale factor)
    • non-blocking, but good to fix
  • Android: it's pretty easy to quickly open and close the app and then the music keeps playing when suspended.
    • non-blocking but worrying
  • Web: the application will hang when switching tabs
  • Cross-platform?: Screenshot failure, ERROR present_frames: wgpu_core::present: No work has been submitted for this frame before taking the first screenshot, but after pressing space
    • non-blocking, but good to fix

@alice-i-cecile alice-i-cecile added this to the 0.14 milestone May 14, 2024
@alice-i-cecile alice-i-cecile added A-Windowing Platform-agnostic interface layer to run your app in C-Dependencies A change to the crates that Bevy depends on D-Complex Quite challenging from either a design or technical perspective. Ask for help! X-Uncontroversial This work is generally agreed upon S-Needs-Testing Testing must be done before this is safe to merge labels May 14, 2024
@alice-i-cecile
Copy link
Member

Does anyone have opinions on squeezing this into 0.14 vs merging it at the start of 0.15? On the one hand fixes are great, on the other hand these upgrade PRs are consistently incredibly painful to review and introduce a large number of new bugs that must be manually tested.

@pietrosophya
Copy link
Contributor Author

IMHO, this could be finished and merged as part of 0.14: I only moved code from free variables and functions inside the WinitAppRunnerState, not really changing any logic.

Also, apart from testing it, the number of missing/required changes is not high: I just need to understand better the AccessKit part, restore the exit flags (that aren't currently working) and compile/test the various platforms.

The low_power example (and I believe others too) in MacOS works already.

@hymm
Copy link
Contributor

hymm commented May 14, 2024

I think android support is broken without the update. See this message from Francois #13254 (comment)

@pietrosophya pietrosophya marked this pull request as ready for review May 15, 2024 14:50
@happydpc
Copy link

I have tested on webgl2, the application will hang when switched tabs. Something like this #13486

@tychedelia
Copy link
Contributor

scale_factor_override

* When the window is maximised, changes in the scale factor don't apply, to make them apply one has to make the window smaller again. (Re-maximising keeps the updated scale factor)

Can reproduce on macOS:

Screenshots:

When full screen, text is wonky
image
Minimizing the window then fixes
image

@alice-i-cecile
Copy link
Member

alice-i-cecile commented May 26, 2024

I've asked for help with this PR over in the winit Matrix chat: link.

@eero-lehtinen
Copy link
Contributor

eero-lehtinen commented May 26, 2024

On android, the app always crashes for me when resuming and starts from the beginning.

I did some more testing on Android and it seems that the crashing might be a MIUI quirk. After clearing other background apps, the crashing went away. Sorry for the noise.

Also it's pretty easy to quickly open and close the app and then the music keeps playing when suspended.

I also did some more investigation on this. It is still an issue. It seems that the suspended-function is just never called by winit if the app is closed early enough (at least on my device with Android 11). I don't know if we can do much about it. It is a regression, but maybe not worth blocking over imo. Otherwise suspension works fine.

ApplicationLifetime::Suspended => music_controller.single().pause(),
ApplicationLifetime::Resumed => music_controller.single().play(),
ApplicationLifetime::Started => (),
AppLifecycle::Idle | AppLifecycle::WillSuspend | AppLifecycle::WillResume => {}
Copy link
Contributor

Choose a reason for hiding this comment

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

Should these be included in the event enum? In my testing none of these three are ever sent.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Idle is correct (probably?) because it happens when the app just started only (and it's never reset). The other 2 though should happen but I could have missed sending them.

I have no opinions on sending Idle, it seems useless apart from initializing the state.

Copy link
Contributor

Choose a reason for hiding this comment

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

Idle is never sent because lifecycle events are only sent when changed, and Idle is never changed into. WillSuspend and WillResume events are never sent because the lifecycle is always immediately changed to Suspend or Running before sending the change.

Sending idle seems pretty useless anyway when it is the default and never seen after startup.

@eero-lehtinen
Copy link
Contributor

eero-lehtinen commented May 26, 2024

I also tested with opt-level=3 and lto="thin", and then the time window for making the music bug happen is very very small. I couldn't make it happen in practice, maybe it would be easier on a slower device or with more stuff happening in startup.

Edit: tried with 4 second sleep in the setup_music function and easily reproduced the bug with even with optimizations. Would it somehow be possible to listen to the suspension while the Startup schedule is running?

@pietrosophya
Copy link
Contributor Author

scale_factor_override

* When the window is maximised, changes in the scale factor don't apply, to make them apply one has to make the window smaller again. (Re-maximising keeps the updated scale factor)

Can reproduce on macOS:

I think this is related to 2 combined things happening:

  1. the scale factor override is only a Bevy internal setup, and it's set in the WindowResolution property of the Bevy Window. There is no counterpart in winit. So the event loop doesn't really react to the change, and a custom event should be emitted instead. It can be handled in the bevy_winit::changed_windows system.
  2. winit can send an event for a changed scale factor (easy to repro changing the browser zoom in a WASM app) and the relative event is handled. This is also a source of a bug related to this even in 0.13 (that's a winit one, but can be solved in Bevy too).

I can handle both in this PR or in a follow up one in the next couple days.

@pietrosophya
Copy link
Contributor Author

I fixed the scale factor override issue and solved this 🤞

@IQuick143
Copy link
Contributor

Can confirm it works!

@alice-i-cecile alice-i-cecile added S-Needs-Review Needs reviewer attention (from anyone!) to move forward S-Needs-Testing Testing must be done before this is safe to merge and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 29, 2024
@pietrosophya
Copy link
Contributor Author

@mockersf is there an easy way to test the IOS build that's failing? it seems the only blocker to merge this PR now...

@alice-i-cecile alice-i-cecile removed the S-Needs-Review Needs reviewer attention (from anyone!) to move forward label May 30, 2024
@alice-i-cecile alice-i-cecile added the C-Needs-Release-Note Work that should be called out in the blog due to impact label May 31, 2024
@mockersf
Copy link
Member

mockersf commented Jun 1, 2024

Could you give me more details on the iOS build failure? I don't see it in CI, I'll try to reproduce tomorrow

also, another patch update please: Sophya#7

@tychedelia
Copy link
Contributor

Could you give me more details on the iOS build failure? I don't see it in CI, I'll try to reproduce tomorrow

Seems like iOS is skipped? This happened last time it was added to the merge queue: https://github.com/bevyengine/bevy/actions/runs/9215755498/job/25354702941

@@ -2,7 +2,7 @@

DEVICE = ${DEVICE_ID}
ifndef DEVICE_ID
DEVICE=$(shell xcrun simctl list devices 'iOS' | grep -v 'unavailable' | grep -v '^--' | grep -v '==' | head -n 1 | grep -E -o -i "([0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12})")
DEVICE=$(shell xcrun simctl list devices 'iOS' | grep -v 'unavailable' | grep -v 'Shutdown' | grep -v '^--' | grep -v '==' | head -n 1 | grep -E -o -i "([0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12})")
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
DEVICE=$(shell xcrun simctl list devices 'iOS' | grep -v 'unavailable' | grep -v 'Shutdown' | grep -v '^--' | grep -v '==' | head -n 1 | grep -E -o -i "([0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12})")
DEVICE=$(shell xcrun simctl list devices 'iOS' | grep -v 'unavailable' | grep -v '^--' | grep -v '==' | head -n 1 | grep -E -o -i "([0-9a-f]{8}-([0-9a-f]{4}-){3}[0-9a-f]{12})")

Copy link
Member

@mockersf mockersf Jun 2, 2024

Choose a reason for hiding this comment

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

this change stops CI from finding the simulator and is unrelated to this PR

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-Dependencies A change to the crates that Bevy depends on C-Needs-Release-Note Work that should be called out in the blog due to impact D-Complex Quite challenging from either a design or technical perspective. Ask for help! S-Needs-Testing Testing must be done before this is safe to merge X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

bevy mobile example crashes on Android
10 participants