fix: rewrite winit loop

[pietrosophya]

Objective

• Simplifies/clarifies the winit loop.
• Fixes CPU spikes when running the unfocused app in web  #12612.

Solution

The Winit loop runs following this flow:

• NewEvents
• Any number of other events, that can be 0, including RequestRedraw
• AboutToWait

Bevy also uses the UpdateMode, to define how the next loop has to run. It can be essentially:

• Continuous, using ControlFlow::Wait for windowed apps, and ControlFlow::Poll for windowless apps
• Reactive/ReactiveLowPower, using ControlFlow::WaitUntil with a specific wait delay

The changes are made to follow this pattern, so that

• NewEvents define if the WaitUntil has been canceled because we received a Winit event.
• AboutToWait:
  • checks if the window has to be redrawn
  • otherwise calls app.update() if the WaitUntil timeout has elapsed
  • updates the ControlFlow accordingly

To make the code more logical:

• AboutToWait checks if any Bevy's RequestRedraw event has been emitted
• create_windows is run every cycle, at the beginning of the loop
• the ActiveState (that could be renamed ActivityState) is updated in AboutToWait, symmetrically for WillSuspend/WillResume
• the AppExit events are checked every loop cycle, to exit the app early

Platform-specific testing

• Windows
• MacOs
• Linux (x11)
• Linux (Wayland)
• Android
• iOS
• WASM/WebGL2 (Chrome)
• WASM/WebGL2 (Firefox)
• WASM/WebGL2 (Safari)
• WASM/WebGpu (Chrome)

[alice-i-cecile]

Does this also fix #12106?

[pietrosophya]

I don't have a Windows machine unfortunately.

I tested these successfully on MacOS:

• the game_menu example (using the Quit button)
• the ecs_guide example (windowless, it exits correctly when the game ends)
• the low_power example (using both CTRL+C from the command line, and the window close button)
• the desk_toy example (right click on the logo)

[BD103]

I tested this on my M1 Macbook, both by using ci and by running random examples. It all seems to work!

[alice-i-cecile]

@pietrosophya I've added a section on platform-specific testing to the PR description: please feel free to update the checklist as reviewer reports come in :)

[pietrosophya]

@alice-i-cecile thank you! I also tested WASM on both Chrome and Firefox, do I have to update the list or is it for reviewers only to check platforms off?

[alice-i-cecile]

Go ahead and check this off yourself :) I've also added Safari: I forgot that its web support is also different and weird.

[mockersf]

wasm/webgl2 support is mostly the same across browsers for the event loop, but wasm/webgpu needs to be tested too

[hymm]

Tested on windows 11 with most of the window example. Seems to work fine.

[kristoff3r]

Tested every example relating to windows and a few other ones on Linux with X11 and XMonad, no issues found.

[wolf-in-space]

Tested the window examples on Linux with Wayland and Sway, with the Wayland feature enabled and disabled: Only issue i noticed was transparent_window not working when the Wayland feature is enabled, but the issue is the same on main / 13.1, was also mentioned in #10929 already. Noticed no issues from this pr.

[pietrosophya]

I fixed the suspension in Android, I had to test it on a previous forked version because of this. I took another refactoring opportunity and moved some comments to clarify them.

@eero-lehtinen could you try android again?

[eero-lehtinen]

It works perfectly for me now on Android.

[pietrosophya]

Tested on the iOS emulator, it works well (suspending and resuming music too).

[eero-lehtinen]

winit 0.30 was released, so maybe we need more rewrites :D

[mockersf]

looks OK to me, except for an enum name. Could you change it and resolve conflicts?

@Maximetinu I'm requesting a new review as the PR changed, please re-review if you can

[Maximetinu]

@Maximetinu I'm requesting a new review as the PR changed, please re-review if you can

Done! It still LGTM, approved ✅

There are some more ideas to clean up this part of the code that I think are interesting to consider, but they may be out of scope for this PR, specially if we don't want to introduce breaking changes:

• The difference between UpdateModes Reactive and ReactiveLowPower is only the kind of events (window and/or device) that should trigger an update, so we could merge them into a single Reactive, with 2 booleans to expose this customizability. Maybe even Continuous could eventually converge into the same type as well. To keep the same current simplicity to the user, we could expose 2 different constructors for UpdateMode: UpdateMode::reactive() and UpdateMode::reactive_low_power() in the same way we have WinitSettings::desktop_app() and WinitSettings::game(). The reason for this is that the user may not want to update due to window or device events at all, so the update is controlled manually in some other way, like from an external host app by a request redraw user event through the event loop proxy.

• The fact that the UserEvent of the Event Loop Proxy is named RequestRedraw makes it easy to confuse with the WindowEvent::RedrawRequested. I often mix that one up with Event::UserEvent(RequestRedraw). In fact, since the RequestRedraw event can now be sent through the Event Loop Proxy, should it still be an ECS event at all? From the user POV, we now have 2 different ways to request a redraw: through the proxy or through the ECS command, and it's unclear to me if the 2 are exactly the same or not. But I don't have an strong opinion on how to solve this or make this clearer tbh.

[pietrosophya]

@mockersf I resolved conflicts and changed the name. I'll create another PR after this is merged to make this new UpdateState and the ApplicationLifecycle converge since they mean the same thing.

[alice-i-cecile]

I'm happy with the improvements to code quality and bug fixes here, and think this is adequately tested. I'm going to leave an approval and get this merged no later than next Monday to ensure that it gets into the hands of folks to test.

[alice-i-cecile]

error[E0277]: the trait bound Result<RawHandleWrapper, HandleError>: bevy_ecs::bundle::Bundle is not satisfied

https://github.com/bevyengine/bevy/actions/runs/8929016345/job/24525847476

@pietrosophya once that's fixed ping me and I'll retry the merge.

[pietrosophya]

@alice-i-cecile argh, sorry about that, it's fixed now.