-
Notifications
You must be signed in to change notification settings - Fork 913
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
API Change Proposal: Events Loop 2.0 #459
Comments
I don't have much to say about the API itself (I'm not that much used to writing apps with winit tbh), just commenting to say I don't see any strong incompatibility / difficulty with implementing this regarding to the wayland backend. The most tricky part will be handling |
I think it's too opaque to have My grasp of your intended I'm curious how you'd say this proposal is better than #231, as your Additionally, since this is an area that has such an immediate and substantial impact on users, before acting on anything I'd want to see a lot of feedback. When we've achieved a good enough internal consensus, I think it would be a good idea to discuss it in other places in the community, and we'd hopefully be able to deliver an API change that people are hyped for, rather than one that unceremoniously breaks their application one morning. |
I think a breaking API change is necessary, but it must require the user to entirely surrender their thread. A callback-only API would fix Windows, it would fix MacOS (which has nearly identical resizing gotchas described in #219), it would fix the web (which needs to Going callback-only is a massive breaking change, but it's the right move. Most platforms expect to call user code from the UI thread when things happen, while Dropping Having said all that, while I am convinced that this is the right way forward, I am not in a position to be the champion of this approach. I no longer use |
@willglynn I'd say I'm in a good position to be the champion of this approach. If necessary, I'd even implement it for every platform, though for my sake it hopefully wouldn't come to that. At this point, I'm quite convinced that this is of great importance. Interfacing with platform APIs the correct way is already hard enough, and our current approach doesn't seem to be doing any favors for the users, contributors, or maintainers. I'm not sure how necessary the Anyway, now we just need to get the approval of... basically everyone. |
I would be in principle in favor of such a change on the winit API (going callback-only), though I'm not sure how much my opinion count on that, the fact remaining that wayland as a platform remains pretty accommodating regarding control-flow handling. Though, if such a thing starts going, I'd like to emphasize the need for good planning ahead (maybe going through an RFC-like process ?), in order to clearly define the contents and guarantees of the API, in order to allow easier coherence in backend implementations. Typical questions that in my opinion need to be clearly addressed are for example:
|
viva de revolution! I'd be happy to provide limited assistance (got all platforms by hand).
You have my seal of approval. Can also speak about some of the clients of
|
@vberger My instinct is to make Windows @francesca64 I can see your point with having My goal with the @willglynn basically explained the advantages of this API over the |
I'd be able to implement this on Windows, at least, as it should be a fairly simple change to my current single-threaded win32 branch. |
A common pattern that pops up in this space is sending yourself a message, particularly to communicate things from other threads onto a UI thread. This can usually be expressed directly using platform-specific hooks, and when it can't, it can be synthesized. This pattern often separates concerns: you end up writing event handling code to handle the message you sent from some other context and do whatever it asks you to do. An alternative to consider: fn run_in_event_loop<F>(f: F) where F: FnOnce(), F: Send + 'static Instead of sending custom events to the event loop, send closures to run within the event loop. (Platforms could implement this by sending themselves a message – though there are a few alternatives on Mac/iOS and a native closure queueing mechanism on the web.) This ability to easily execute code within the event loop from arbitrary contexts could substantially reduce the need for other thread-safe interfaces. |
@willglynn An issue I can see with that is that any closures you pass through that function don't gain access to the data in the main UI closure, which would limit pretty heavily what you could do with the passed closure. Passing custom events which are handled by the main event closure gives you access to that data. How would that be handled? The best solution I can see would be to wrap all the data in the main UI closure in a |
An other possibility would be to add a type argument to the event loop : However, I suspect several of the platform backend would implement it by using an For what it's worth, I've kind of struggled with this question for quite some time in wayland-client, for a very similar mechanism (allow closures run by the event loop to share data without using |
@Osspial Sure; there's definitely API design to do here. It's generally not sufficient just to be on the right thread, you also need the ability to use resources which are exclusive to that thread. That relationship – "there are things which are exclusive to this thread, but you can interact with them until your function returns" – should be described via the type system. This sounds The user probably has application-wide state which is synchronized with the windowing system. Parameterizing Thing is: this same thing (providing thread-local exclusive access to the platform and exclusive access to some application data) is needed for the event handling callback too. Whatever solution is used there should inform the parameter(s) to the |
@willglynn Regardless, I think that custom event passing is a more flexible solution than closure passing. My big reason for that is it allows users to implement a much more flexible closure-passing system than we could by just exposing a closure API, while the same couldn't be done the other way around. By letting users define their own closures (or enums of closures) as events, they can send closures with more return types or parameters than we could expose through the |
Speaking from almost complete ignorance here, but I wanted to throw out a suggestion.
Is it conceivable to try to invert winit's model, so it's more of a set of ui toolkits for the various backends, rather than an abstraction over the various windowing apis? I'm not sure that even makes sense as an idea, or whether there are more granular abstractions that make sense to make over parts of the different backends enough to warrant them all being in a single project. I also don't know how this would interact with glutin. Please don't take this as criticism of your great work! |
Btw, here's some discussion about inverting alacritty's model as well as a wip branch. |
I'm also in favor of windows being send+sync and the event loop being neither, since... that's what we do now, and it seems fine. However, there's something much more controversial I want to throw out there - should we rename As for messages vs. closures, I'm not very opinionated about that, and I'll be the first to admit I'm not very good at imagining APIs before using them. The snippet in #231 (comment) looks the most like what I'd expect, though that's partially because of aesthetics and familiarity. I understand the Whatever design we use, I want to move forward on this; I've been wanting to do more to attract contributors/maintainers, but it seems like a good idea to wait to do that until after we tear up the floorboards. Plus, I'm a bit tired of @Xaeroxe @icefoxen it would also be great to hear from both of you (sorry that this situation is sorta hard to follow, but the gist is that we want to change from "your code calls into winit" to "winit calls into your code" for better platform coherence/etc).
You'll be disappointed to discover that none of us are actually experts on creating cross-platform window/event abstractions. It's winit's best kept secret.
For as fragmented as winit's development has been, it does a pretty good job at this IMO. Of course, I don't doubt there are problems (and big ones at that), but I have a much easier time understanding what you mean when provided with concrete/specific examples. As for your overall concept, I don't have the time or energy to investigate the viability and merits of it. I'm having a hard time visualizing what it would be like in practice, but I will agree that there are holes in the stack that need to be addressed somewhere. |
I'm currently trying to digest the full implications of this proposal, and I have a question. Is the events loop thread expected to do draw synchronously to windows? This seems desirable for actions like resizing and undesirable for normal usage where draw time is high. |
@jwilm This redesign makes it easier for the event loop thread to draw synchronously to windows, but it's by no means required. I don't think it negatively impacts normal redraws, as the currently encouraged design has programmers blocking the event loop to redraw on a normal update, which would still be happening here. @sodiumjoe Correct me if I'm reading this wrong, but it sounds like you're suggesting winit be able to handle native OS widgets, right? I'm not opposed to that in principal, but native widgets are... complicated, in ways that make it pretty difficult for winit to handle properly without heavily expanding its scope (see #446). This is mostly Linux's fault, as it doesn't really have a native widget API in the way OSX and Windows do. |
@francesca64 @sodiumjoe I'm pretty strongly opposed to a total inversion of winit's control flow, as is done in the snippet from #231, for a couple reasons:
I'd like to clarify that second point using examples, but it can be difficult to do that without losing the forest for the trees, so to speak. Regardless, I'm not going to focus on the specifics of the API I present here, and rather look at the general issues I think you'd see with a totally inverted design. Compare a placeholder totally inverted design here: struct MyApp {
window: winit::Window
}
impl winit::App for MyApp {
fn init(windows: &mut EventsLoopWindows) -> MyApp {
MyApp {
window: winit::WindowBuilder::new()
.with_title("Hello, Winit!")
.build(windows)
}
}
fn on_event(&mut self, event: winit::Event) -> winit::ControlFlow {
match event {
winit::Event::Closed => winit::ControlFlow::Break,
// Process whatever other events you want to process.
_ => winit::ControlFlow::Wait
}
}
}
fn main() {
winit::Main::<MyApp>();
} With a callback-based design, here: fn main() {
let events_loop = winit::EventsLoop::new();
let window = winit::WindowBuilder::new()
.with_title("Hello, Winit!")
.build(&events_loop);
events_loop.run_forever(|event| {
match event {
winit::Event::Closed => winit::ControlFlow::Break,
// Process whatever other events you want to process.
_ => winit::ControlFlow::Wait
}
});
} The totally inverted design requires a user to:
A closure-based avoids the increased API surface from the first two points, and doesn't require users juggle API the guarantees from the third in their head by making them implicit to the structure of the code. Users don't need to assume that winit will call I don't see how a completely inverted design can be made that works around those problems in as intuitive a way as closures already do. |
@Osspial The scheme I outlined in #231 was designed around the idea that Is there an API you like better which allows |
@mtak- We've slowly been merging backends on the |
Since the main discussion seems to be happening here regarding these changes, I'll address #638 (comment) here. The extension traits have been renamed to include the operating system name, which feels very awkward in my opinion since this isn't standard convention. It's also not documented in |
@nvzqz That does sound awkward, I too wonder why this is the case. |
Not sure if this has been proposed before, but since some platforms may have multiple backends, it may be worth making |
Potential source of difficulty again with wasm: Instant is required in the Winit public API, but as far as I know you cannot obtain an Instant on web. |
I'm confused! Hardly anything is explained for the behavior, what happens in explicit detail, for winit::event_loop::ControlFlow. The docs should read like a man page in that, after reading, one should have the feeling of being able to write the entire event loop. Every conditional branch(if and match statements) should be explained, as should every other code block. winit::event::Event would logically have:
Consider redoing winit::event_loop::ControlFlow as a trait instance that has callbacks for set_timer(T, Instant), set_sleeping(), set_polling() and set_exiting(). Edit: |
Rational: Users of winit should know, unequivocally, what to expect from the event loop. If instead applications are coded to the probed behavior of the system they are developing on, this will lead to poor support for the other platforms. When behavior is discovered that's unexpected users of winit should know to seek help prior to continuing as a determination of feature or bug must be made and the documentation should be further refined in effect to remove ambiguity. |
@cheako Make a separate issue please. |
…ples/canvas_webgl_minimal/www/url-parse-1.5.1, r=jdm Bump url-parse from 1.4.7 to 1.5.1 in /examples/canvas_webgl_minimal/www Bumps [url-parse](https://github.com/unshiftio/url-parse) from 1.4.7 to 1.5.1. <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/unshiftio/url-parse/commit/eb6d9f51e395b7e47bf2594e457d541db21c713b"><code>eb6d9f5</code></a> [dist] 1.5.1</li> <li><a href="https://github.com/unshiftio/url-parse/commit/750d8e8a9d45dbce9ff09759f0fe4564cdd47d74"><code>750d8e8</code></a> [fix] Fixes relative path resolving <a href="https://github-redirect.dependabot.com/unshiftio/url-parse/issues/199">#199</a> <a href="https://github-redirect.dependabot.com/unshiftio/url-parse/issues/200">#200</a> (<a href="https://github-redirect.dependabot.com/unshiftio/url-parse/issues/201">#201</a>)</li> <li><a href="https://github.com/unshiftio/url-parse/commit/3ac777474ba5dc48a7e33771cbb311fc6f69bef8"><code>3ac7774</code></a> [test] Make test consistent for browser testing</li> <li><a href="https://github.com/unshiftio/url-parse/commit/267a0c6f7ef1a58271be61611c5103daace602c9"><code>267a0c6</code></a> [dist] 1.5.0</li> <li><a href="https://github.com/unshiftio/url-parse/commit/d1e7e8822f26e8a49794b757123b51386325b2b0"><code>d1e7e88</code></a> [security] More backslash fixes (<a href="https://github-redirect.dependabot.com/unshiftio/url-parse/issues/197">#197</a>)</li> <li><a href="https://github.com/unshiftio/url-parse/commit/d99bf4cf259b7378c855f786edc253e70405ffdc"><code>d99bf4c</code></a> [ignore] Remove npm-debug.log from .gitignore</li> <li><a href="https://github.com/unshiftio/url-parse/commit/422c8b5e4cac6a79cd35b4e86731476dcbeec7e4"><code>422c8b5</code></a> [pkg] Replace nyc with c8</li> <li><a href="https://github.com/unshiftio/url-parse/commit/933809d630c7b21399b4e5df59fccccd80033b21"><code>933809d</code></a> [pkg] Move coveralls to dev dependencies</li> <li><a href="https://github.com/unshiftio/url-parse/commit/190b2168035899a2a88f2dc2625963bf7e2f338f"><code>190b216</code></a> [pkg] Add .npmrc</li> <li><a href="https://github.com/unshiftio/url-parse/commit/ce3783f4ea25753cfa36376769c14e4e2fe6ea80"><code>ce3783f</code></a> [test] Do not test on all available versions of Edge and Safari</li> <li>Additional commits viewable in <a href="https://github.com/unshiftio/url-parse/compare/1.4.7...1.5.1">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://dependabot-badges.githubapp.com/badges/compatibility_score?dependency-name=url-parse&package-manager=npm_and_yarn&previous-version=1.4.7&new-version=1.5.1)](https://docs.github.com/en/github/managing-security-vulnerabilities/about-dependabot-security-updates#about-compatibility-scores) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language You can disable automated security fix PRs for this repo from the [Security Alerts page](https://github.com/servo/pathfinder/network/alerts). </details>
My apologies in advance for digging up this topic, but I am wondering if this implementation using custom events might allow for high-precision timestamping of the exact time that the OS places a message into the app's queue? Make it so that the very first thing the handler does upon unblocking is a query to the platform's performance counter, to ensure the absolute minimal latency error in the timestamp. Then just include the timestamp information in the custom event being forwarded. This level of timestamp accuracy is only achievable with wait-based message loops. A polling loop will invariably create a beat pattern/frequency aliasing in the timestamp deltas for high frequency events. It is not really an option to have the timestamping implemented by individual app developers further down the pipeline, since all the extra plumbings in the backend introduces timing variance that defeats the whole purpose of using the high-precision nano/microsecond timers. Making high-precision timestamp available across the board instantly give Winit the market corner for latency benchmarking. No other frameworks offer anything like it -- SDL2 doesn't even have a functioning wait_event, and most other frameworks' wait-based message loops are just nominal abstractions over their polling based backend. |
UPDATE 8/23/18:
A PR for Windows is available at #638
UPDATE 7/12/18:
I've adapted the agreed-upon changes into a changelog here: https://gist.github.com/Osspial/d3a3390807fa95a469d9c9b677db4715. The original proposal is below.
Events Loop 2.0
This issue proposes a series of changes to winit's
EventsLoop
API, based around the experiences I've had with creating a GUI library that uses winit as a window creation backend. The main goal is to give winit complete control over the event loop, both for event polling and event waiting. These changes should let us fix various implementation bugs on the win32 backend, as well as fix what I believe are design bugs in the current API.Technical Motivation:
EDIT: The technical motivation for the Windows backend is included below. However, pretty much every platform winit supports is hacky or broken in some way by necessity of the current API we expose.
Right now, the
win32
backend creates a background thread, in which the events loop resides. The biggest problem with this is that it introduces various unexpected synchronization bugs that require a good amount of effort to fix and increase the complexity of the backend (see #415 and #391). However, it's necessary to cleanly handlewin32
's various modal event loops.So, what are these modal loops, and why do they matter? On Windows, when a user begins resizing a window, Windows suspends the program's main event loop and begins its own loop, inside of the resize event's handler. Unfortunately, this freezes any processing that's going on outside the Windows event loop, which is currently worked around by having the loop exist in its own thread, introducing quite a few bugs as mentioned above. Inside the loop, Windows does a bunch of additional processing, most notably handling window snapping (although it probably does other stuff too that isn't obvious).
Windows doesn't publicly expose the APIs used to handle snapping, so re-implementing the modal loops ourselves so that they don't suspend the main event loop isn't really an option. However, Windows should still process events we push to the Windows events queue, letting us implement event polling inside the Windows event loop with custom events (using the same mechanisms used for wakeup messages), as long as winit's API lets it assume complete control over program control flow. Right now, it doesn't, but this change would let us remove the second thread and simplify much of the implementation.
Actually removing the second thread isn't that difficult (see #445), but these bugs present pretty major blockers that should be resolved before that's merged.
Design Details
EDIT: The original design has been removed from this post, as it is outdated and may cause confusion for new readers. However, it does contain some useful information about the rationale behind the changes, and can be accessed here: https://gist.github.com/Osspial/53efa87797899872650f6bece14736c0
The text was updated successfully, but these errors were encountered: