Re-work event loop run APIs: adds `run -> Result<>`, `run_ondemand` and `pump_events` #2767

rib · 2023-04-12T14:32:55Z

Overall this re-works the APIs for how an EventLoop is run across all backends, based on the recent loong discussion in #2706

See this higher-level "EventLoop 3.0" tracking issue too: #2900

The changes let us address several open issues and cover these use-cases, with varying portability caveats:

A portable run() API that consumes the EventLoop and runs the loop on the calling thread until the app exits. This can be supported across all platforms and compared to the previous run() -> ! API is now able to return a Result status on all platforms except iOS and Web. Fixes: Android: shouldn't call std::process:exit in EventLoop::run #2709
A less portable run_ondmand() API that covers the use case in Can't create event loop ondemand #2431 where applications need to be able to re-run a Winit application multiple times against a persistent EventLoop. This doesn't allow Window state to carry across separate runs of the loop, but does allow orthogonal re-instantiations of a gui application.

Available On: Windows, Linux, MacOS, Android
Incompatible with iOS, Web
Fixes: Can't create event loop ondemand #2431
A less portable (and on MacOS, likely less-optimal) pump_events() API that covers the use case in Winit run_return and Android does not get Resumed and Suspended #2706 and allows a Winit event loop to be embedded within an external loop {}. Applications call pump_events() once per iteration of their own external loop to dispatch all pending Winit events, without blocking the external loop.

Available On: Windows, Linux, MacOS, Android
Incompatible With: iOS, Web
Fixes: Winit run_return and Android does not get Resumed and Suspended #2706

Each method of running the loop will behave consistently in terms of how NewEvents(Init), Resumed and LoopDestroyed events are dispatched (so portable application code wouldn't necessarily need to have any awareness of which method of running the loop was being used)

In particular by moving away from run() -> ! we stop calling std::process::exit() internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their main() function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads).

run_return has been removed, and the overlapping use cases that run_return previously partially aimed to support have been split across run_ondemand and pump_events.

To help test the changes this adds:

  examples/window_ondemand
  examples/window_pump_events
  examples/window_pump_events_rfd

The last _rfd example, tests the interaction between the rfd crate and using pump_events to embed Winit within an external event loop, and confirms that #2752 is also fixed by this change.

Additionally all examples have generally been updated so that main() returns a Result from run()

Fixes: #2709
Fixes: #2706
Fixes: #2431
Fixes: #2752

Platform Details

Just some of the more pertinent notes from enabling this in different backends...

Windows

The changes in the Windows backend were more involved than I expected (I had originally assumed that pump_events was going to be very similar to run except would use PeekMessageW instead of GetMessageW to avoid blocking the external loop) but I realized that the Windows backend implementation really didn't match several of my assumptions.

Overall I think my changes can hopefully be considered a quite a significant simplification (I think it's a net deletion of a fair amount of code in the Windows backend) and I think it also helps bring it into slightly closer alignment with other backends too.

Key changes:

I have removed the wait_thread that was a fairly fiddly way of handling ControlFlow::WaitUntil timeouts in favor of using SetTimer which works with the same messages picked up by GetMessage and PeekMessage.
I have removed the ordering guarantees between MainEventsCleared, RedrawRequested and RedrawEventsCleared events due to the complexity in maintaining this artificial ordering, which is already not supported consistently across backends anyway (in particular this ordering already isn't compatible with how MacOS / iOS work).
RedrawRequested events are now directly dispatched via WM_PAINT messages - comparable to how RedrawRequested is dispatched via drawRect in the MacOS backend.
I have re-worked how NewEvents, MainEventsCleared, and RedrawEventsCleared get dispatched to be more in line with the MacOS backend and also more in line with how we have recently discussed defining them for all platforms. NewEvents is conceptually delivered when the event loop "wakes up" and MainEventsCleared gets dispatched when the event loop is about to ask the OS to wait for new events. This is a more portable model, and is already how these events work in the MacOS backend. RedrawEventsCleared are just delivered after MainEventsCleared but this event no longer has a useful meaning.

Probably the most controversial thing here is that this "breaks" the ordering rules for redraw event handling, but since my changes interacted with how the order is maintained I was very reluctant to figure out how to continue maintaining something that we have recently been discussing changing: #2640. Additionally, since the MacOS backend already doesn't strictly maintain this order it's somewhat academic to see this as a breakage if Winit applications can't really rely on it already.

MacOS

The implementation of pump_events essentially works by hooking into the RunLoopObserver and requesting that the app should be stopped the next time that the RunLoop prepares to wait for new events.

Originally I had thought I would poke the CFRunLoop for the app directly and I was originally going to implement pump_events based on a timeout which I'd seen SDL doing. I found that [NSApp run] wasn't actually being stopped by asking the RunLoop to stop directly and inferred that NSApp run will actually catch this and simply re-start the loop. I guess maybe SDL doesn't use [NSApp run], I'm not really sure.

Hooking into the observer and calling [NSApp stop] actually seems like a better solution that doesn't need a hacky constant timeout and is going to be more similar to how run_return would have worked before.

Android

Nothing particularly unusual / unexpected here, except to note that I decided to make run_ondemand public even though I couldn't previously imagine a use case for run_ondemand on Android. As it happens I recently started working on a project that coincidentally looks like it has a potential use case for run_ondemand that may also make sense to use Android.

X11

Nothing unusual. I think it may also fix an inconsistency with how the StartCause is set for WaitUntil control_flow

Wayland

I found the calloop abstraction a little awkward to work with while I was trying to understand why there was surprising workaround code in the wayland backend for manually dispatching pending events. Investigating this further it looks like there may currently be several issues with the calloop WaylandSource (with how prepare_read is used and with (not) flushing writes before polling)

Considering the current minimal needs for polling in all winit backends I do personally tend to think it would be simpler to just own the responsibility for polling more directly, so the logic for wayland-client prepare_read wouldn't be in a separate crate (and in this current situation would also be easier to fix)

I've tried to maintain the status quo with calloop + workarounds after the my initial suggestion to use mio and handle the prepare_read protocol directly was shot down pretty hard (on IRC).

Orbital

I've made blind, untested changes to the Orbital backend and for simplicity I haven't added support for run_ondemand or pump_events, I have just maintained support for the portable run API.

TODO

madsmtm

Thanks for taking the time to try improving the situation here. I've stated my views below, but in any case this PR is an improvement over the status quo, so approval from my side.

Haven't had the time to look much at the macOS implementation yet, but don't block on that.

src/event_loop.rs

src/platform_impl/macos/app_state.rs

src/platform_impl/macos/event_loop.rs

src/platform/run_ondemand.rs

src/platform/pump_events.rs

madsmtm · 2023-04-16T17:15:06Z

src/platform/mod.rs

 ))]
-pub mod run_return;
+pub mod pump_events;


As you said, this is non-performant on macOS, but it's also quite bug-prone. Like sure, an application using it will "work", for some definition of work, but it will not really work properly: When resizing, the event loop will not yield back to the user/developer, in the end leading to an application that feels very janky.

So ideally, I would like to avoid having this implemented for macOS, since I would never want anyone to use it.

run_ondemand sounds more reasonable to my ears, though I suspect that even then the better thing to do would be to recommend users to start a new process instead. As an example, try adding std::thread::sleep_ms(10000); between the two invocations; you'll see that it looks like the application is unresponsive, even though it might actually be doing something in background.

Although I generally agree that ideally pump_events wouldn't be exposed for macos, I think for now it's beneficial/pragmatic for existing applications that are dependent on [ab]using run_return to embed Winit into an external event loop.

Especially in the context of integrating / leveraging winit within existing codebases where there may be a willingness to accept some trade offs (such as janky resizing) if it avoids a bunch of upheaval, and e.g. for a game which is most likely going to run fullscreen.

E.g. I'm working with a game that aims to run across Windows, Linux, MacOS and Android based on an external loop that currently integrates Winit via run_return. This has been working well enough so far, except for some issues with rfd dialogs on macos and suspend/resume issues on Android.

I'm keen to at least migrate this project to using pump_events as a stepping stone that is at least a better solution for integrating with external loops, before looking at re-working to try and use run.

Since this project currently depends on run_return on macos being usable in a way that's similar to pump_events it would be really awkward if run_return where removed and no replacement was provided on macos. Unless we dropped macos support that would also mean we couldn't switch to using pump_events for the other platforms either (since this project doesn't support running in any other way currently, we can't fallback to using run on macos)

My hope is that when documenting pump_events we can try to make all these caveats and trade offs very clear to developers and hopefully make it clear that the API shouldn't generally be used unless you know it's the only practical option you have.

Yeah, those are all good points, and I would of course prefer an application that supports macOS over one that doesn't, even if it's janky.

And yeah, good documentation about the caveats (as well as maybe a note/policy that issues with it will have a lower priority) will probably mitigate most of my concerns.

I think some applications like games don't really resize that often, so for them pump_events will probably work, since they already use run_return for that.

rib · 2023-04-17T14:04:57Z

Thanks for taking the time to try improving the situation here. I've stated my views below, but in any case this PR is an improvement over the status quo, so approval from my side.

Haven't had the time to look much at the macOS implementation yet, but don't block on that.

Thanks for taking a look and for the feedback!

madsmtm · 2023-04-21T20:41:23Z

src/platform_impl/macos/view.rs

+    pub(super) fn request_redraw(&self) {
+        if is_main_thread() {
+            // Request a callback via `drawRect`
+            self.setNeedsDisplay(true);


This has been tried before, if I remember correct it is not enough to make it work in the case where the view is layer backed, see #2189 (comment). Have you tested it with wgpu?

(Also, things that you want done on the main thread should likely just be done as in platform_impl/macos/util/async.rs).

I would suggest fixing the RedrawRequested events in another PR, otherwise I suspect this one may drag out for long.

Ah, that's awkward.

Yeah, the general reason for the change was just to do with also wanting to use RedrawRequested as a hard interruption point for pump_events, as a mitigation factor for being able to spam input events that might block from returning to the external loop.

That strategy can still be used to cap how long pump_events runs without this change, it's just unfortunate that the RedrawRequested events aren't throttled in any way.

I'll revert this bit so it can be investigated in a separate PR like you say.

For metal it looks like you can also manually request redraws explicitly via https://developer.apple.com/documentation/metalkit/mtkview/1535993-enablesetneedsdisplay

I would guess that it could be ok that setNeedsDisplay is ignored with a metal/gl view by default, from the pov that you will instead get continuous, regular drawRect callbacks (so request_redraw can be a NOP) but it would be good to investigate this separately - it sounds like it's not that simple.

rib · 2023-06-18T21:13:38Z

Okey, I've done a big rebase, on top of the keyboard changes and the x11 calloop changes.

I've also tried to split this work into a logical sequence of smaller commits, that first add the pump_events and run_ondemand extension traits followed by patches to implement in each backend before removing the run_return extension.

The run() API change is then handled separately on top of that, along with updating and adding examples.

I'm going to mark this PR as ready for review since I've added some basic documentation now - though I still have some CHANGELOG entries to add.

I could potentially split this up into some separate PRs if that's preferable.

This re-works the portable `run()` API that consumes the `EventLoop` and runs the loop on the calling thread until the app exits. This can be supported across _all_ platforms and compared to the previous `run() -> !` API is now able to return a `Result` status on all platforms except iOS and Web. Fixes: rust-windowing#2709 By moving away from `run() -> !` we stop calling `std::process::exit()` internally as a means to kill the process without returning which means it's possible to return an exit status and applications can return from their `main()` function normally. This also fixes Android support where an Activity runs in a thread but we can't assume to have full ownership of the process (other services could be running in separate threads). Additionally all examples have generally been updated so that `main()` returns a `Result` from `run()` Fixes: rust-windowing#2709

A minimal example of an application based on an external event loop that calls `pump_events` for each iteration of the external loop.

A minimal example that shows an application running the event loop more than once via `run_ondemand` There is a 5 second delay between each run to help highlight problems with destroying the window from the first loop.

Although we document that applications can't keep windows between separate run_ondemand calls it's possible that the application has only just dropped their windows and we need to flush these requests to the server/compositor. This fixes the window_ondemand example - by ensuring the window from the first loop really is destroyed before waiting for 5 seconds and starting the second loop.

Considering the strict requirement that applications can't keep windows across run_ondemand calls, this tries to make the window_ondemand example explicitly wait for its Window to be destroyed before exiting each run_ondemand iteration. This updates the example to only `.set_exit()` after it gets a `Destroyed` event after the Window has been dropped. On Windows this works to ensure the Window is destroyed before the example waits for 5 seconds. Unfortunately though: 1. The Wayland backend doesn't emit `Destroyed` events for windows 2. The macOS backend emits `Destroyed` events before the window is really destroyed. and so the example isn't currently portable.

notgull

Overall nothing actually blocking from me.

src/platform_impl/linux/x11/mod.rs

rib · 2023-07-26T09:30:15Z

okey, I've hopefully addressed your latest comments @kchibisov.

There is just one about using .take() for the sake of squashing one line which I would rather leave as is ideally since I find the logical ordering looks surprising, like: mutate state, check outcome, possibly mutate more state - when the intent is check state, possibly mutate state

One unrelated change I also made was adding some docs about the timeout argument to pump_events which I had thought I had already done but found the staged change on my mac and realized I never committed the change, oops.

This layers pump_events on a pump_events_with_timeout API, like we have for Linux and Android. This is just an internal implementation detail for now but we could consider making pump_events_with_timeout public, or just making it so that pump_events() takes the timeout argument.

This renames all internal implementations of pump_events_with_timeout to pump_events and makes them public. Since all platforms that support pump_events support timeouts there's no need to have a separate API.

kchibisov · 2023-07-27T22:53:30Z

src/platform_impl/macos/observer.rs

-            EventLoopWaker { timer }
+            EventLoopWaker {
+                timer,
+                start_instant: Instant::now(),


I've removed the - because it was crashing, since you need to wait 1000 seconds from the boot to make it work, which I usually don't, the clock is in monotonic anyway, so if you call ::now later, this one will be in the past.

Ah, yeah, that makes sense

kchibisov · 2023-07-27T23:04:40Z

Thanks!

rib · 2023-07-27T23:42:21Z

Wooo, thanks!

Hoodad · 2023-07-28T03:36:22Z

Wow! It's finally in 🥳👍 Great work everyone 🎉

It seems that Xlib's event code ignores device events if I don't do this. Once rust-windowing#2767 is merged we can work around this issue.

rib requested review from jackpot51, msiglreith and madsmtm as code owners April 12, 2023 14:32

rib marked this pull request as draft April 12, 2023 14:33

rib force-pushed the run-ondemand-and-event-pumping branch from 128743e to c778d4a Compare April 14, 2023 08:12

madsmtm approved these changes Apr 16, 2023

View reviewed changes

rib force-pushed the run-ondemand-and-event-pumping branch 3 times, most recently from 6efa736 to 68a61df Compare April 21, 2023 16:08

madsmtm reviewed Apr 21, 2023

View reviewed changes

rib force-pushed the run-ondemand-and-event-pumping branch from 68a61df to be5ea0e Compare April 24, 2023 11:50

rib mentioned this pull request May 20, 2023

Using WM_PAINT for controlling the event loop is way too slow for real-time rendering #2782

Open

daxpedda mentioned this pull request Jun 7, 2023

Using custom super loop for managing events #2857

Closed

rib force-pushed the run-ondemand-and-event-pumping branch 2 times, most recently from 5eac04d to b0d9048 Compare June 18, 2023 21:02

rib marked this pull request as ready for review June 18, 2023 21:14

rib force-pushed the run-ondemand-and-event-pumping branch from b0d9048 to bd558d8 Compare June 18, 2023 21:23

rib mentioned this pull request Jun 22, 2023

Creating EventLoop multiple times is not supported. #2885

Closed

rib force-pushed the run-ondemand-and-event-pumping branch 9 times, most recently from 1c47138 to 32ba29a Compare June 22, 2023 21:57

rib added 7 commits July 26, 2023 01:30

Remove EventLoopExtRunReturn

5a80523

Add examples/window_pump_events

dc75dab

A minimal example of an application based on an external event loop that calls `pump_events` for each iteration of the external loop.

Add examples/window_ondemand

d32841b

A minimal example that shows an application running the event loop more than once via `run_ondemand` There is a 5 second delay between each run to help highlight problems with destroying the window from the first loop.

Update CHANGELOG.md

7215843

notgull approved these changes Jul 26, 2023

View reviewed changes

src/platform_impl/linux/x11/mod.rs Show resolved Hide resolved

src/platform_impl/linux/x11/mod.rs Show resolved Hide resolved

src/platform_impl/linux/x11/mod.rs Show resolved Hide resolved

rib force-pushed the run-ondemand-and-event-pumping branch from de9dd42 to aa2cb60 Compare July 26, 2023 00:36

rib requested a review from kchibisov July 26, 2023 09:30

rib added 3 commits July 28, 2023 02:46

Windows: implement pump_events_with_timeout internally

3f67f94

Add timeout argument to pump_events

f2bd7e6

This renames all internal implementations of pump_events_with_timeout to pump_events and makes them public. Since all platforms that support pump_events support timeouts there's no need to have a separate API.

kchibisov force-pushed the run-ondemand-and-event-pumping branch 2 times, most recently from 6e2ce9e to f2bd7e6 Compare July 27, 2023 22:51

kchibisov reviewed Jul 27, 2023

View reviewed changes

kchibisov approved these changes Jul 27, 2023

View reviewed changes

kchibisov merged commit ae9b02e into rust-windowing:master Jul 27, 2023
55 checks passed

zshehov mentioned this pull request Aug 4, 2023

eframe::Frame::close affects following eframe::run_native calls #3002

Closed

notgull added a commit to forkgull/winit that referenced this pull request Aug 5, 2023

Add back Xlib-Xinput2 device handling

0efdb15

It seems that Xlib's event code ignores device events if I don't do this. Once rust-windowing#2767 is merged we can work around this issue.

notgull added a commit to forkgull/winit that referenced this pull request Aug 5, 2023

Add back Xlib-Xinput2 device handling

47dcf52

It seems that Xlib's event code ignores device events if I don't do this. Once rust-windowing#2767 is merged we can work around this issue.

notgull added a commit to forkgull/winit that referenced this pull request Aug 6, 2023

Add back Xlib-Xinput2 device handling

cbcedc0

It seems that Xlib's event code ignores device events if I don't do this. Once rust-windowing#2767 is merged we can work around this issue.

Vrixyz mentioned this pull request Aug 24, 2023

Update winit dependency to 0.29 bevyengine/bevy#8745

Closed

11 tasks

notgull added a commit to forkgull/winit that referenced this pull request Aug 27, 2023

Add back Xlib-Xinput2 device handling

e2f3efd

It seems that Xlib's event code ignores device events if I don't do this. Once rust-windowing#2767 is merged we can work around this issue.

daxpedda mentioned this pull request Dec 24, 2023

Remove wrong documentation on EventLoop::run() #3314

Merged

madsmtm mentioned this pull request Jan 25, 2024

Remove EventLoopError::AlreadyRunning #3425

Merged

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Re-work event loop run APIs: adds `run -> Result<>`, `run_ondemand` and `pump_events` #2767

Re-work event loop run APIs: adds `run -> Result<>`, `run_ondemand` and `pump_events` #2767

rib commented Apr 12, 2023 •

edited

madsmtm left a comment

madsmtm Apr 16, 2023

rib Apr 17, 2023

madsmtm Apr 17, 2023

kchibisov Jun 23, 2023

rib commented Apr 17, 2023

madsmtm Apr 21, 2023 •

edited

rib Apr 21, 2023

rib Apr 21, 2023

rib commented Jun 18, 2023

notgull left a comment

rib commented Jul 26, 2023

kchibisov Jul 27, 2023 •

edited

rib Jul 27, 2023

kchibisov commented Jul 27, 2023

rib commented Jul 27, 2023

Hoodad commented Jul 28, 2023

Re-work event loop run APIs: adds run -> Result<>, run_ondemand and pump_events #2767

Re-work event loop run APIs: adds run -> Result<>, run_ondemand and pump_events #2767

Conversation

rib commented Apr 12, 2023 • edited

Platform Details

Windows

Key changes:

MacOS

Android

X11

Wayland

Orbital

TODO

madsmtm left a comment

Choose a reason for hiding this comment

madsmtm Apr 16, 2023

Choose a reason for hiding this comment

rib Apr 17, 2023

Choose a reason for hiding this comment

madsmtm Apr 17, 2023

Choose a reason for hiding this comment

kchibisov Jun 23, 2023

Choose a reason for hiding this comment

rib commented Apr 17, 2023

madsmtm Apr 21, 2023 • edited

Choose a reason for hiding this comment

rib Apr 21, 2023

Choose a reason for hiding this comment

rib Apr 21, 2023

Choose a reason for hiding this comment

rib commented Jun 18, 2023

notgull left a comment

Choose a reason for hiding this comment

rib commented Jul 26, 2023

kchibisov Jul 27, 2023 • edited

Choose a reason for hiding this comment

rib Jul 27, 2023

Choose a reason for hiding this comment

kchibisov commented Jul 27, 2023

rib commented Jul 27, 2023

Hoodad commented Jul 28, 2023

Re-work event loop run APIs: adds `run -> Result<>`, `run_ondemand` and `pump_events` #2767

Re-work event loop run APIs: adds `run -> Result<>`, `run_ondemand` and `pump_events` #2767

rib commented Apr 12, 2023 •

edited

madsmtm Apr 21, 2023 •

edited

kchibisov Jul 27, 2023 •

edited