Re-work event loop run APIs: adds run -> Result<>, run_ondemand and pump_events#2767
Conversation
128743e to
c778d4a
Compare
madsmtm
left a comment
There was a problem hiding this comment.
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.
| orbital_platform | ||
| ))] | ||
| pub mod run_return; | ||
| pub mod pump_events; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
Thanks for taking a look and for the feedback! |
6efa736 to
68a61df
Compare
src/platform_impl/macos/view.rs
Outdated
| pub(super) fn request_redraw(&self) { | ||
| if is_main_thread() { | ||
| // Request a callback via `drawRect` | ||
| self.setNeedsDisplay(true); |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
68a61df to
be5ea0e
Compare
5eac04d to
b0d9048
Compare
|
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 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. |
b0d9048 to
bd558d8
Compare
1c47138 to
32ba29a
Compare
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
left a comment
There was a problem hiding this comment.
Overall nothing actually blocking from me.
de9dd42 to
aa2cb60
Compare
|
okey, I've hopefully addressed your latest comments @kchibisov. There is just one about using One unrelated change I also made was adding some docs about the |
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.
6e2ce9e to
f2bd7e6
Compare
| EventLoopWaker { timer } | ||
| EventLoopWaker { | ||
| timer, | ||
| start_instant: Instant::now(), |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Ah, yeah, that makes sense
|
Thanks! |
|
Wooo, thanks! |
|
Wow! It's finally in 🥳👍 Great work everyone 🎉 |
Overall this re-works the APIs for how an
EventLoopis run across all backends, based on the recent loong discussion in #2706See 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 theEventLoopand runs the loop on the calling thread until the app exits. This can be supported across all platforms and compared to the previousrun() -> !API is now able to return aResultstatus on all platforms except iOS and Web. Fixes: Android: shouldn't callstd::process:exitinEventLoop::run#2709A 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 persistentEventLoop. This doesn't allowWindowstate 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 Winitrun_returnand Android does not getResumedandSuspended#2706 and allows a Winit event loop to be embedded within an externalloop {}. Applications callpump_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_returnand Android does not getResumedandSuspended#2706Each method of running the loop will behave consistently in terms of how
NewEvents(Init),ResumedandLoopDestroyedevents 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 callingstd::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 theirmain()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_returnhas been removed, and the overlapping use cases thatrun_returnpreviously partially aimed to support have been split acrossrun_ondemandandpump_events.To help test the changes this adds:
The last _rfd example, tests the interaction between the
rfdcrate and usingpump_eventsto 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 aResultfromrun()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_eventswas going to be very similar torunexcept would usePeekMessageWinstead ofGetMessageWto 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:
wait_threadthat was a fairly fiddly way of handlingControlFlow::WaitUntiltimeouts in favor of usingSetTimerwhich works with the same messages picked up byGetMessageandPeekMessage.MainEventsCleared,RedrawRequestedandRedrawEventsClearedevents 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).RedrawRequestedevents are now directly dispatched viaWM_PAINTmessages - comparable to howRedrawRequestedis dispatched viadrawRectin the MacOS backend.NewEvents,MainEventsCleared, andRedrawEventsClearedget 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.NewEventsis conceptually delivered when the event loop "wakes up" andMainEventsClearedgets 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.RedrawEventsClearedare just delivered afterMainEventsClearedbut 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_eventsessentially works by hooking into theRunLoopObserverand requesting that the app should be stopped the next time that theRunLoopprepares to wait for new events.Originally I had thought I would poke the
CFRunLoopfor the app directly and I was originally going to implementpump_eventsbased 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 thatNSApp runwill 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 howrun_returnwould have worked before.Android
Nothing particularly unusual / unexpected here, except to note that I decided to make
run_ondemandpublic even though I couldn't previously imagine a use case forrun_ondemandon Android. As it happens I recently started working on a project that coincidentally looks like it has a potential use case forrun_ondemandthat may also make sense to use Android.X11
Nothing unusual. I think it may also fix an inconsistency with how the
StartCauseis set forWaitUntilcontrol_flowWayland
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_readwouldn'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
mioand handle theprepare_readprotocol 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_ondemandorpump_events, I have just maintained support for the portablerunAPI.TODO
run_noreturnextension for iOS/webCHANGELOG.mdif knowledge of this change could be valuable to users