-
Notifications
You must be signed in to change notification settings - Fork 921
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
MacOS can't wake up during resize, breaks timeout schemes #219
Comments
As far as I understand, this modal resize event loop is an unfortunate wart stemming from macos history 😿 I've spent quite a bit of time looking into solutions but haven't personally had much luck - hopefully your endeavour turns out better than mine! One of the particularly frustrating things about this is that, to properly handle live-resize a winit application's drawing must be abstracted in a way that allows it to be called when All that said, the current situation is much better than pre #126 days, where it was impossible to handle live resize without registering a totally separate callback function that could only share data with the main application via global state 😱 Hopefully we can work out some way to improve things even further though. I'm not sure about adding a timeout method, or whether it will even help much with our macos problem. In theory, this should already be easy for users to do by using a timer to call |
That's why I'm looking at this now and not earlier :-)
A timeout-capable API would remove this pattern: let events_loop_proxy = self.events_loop.create_proxy();
thread::spawn(move || {
thread::sleep(timeout);
events_loop_proxy.wakeup().ok();
});
{
let ref mut events = self.events;
self.events_loop.run_forever(|ev| {
events.push_back(ev);
glutin::ControlFlow::Break
});
} That's Letting the caller express timeouts seems intrinsically useful: if it's a timed event loop, the caller wants to either a) receive a message from the windowing system or b) get control again without a message so it can start another draw cycle. Every platform should have a way of accommodating this, and it's likely better than unconditionally spawning a thread for every message received. If not, maybe the platform should be responsible for implementing timeouts by spawning a thread and waking itself. "Receive with timeout" is also a more fundamental approach than the current APIs:
Yeah… the situation is worse than the comment in the code, and a timeout here doesn't help as much as I initially thought. Resizing does block on
That's bad. For resize events, A timeout on the outer loop won't fix that, but let's say the caller told us when they needed control again. How would that help? Well, I don't think we can avoid this inner runloop, but a runloop observer could punt control back to user code before that inner runloop goes to sleep or after it wakes. We should be able to wake up the event loop with a timer, as @mitchmindtree point outs, even if the timer message gets ignored by the mask and therefore not delivered in a timely fashion. How would that help? Well… let's say we somehow manage to get back to This would let |
I have a First, I started with refactoring: the MacOS event loop now has a Second, I added an optional Mac dependency on Third, I added an observer to the default runloop triggering on Next steps:
|
That's not exactly what I ended up doing, but oh man does this approach feel better. willglynn@12cd9f3:
This is what I mean by a timeout-centric API being fundamental. pub fn poll_events<F>(&mut self, mut callback: F)
where F: FnMut(Event),
{
// Return as many events as we can without blocking
while let Some(event) = self.get_event(Timeout::Now) {
callback(event);
}
}
pub fn run_forever<F>(&mut self, mut callback: F)
where F: FnMut(Event) -> ControlFlow
{
// Get events until we're told to stop
while let Some(event) = self.get_event(Timeout::Forever) {
// Send to the app
let control_flow = callback(event);
// Do what it says
match control_flow {
ControlFlow::Break => break,
ControlFlow::Continue => (),
}
}
} So far I've implemented only |
Per the earlier changeset, willglynn@1ffc1bf adds a shared Experimentation suggested that the timer callback alone was insufficient, as was a timer + runloop observer, but the combination of timer, runloop observer, and explicitly waking the event loop seems at least minimally passable. My crude instrumentation shows |
From #231, @mitchmindtree said:
Are you running with |
Ah my mistake, I did not! I just ran the window.rs example briefly again, this time with I just had a brief read through the diff for your coroutines branch. I've never done any stack switching before and so I'm yet to wrap my head around it fully (I think reading through this context crate might help), but this solution seems promising.
Would you mind elaborating on this a little more? Where is this delay coming from? |
Short answer: I don't know. My
When triggered from the coroutine, these callbacks yield back to the "normal" stack frame, which jumps back to The runloop observer keeps us from blocking indefinitely, and the timer is something we can trigger across threads, so we have all the pieces.
That said, I realized something else: in the case where we're trying to post an event from inside the coroutine -- say because the resize event loop triggered our This totally works, and it drops the event delivery time to 11-14 µs using the same unscientific measuring protocol. |
The code in my branch isn't at all a clear introduction to the concept, but it's what I produced after a series of incremental refactorings. My plan for the last action item ("optimization") involves moving the The problemA basic Cocoa runloop looks like: loop {
event = receive_event_from_cocoa(distant_future);
forward_event_to_cocoa(event);
match event {
appkit::NSLeftMouseDown => {
emit(WindowEvent::MouseInput{ … });
},
appkit::NSLeftMouseUp => {
emit(WindowEvent::MouseInput{ … });
},
…
}
}
extern fn resize_callback() {
emit(WindowEvent::Resized{ … });
} We need to map this to fn poll_events(callback) {
while let Some(event) = dequeue_event_nonblocking() {
callback(event);
}
global_callback = Some(callback);
while let Some(event) = receive_event_from_cocoa(distant_past) {
forward_event_to_cocoa(event);
match event {
appkit::NSLeftMouseDown => {
callback(WindowEvent::MouseInput{ … });
},
appkit::NSLeftMouseUp => {
callback(WindowEvent::MouseInput{ … });
},
…
}
}
global_callback = None;
}
extern fn resize_callback() {
if let Some(callback) = global_callback {
callback(WindowEvent::Resized{ … });
} else {
enqueue_event(WindowEvent::Resized{ … });
}
} This is roughly what The call stack at this point looks like:
We can get callbacks from within the internal event loop, but we can't return from The fix that isn'tOkay, so let's split this into two threads: one for the caller's code, one for Cocoa runloop. fn poll_events(callback) {
while let Some(event) = dequeue_event_nonblocking() {
callback(event)
}
}
fn runloop() {
loop {
event = receive_event_from_cocoa(distant_future);
forward_event_to_cocoa(event);
match event {
appkit::NSLeftMouseDown => {
enqueue_event(WindowEvent::MouseInput{ … });
},
appkit::NSLeftMouseUp => {
enqueue_event(WindowEvent::MouseInput{ … });
},
…
}
}
}
extern fn resize_callback() {
enqueue_event(WindowEvent::Resized{ … });
}
thread::spawn(|| { runloop() }); Brilliant. Now it doesn't matter if we're in the outer loop or an inner loop, since the only way Problem:
The fix that isThe realization is that while we do need two separate threads of execution -- one of them needs to stay in a loop while the other one returns from a function -- we do not need two separate OS threads. We need only the ability to enter and leave fn poll_events() {
while let Some(event) = yield_to_runloop(Timeout::Now) {
emit(event)
}
}
static timeout: Timeout;
fn runloop() {
timeout = yield_to_caller(None);
loop {
event = receive_event(timeout);
forward_event(event);
match event {
appkit::NSLeftMouseDown => {
timeout = yield_to_caller(WindowEvent::MouseInput{ … });
},
appkit::NSLeftMouseUp => {
timeout = yield_to_caller(WindowEvent::MouseInput{ … });
},
…
}
}
}
fn resize_callback() {
yield_to_caller(WindowEvent::Resized{ … })
} When At the machine level, this is a user-mode context switch -- The magic is that the If Right now in my |
So you're context switching by using inline assembly? That looks like dark magic. |
Well… I'm context switching using (Edit: LLVM is getting coroutine intrinsics, so it stands to reason we'll get them in Rust eventually.) As I see it, the facts are that a) the MacOS platform has unavoidable loops and b) the
|
There's also libfringe which provides this without the boost dependency. libfringe is of course implemented using inline assembly as well. In any case, the coroutine solution seems rather clever for the Cocoa run loop problem. It would be nice to not worry about macOS-specific details and have the control flow friendly event fetching API. |
I still have zero knowledge of how cocoa behaves in the first place, but I'm really dubious about this because you have to make sure that whatever the user does and whatever cocoa does this can't trigger an undefined behavior. |
I'm not even sure that context switching are compatible with safe Rust in the first place. We would need an expert to analyse that. |
I'll make the case that this is less crazy than it sounds. There's no concurrent multithreading. It's not possible for the user's code on the main thread and the runloop coroutine to concurrently mutate anything because they're not running at the same. We'd context switch into the runloop coroutine from There's no shared state between the runloop and the rest of the main thread. The runloop can keep everything in its private stack, except for events which it can push onto a mutex-protected queue, which is what it does now. That mutex can't deadlock on the main thread because there's exactly one call site where the main thread can enter the runloop coroutine. "How do we panic?" is a valid question, given that panicing unwinds the stack and we're on a different stack. Note however that I'm not suggesting we run the user's code in a coroutine, only |
I refactored a bunch of event-handling concerns, ripped out my I haven't yet hooked up the timer or runloop observer again, but |
There's now a timer and a runloop observer which helps the resize situation considerably. This brings my observed worst-case (When not resizing, call durations vary from 14-50 µs with a median of 26 µs. Switching contexts each trip into and out of the runloop does not seem to be a performance problem.) It's probably worth making the timer fire only during a resize event, since I think that's the only time we need it, and that would reduce the non-resize overhead. That would also let us make it fire ridiculously fast during a resize to further constrain the amount of time the inner event loop can spend on other things without yielding. |
I really don't think this should be required any more with the Event Loop 2.0 stuff? If you need control back during the live resize to allow the user to render, then an |
Right now during a window resize, alacritty window contents are unresponsive until after the resize is complete. Is this why? (See alacritty/alacritty#2283) |
- Fixes #163 - Reimplements window resize - Still has problems on macOS; Resizing with the drag handle blocks the main loop. See: - glfw/glfw#1251 - rust-windowing/winit#219
- Fixes #163 - Reimplements window resize - Still has problems on macOS; Resizing with the drag handle blocks the main loop. See: - glfw/glfw#1251 - rust-windowing/winit#219
fix rust-windowing#219 (minimum stroke thickness)
* refactor(windows): `begin_resize_drag` now similar to gtk's (rust-windowing#200) * refactor(windows): `begin_resize_drag` now similart to gtk's * fix * feat(linux): skipping taskbar will now also skip pager (rust-windowing#198) * refactor(linux): clean dummy device_id (rust-windowing#195) * refactor(linux): clean dummy device_id * fmt * feat(linux): allow resizing undecorated window using touch (rust-windowing#199) * refactor(windows): only skip taskbar if needed when `set_visible` is called (rust-windowing#196) * fix: increase borderless resizing inset (rust-windowing#202) * fix: increase borderless resizing inset * update some comments * Replace winapi with windows crate bindings shared with WRY (rust-windowing#206) * fix(deps): update rust crate libayatana-appindicator to 0.1.6 (rust-windowing#190) Co-authored-by: Renovate Bot <[email protected]> * Add Windows crate and webview2-com-sys bindings * Initial port to webview2-com-sys * Finish conversion and remove winapi * Fix renamed lint warning * Fix all match arms referencing const variables * Put back the assert instead of expect * Point to the published version of webview2-com-sys * Cleanup slightly weird BOOL handling * Replace mem::zeroed with Default::default * Add a summary in .changes * Remove extra projects not in config.json * Fix clippy warnings * Update to 32-bit compatible webview2-com-sys * Better fix for merge conflict * Fix clippy errors on Windows * Use path prefix to prevent variable shadowing * Fix Windows clippy warnings with nightly toolchain * Fix Linux nightly/stable clippy warnings * Fix macOS nightly/stable clippy warnings * Put back public *mut libc::c_void for consistency * Re-run cargo fmt * Move call_default_window_proc to util mod * Remove unnecessary util::to_wstring calls * Don't repeat LRESULT expression in match arms * Replace bitwise operations with util functions * Cleanup more bit mask & shift with util fns * Prefer from conversions instead of as cast * Implement get_xbutton_wparam * Use *mut libc::c_void for return types Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Renovate Bot <[email protected]> * fix(keyboard): add mapping for space key on Windows (rust-windowing#209) * fix(keyboard): add mapping for space key on Windows * change file * feat: impl Clone for EventLoopWindowTarget (rust-windowing#211) * chore: add `on_issue_closed.yml` (rust-windowing#214) * Update tray dependency version (rust-windowing#217) * Delete on_issue_closed.yml (rust-windowing#221) * refactor(linux): event loop (rust-windowing#233) * Use crossbeam::channel * Fix crossbeam channel import * Add check on poll event * Fix deadlock when unregistering shortcut on Linux (rust-windowing#230) * Add fullscreen monitor selection support on Linux (rust-windowing#235) * Add fullscreen monitor support on Linux * Add change file * Remove todo on videomode * Fix clippy * Update to 2021 edition (rust-windowing#236) * Update to 2021 edition * Fix clippy * Add run_return on Linux (rust-windowing#237) * Add run_return on Linux * Add main context * Add run_return trait on Linux (rust-windowing#238) * Fix: rust-windowing#239 Update webview2-com and windows crates (rust-windowing#240) * Replace webivew2-com-sys with prebuilt windows * Use windows utility instead of direct GetLastError * Bump windows version and add changelog * Run cargo fmt * Restore inverted matches macro * Scope constants in match arms * Fix inverted null check * Update src/platform_impl/windows/util.rs Co-authored-by: Amr Bashir <[email protected]> * Use env_logger instead of simple_logger (rust-windowing#241) * Use env_logger instead of simple_logger * Make clippy happy * Cherry pick commits to next (rust-windowing#244) * feat(macos): Add `unhide_application` method, closes rust-windowing#182 (rust-windowing#231) * feat(macos): Add `unhide_application` method * Update src/platform/macos.rs Co-authored-by: Amr Bashir <[email protected]> * Reanme to `show_application()` * Remove broken doc link Co-authored-by: Amr Bashir <[email protected]> * feat: Allow more strings to parse to keycode (rust-windowing#229) * feat: support accelerator key strings `,` `-` `.` `Space` `Tab` and `F13`-`F24` (rust-windowing#228) * feat(macOS): support more accelerator key strings * Move function keys together * Add `,` `-` `.` `Space` `F20-F24` for Windows * Remove support for accelerators not found in `winapi` * Add `,` `-` `.` `Space` `F13-F24` for Linux * Update .changes * Add the rest for Windows * Add the rest for Linux * Add the rest on macOS * Update accelerator-strings.md * Fix git comments Co-authored-by: Kasper <[email protected]> Co-authored-by: Amr Bashir <[email protected]> * Add redraw events on Linux (rust-windowing#245) * Add redraw events on Linux * Update doc of RequestRedraw Event * Add change file * Fix missing menu bar on borderless window (rust-windowing#247) Credit goes to irh's work on winit commit f2de847 * refactor: improve `set_skip_taskbar` impl on Windows (rust-windowing#250) * fix: emit errors on parsing an invalid accelerator for string, closes rust-windowing#135 (rust-windowing#252) * chore: update comment * fix(linux): fix focus events not firing properly (rust-windowing#253) * fix(linux): fix focus events not firing properly * add changelog * chore: update focus events error message * chore: fmt * fix: revert windows-rs 0.28 version bump * fix(linux): fix native menu items (rust-windowing#256) * chore: remove examples commited by accident * Update `ReceivedImeText` (rust-windowing#251) * Allow receiving text without Ime on Windows * Avoid panic todo * Receive text without ime on mac * Fix CursorMoved event on Linux * Add ReceivedImeText on Linux This only add Simple IME from GTK for now. We should add the actual IME from system in the future. * Fix redraw event that causes inifinite loop (rust-windowing#260) * Fix redraw event that causes inifinite loop * Refactor event loop * Remove unused function * Add doc comment on linux's run_return * Ignore doc test on run_return * Add non blocking iteration on Linux (rust-windowing#261) * Docs: SystemTrayExtWindows::remove() is gone (rust-windowing#262) Fix docs following rust-windowing#153 * Fix busy loop on Linux (rust-windowing#265) * Update windows crate to 0.29.0 (rust-windowing#266) * Update to windows 0.29.0 * Add change description * Remove clippy check (rust-windowing#267) * refactor(windows): align util function with win32 names * chore: update PR template * fix(linux): fire resized & moved events on min/maximize, closes rust-windowing#219 (rust-windowing#254) * feat(linux): implement `raw_window_handle()` (rust-windowing#269) * chore(deps): update to raw-window-handle 0.4 * add linux raw-window-handle support * update macos/ios/android * fix ios * Fix core-video-sys dependency (rust-windowing#274) * The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (rust-windowing#275) However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly. * Fix some invalid msg_send! usage (rust-windowing#276) * Revert "Fix some invalid msg_send! usage (rust-windowing#276)" (rust-windowing#277) This reverts commit a3a2e0cfc49ddfa8cdf65cf9870fb8e3d45b4bc0. * Revert "The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (rust-windowing#275)" (rust-windowing#279) This reverts commit 6f9c468f26ddb60e29be2139397bfaf3b30eab1e. * The `cocoa` crate links to AppKit, which made the symbol `CGDisplayCreateUUIDFromDisplayID` from ApplicationServices/ColorSync (which AppKit uses internally) available to us on macOS 10.8 to 10.13. (rust-windowing#280) However, this does not work on macOS 10.7 (where AppKit does not link to ColorSync internally). Instead of relying on this, we should just link to ApplicationServices directly. Co-authored-by: madsmtm <[email protected]> * Fix some invalid msg_send! usage (rust-windowing#278) Co-authored-by: madsmtm <[email protected]> * Add exit code to ControlFlow::Exit (rust-windowing#281) * Add exit code to ControlFlow::Exit * Cargo fmt * Add change files Co-authored-by: multisn8 <[email protected]> * Add new_any_thread to Unix event loop (rust-windowing#282) * Update windows crate to 0.30.0 (rust-windowing#283) * Update windows crate to 0.30.0 * Simplify new-type usage * Fix boxing in GWL_USERDATA * Make sure everyone is using Get/SetWindowLongPtrW * build the system_tray module when "ayatana" feature is enabled (rust-windowing#285) Without those cfg feature checks, the "ayatana" feature does actually not enable anything. * Fix click events missing whe tray has menu (rust-windowing#291) * Fix click events missing whe tray has menu * Add change file * Fix crash when tray has no menu (rust-windowing#294) * chore: update pull request commit exmple * fix(windows): send correct position for system tray events, closes rust-windowing#295 (rust-windowing#300) * fix(windows): revert maximized state handling to winit impl, closes rust-windowing#193 (rust-windowing#299) * fix(windows): revet maximized state handling to winit impl, closes rust-windowing#193 * add chanefile [skip ci] * fix: `MenuItem::Quit` on Windows (rust-windowing#303) * fix: `MenuItem::Close` on Windows * use `PostQuitMessage` instead Co-authored-by: amrbashir <[email protected]> * feat: v1 audit by Radically Open Security (rust-windowing#304) * Update to gtk 0.15 (rust-windowing#288) * Update to gtk 0.15 * Fix picky none on set_geometry_hint * Fix CursorMoved position Co-authored-by: Amr Bashir <[email protected]> Co-authored-by: Bill Avery <[email protected]> Co-authored-by: renovate[bot] <29139614+renovate[bot]@users.noreply.github.com> Co-authored-by: Renovate Bot <[email protected]> Co-authored-by: Lucas Fernandes Nogueira <[email protected]> Co-authored-by: Kasper <[email protected]> Co-authored-by: amrbashir <[email protected]> Co-authored-by: Jay Oster <[email protected]> Co-authored-by: madsmtm <[email protected]> Co-authored-by: multisn8 <[email protected]> Co-authored-by: Aurélien Jacobs <[email protected]> Co-authored-by: Lucas Fernandes Nogueira <[email protected]>
For posterity and because this ended up being one of the few resources about this, I'm gonna post this here too: |
@CrabNejonas I'd like to implement resizing myself to avoid this very issue of
I assume a |
The way I have implemented this is through testing for a click on the "resize areas" when a Then you just switch the mode for |
Thanks, @CrabNejonas for the help. Your idea sounds good, but I've been unable to figure out how to robustly determine whether a button press is bound for the resize areas. How do you do that? If I have to hard-code the areas, I risk getting them wrong and a button press slipping through to the NSWindow inner resize loop. |
As the comment describes,
nextEventMatchingMask:untilDate:inMode:dequeue:
blocks during a resize and does not return events until the resize is complete. That includes not returning events sent viapostEvent:atStart:
, which meansProxy::wakeup()
doesn't actually causerun_forever()
to wake up.This property is especially unfortunate with
glutin_window
, which implements its ownfn wait_event_timeout(&mut self, timeout: Duration)
by callingrun_forever()
and interrupting when the timeout expires. Since it can't interrupt, await_event_timeout()
that wants to yield 60 fps to e.g. drive a render loop will simply hang entirely until the user is done resizing.I've been experimenting with ways to get
nextEventMatchingMask:
to be less thorny, but another complimentary approach would be to expose a newrun_until_timeout()
call in addition topoll_events()
andrun_forever()
-- or perhaps to deprecatepoll_events()
andrun_forever()
in favor ofrun_until_timeout(Some(0))
andrun_until_timeout(None)
. A timeout-aware API would letglutin_window
pass its deadline all the way down tonextEventMatchingMask:untilDate:
, avoid spawning needless timeout threads, and sidestep some of the resize-related blocking issues all at once.The text was updated successfully, but these errors were encountered: