Skip to content
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

Winit API redesign #3367

Closed
kchibisov opened this issue Jan 6, 2024 · 61 comments
Closed

Winit API redesign #3367

kchibisov opened this issue Jan 6, 2024 · 61 comments
Labels
S - api Design and usability

Comments

@kchibisov
Copy link
Member

kchibisov commented Jan 6, 2024

Discussion summary.

Motivation

We need feedback for events and immediate reactions

The current API doesn't really work when you must get some values from the users
in reply to some events, because users has a clear choice to not do so. In some
cases it could lead to undesired behavior and even crashes. An example of such
thing could be:

The getting feedback part could be solved vi abuse of Mutex but it's not like
you can force users into doing correct cross-platform code, which actually
behaves the same.

There are more issues due to current massive callback approach, I just picked
few of them.

Monolithic design

In the current form winit is monolithic meaning that if the users want to use
winit they must include the entire crate introducing a lot of dependencies
and increase in compile time.

This is not great since GUIs don't really need all the winit or all the
backends. We also have a situation where you use winit or die, which is also
not great, so having a crate which provides mostly types could benefit.

Other issue of monolithic design is that winit is growing on its backends and
there's a demand to support niche platforms. Unfortunately, we can't provide
good support for all of them with the current maintainers we have. And some
platforms we can't event test reliably (e.g. redox). To solve this
winit should allow writing out-of-tree backends.

Interior mutability mess

Winit historically marks the Window as Send + Sync, while it's actually good
to write multi threaded code, internally it all goes back to event loop thread,
this is true for Windows, Wayland, Web, iOS.

If you even tried contributing into early winit it was event worse on interior
mutabilty and the use of Mutex, which were not need in 99% of the time.

Objects actually have lifetime

Window and other objects created while event loop is running actually has
lifetime and generally can't be used when the event loop is paused.

This issue is pretty clear with the run_on_demand sort of APIs where we must
destroy everything between the runs.

This issue is usually solved with lifetimes or Weak objects. The Weak
objects may require some sort of Upgrade in the API and doing so in
every call possible is strange. While the lifetimes sounds scary here,
it's a matter how you look at them, since one option is to give a
reference into the resource owned by winit, thus you can use object
only via the event loop is running.

The key could be some Id type and you may fail to get the resource if
it's no longer available.

The relevant issue #2903

No way to exclude functionality

The API surface is huge and we need a reliable extension system, so
the users won't end up with functionality they likely don't need.

One approach could be features, but it'll make the testing really hard, since
you'd need to test every per-mutation, the more natural approach would be
IDETs
. But they don't work with the massive callback design or declerative async API.

Proposed solution

To address the raised issues the proposed solution is being worked on
in https://github.com/rust-windowing/winit-next. It's not even remotely
complete but it should show the vector of development.

The user facing API is based around the Application trait, which
may be extended,

pub trait Application: ApplicationWindow {
    /// Emitted when new events arrive from the OS to be processed.
    fn new_events(&mut self, loop_handle: &mut dyn EventLoopHandle, start_cause: StartCause);

    /// Emitted when the event loop is about to block and wait for new events.
    fn about_to_wait(&mut self, loop_handle: &mut dyn EventLoopHandle);

    /// Emitted when the event loop is being shut down.
    fn loop_exiting(&mut self, loop_handle: &mut dyn EventLoopHandle);

    // The APIs which we consider optional (IDETs).

    #[inline(always)]
    fn touch_handler(&mut self) -> Option<&mut dyn TouchInputHandler> {
        None
    }

    #[inline(always)]
    fn device_events_handelr(&mut self) -> Option<&mut dyn DeviceEventsHandler> {
        None
    }
}

and the event loop handler.

/// Handle for the event loop.
pub trait EventLoopHandle: HasDisplayHandle {
    /// Request to create a window.
    fn create_window(&mut self, attributes: &WindowAttributes) -> Result<(), ()>;

    fn num_windows(&self) -> usize;

    fn get_window(&self, window_id: WindowId) -> Option<&dyn Window>;

    fn get_window_mut(&mut self, window_id: WindowId) -> Option<&mut dyn Window>;

    fn get_monitor(&self, monitor_id: MonitorId) -> Option<&dyn Monitor>;

    fn monitors(&self) -> Vec<&dyn Monitor>;

    fn exit(&mut self);
}

The backend facing APIs uses the traits like

pub trait Monitor {
    /// Return the given monitor id.
    fn id(&self) -> MonitorId;
    /// Returns a human-readable name of the monitor.
    ///
    /// Returns `None` if the monitor doesn't exist anymore.
    fn name(&self) -> Option<String>;

    /// Returns the monitor's resolution.
    fn size(&self) -> PhysicalSize<u32>;

    /// Returns the top-left corner position of the monitor relative to the
    /// larger full screen area.
    fn position(&self) -> PhysicalPosition<i32>;

    /// The monitor refresh rate used by the system.
    ///
    /// Return `Some` if succeed, or `None` if failed, which usually happens
    /// when the monitor the window is on is removed.
    ///
    /// When using exclusive fullscreen, the refresh rate of the
    /// [`VideoModeHandle`] that was used to enter fullscreen should be used
    /// instead.
    fn refresh_rate_millihertz(&self) -> Option<u32>;

    fn scale_factor(&self) -> f64;
}

An example could be seen at the winit-next
repo

What is not clear with the current approach

Multithreaded is not clear, but it's actually solvable. For example we may
have a way to enqueue callbacks into event loop thread or have types which
can perform rendering related requests from non-main thread.

However, we won't be able to provide all Window APIs on non-main thread, but
it is like that on most backends, except X11 and recent Wayland, so having
explicit API to queue callback which will get the relevant state will
work around the same to how it worked before.

We could also have a Weak<View> to pass to other threads, though given
that most drawing libraries Surface types are Send nothing stops
from sending the render target and some fences to ensure that the window
is not dropped in-between of the rendering.

Alternatives

Not aware of other options and given that most toolkits(outside of rust) do
generally similar things, it's not like their approach is bad.

What will not change

Fortunately, not everything will change, the things which likely stay
the same will be:

  • Event loop run APIs, they work fine and there's no general issue with
    them.
  • POD-like types.
  • While the events will spread, they'll still be there.
@kchibisov kchibisov added the S - api Design and usability label Jan 6, 2024
@kchibisov kchibisov added this to the Version 0.30.0 milestone Jan 6, 2024
@kchibisov kchibisov pinned this issue Jan 6, 2024
@notgull
Copy link
Member

notgull commented Jan 6, 2024

As I've stated in chat I don't think that a trait based system would be the solution here, as it doesn't solve a number of problems that the current system has:

  • The callback model maps very poorly onto many platforms (e.g. it barely works at all for Web, macOS has issues from what I'm aware)
  • It's not very backwards compatible; it’s a lot harder to change a trait than it is the current Winit API (which is already pretty hard to change without breaking).
  • It inaccurately represents the GUI system as a callback/response system.

In the past I’ve proposed replacing the event loop with a "block-on" call that blocks on a future, and then having most of the functionality take place via async calls.

Many of the problems mentioned are either solved or solvable with async, and as an added bonus we also get access to the Rust async ecosystem for free. If we’re going to be rewriting all of winit I think we should go in this direction.

I agree with splitting out a types crate and different backend crates, as well as most of the other proposals here.

@notgull
Copy link
Member

notgull commented Jan 6, 2024

I have a demo of such a system here: https://github.com/notgull/project-keter

For the most part I've only created the cross-platform integration test harness. However it should demo some of my ideas here.

@kchibisov
Copy link
Member Author

kchibisov commented Jan 6, 2024

The callback model maps very poorly onto many platforms (e.g. it barely works at all for Web, macOS has issues from what I'm aware)

That's how macOS actually works. You're getting called with a state. You also setup methods macOS calls to you, so it's just a trait based. You can't really model any async/await because there's really nothing to poll and you must communicate back and forth with it.

It's not very backwards compatible; it’s a lot harder to change a trait than it is the current Winit API (which is already pretty hard to change without breaking).

IDETs solve that. You can't even add event without a breaking change now, and no, non-exhaustive is not a solution to any of that.

Many of the problems mentioned are either solved or solvable with async, and as an added bonus we also get access to the Rust async ecosystem for free. If we’re going to be rewriting all of winit I think we should go in this direction.

I mean, you're free to make backend async/await internally, unfortunatelly I don't want to deal with endless Mutex sending around, because we need to communicate back and forth all over the place. How would you do a live resizes with async/await?

@dhardy
Copy link
Contributor

dhardy commented Jan 6, 2024

I like the idea @kchibisov but know very little of the backends.

Regarding multithreading, the only thing winit needs in my opinion is the ability to wake the event loop from another thread (i.e. EventLoopProxy).

@notgull
Copy link
Member

notgull commented Jan 6, 2024

That's how macOS actually works. You're getting called with a state. You also setup methods macOS calls to you, so it's just a trait based. You can't really model any async/await because there's really nothing to poll and you must communicate back and forth with it.

Any event loop that can be woken (I.e. any reasonable event loop) can be turned into an async/await loop that can poll a future. See async-winit for an example of this.

With the exception of Windows, in my opinion most of the platforms map better into async than not. X11 and Wayland are epoll (what async was designed to replace), macOS's and iOS's event callbacks can be turned into event listeners, and android-activity is basically epoll with a couple of weird quirks. I can make all of these run futures and send events as futures.

IDETs solve that. You can't even add event without a breaking change now, and no, non-exhaustive is not a solution to any of that.

Fair enough.

I mean, you're free to make backend async/await internally, unfortunatelly I don't want to deal with endless Mutex sending around, because we need to communicate back and forth all over the place.

In async this can be solved via the use of tasks and channels. Actually I’d argue that the actor model maps better onto GUI systems anyways.

How would you do a live resizes with async/await?

It’s possible to prevent the reactor from making progress until some event has happened. I use this in async-winit to handle resume/suspend events. I think this can also apply to these other immediate events.

@Jasper-Bekkers
Copy link
Contributor

While I don't have much direct suggestions in terms of how a future winit api needs to look like, I would like to point out a few things that we've encountered over the past few years of using winit.

🦣 General major API level changes

Please consider that winit is a beacon of the rust ecosystem and any changes made to the public interface of this crate typically end up costing quite a few man-hours downstream in terms of changes us downstream users need to make as well. I know winit doesn't have a 1.0 release at this point, but it's already quite widely used and so the impact of seeminly minor changes have a massive impact.

For example in the 0.29 release the PR that changed the Key & KeyCode enum (#3143) ended up costing us quite some time refactoring code we already had and code that was already working and tested in our application; effectively costing us some "busy work" to keep up to date.

Obviously this is kind of expected when you want to keep up to date with latest dependencies, so this is definitely not a call to stop all changes to public apis. However, it is an ask to be mindful of the changes proposed and their downstream cost.

📱 Application trait

We started using winit around 0.19 and one of the primary reasons was that winit at the time didn't really have an opinion on how your application needed to be structured. At the time there was a clean EventsLoops with a simple poll_events function that you'd call into and that was pretty much all the opinion that winit had about how we should structure our applications.

Over the years winit got more and more opinionated as an api (poll_events became run and run_return etc) and asked for more and more control over our main application loop. However, as we're developing a platform for building applications on top, with a minimal opinion on how the main loop needs to be structured we've always been in tension here with some of the decisions winit has taken over the years.

We've mostly always used winit to create a simple window so we could create a DirectX, Vulkan or Metal device that would do most of the rendering, but this has some implications later on as well. For example: our event loop thread and render thread need to run decoupled to maintain correct framerates (we need to use the swapchain to drive our present rate, rather then redraw requested events from a window manager). So for us winit just provides us with a window and keyboard/mouse input and it mostly should get "out of our way" after that point.

Introducing an Application style trait (or other traits) implies that we'll need to start structuring our applications in structs which lose control over our loops and over the structure of our apps.

🪂 async / await

We (and others) have a hard ban on async / await due to various reasons; some of these reasons are directly linked to tokio (it's a huge and complex dependency) and some of it is linked to async / await in general making it a poor fit for our application because of it's focus on high-latency / low throughput model of concurrency and it's general lack of focus on parallelism and some of it just the general immaturity of async / await in terms of ergonomics within Rust (compile errors, general confusion about how the model works for some engineers, no fire-and-forget support etc).

If winit ends up adopting async / await as a core primitive for building it's windowing api on top, I think we and a few others will most likely be forced to stand up a fork of winit 0.29 which we'd rather not do.

🧑‍💻 General complexity

I think some of these things stem from the fact that winit already supports quite a few platforms, and they don't always behave in exactly the same way (android has activities that behave very different from "windows" conceptually). Additionally there are some requirements from some platforms to take over the entire application loop to begin with, however, and some / most platforms requiring some additional steps for multi-threaded UI modifications.

However, C/C++ libraries such as SDL2 also manage to support all of the platforms winit supports, and more, while still having an equally simple API as we used to have in in winit 0.19 while also supporting macOS, Android, Windows, X11, Wayland and many, many more.

int main(int argc, char *argv[])
{
    SDL_Init(SDL_INIT_EVERYTHING);
    window = SDL_CreateWindow(window_name,SDL_WINDOWPOS_CENTERED,SDL_WINDOWPOS_CENTERED,800,600,SDL_WINDOW_VULKAN | SDL_WINDOW_SHOWN);

    SDL_Event event;
    bool running = true;
    while(running)
    {
        while(SDL_PollEvent(&event))
        {
            if(event.type == SDL_QUIT)
            {
                running = false;
            }
        }

 ...
}

Now, obviously, the requirements for winit and for SDL2 are different and I think the ambitions for winit have always been wider then the game-developer use cases but that doesn't invalidate some of this feedback I hope.

@alice-i-cecile
Copy link

async/await would be quite challenging to integrate nicely with Bevy, and similarly, would be quite aggressively opposed due to impacts on compile times.

@jackpot51
Copy link
Member

jackpot51 commented Jan 6, 2024

I am strongly in agreement with @Jasper-Bekkers that winit changes at this point should consider the large amount of downstream work required to adapt to them. Please consider the most minimal changes required to resolve each of the issues listed, I doubt a complete redesign is required.

The 0.29 changes were particularly painful for COSMIC and Redox OS, I am now using a forked 0.28 to support things already in 0.29 like Redox support and window resizing with CSDs. I had to backport those changes, quite painfully, because iced has not yet adapted to 0.29. And before such projects have even transitioned, to talk about more breaking changes, fills me with unease.

@notgull
Copy link
Member

notgull commented Jan 6, 2024

However, it is an ask to be mindful of the changes proposed and their downstream cost.

Thank you for the feedback here. I think that, if we rewrite the API here, it should be the intention that the new API will be the v1.0 API.

🪂 async / await

We (and others) have a hard ban on async / await due to various reasons; some of these reasons are directly linked to tokio (it's a huge and complex dependency)

This can be done without linking to tokio at all.

some of it is linked to async / await in general making it a poor fit for our application because of it's focus on high-latency / low throughput model of concurrency

Just because networking primitives have been designed in this way doesn't mean async for GUI has to be.

it's general lack of focus on parallelism and some of it just the general immaturity of async / await in terms of ergonomics within Rust (compile errors, general confusion about how the model works for some engineers, no fire-and-forget support etc).

tokio and smol have been out for years, have stable versions and are very mature. Yes, there is still more to go. But they are very parallel and scaffolding has reached a point where tokio and smol are used in many different types of production software.

If winit ends up adopting async / await as a core primitive for building it's windowing api on top, I think we and a few others will most likely be forced to stand up a fork of winit 0.29 which we'd rather not do.

The current winit API is nigh-unmaintainable. This would be an unavoidable black-hole for man hours for a project that, for the reasons mentioned above, is already broken.

If the overwhelming consensus is "if winit moves to an async model I'll stop using it," then I'll stop suggesting it. However, I'd like to point out that a lot of the projects described here (including bevy) are actually async/await; they just don't realize it yet! I talk about it in greater length in this blog post.

However, C/C++ libraries such as SDL2 also manage to support all of the platforms winit supports, and more, while still having an equally simple API as we used to have in in winit 0.19 while also supporting macOS, Android, Windows, X11, Wayland and many, many more.

This comes at a hidden cost for SDL2: it relies on a number of very fragile hacks that make its Android, Windows and macOS support very precocious. In fact, SDL3 is moving to a controlled callback model similar to what winit currently uses.

Even this restricted model doesn't work anymore for winit for the reasons mentioned above. Hence the reasons for this change.

@daxpedda
Copy link
Member

daxpedda commented Jan 7, 2024

Addressing OP

Motivation

We need feedback for events and immediate reactions

Monolithic design

Interior mutability mess

Objects actually have lifetime

No way to exclude functionality

I totally agree these are issues with the current design that must be addressed one way or another.

Proposed solution

Application as a trait

Generally speaking, I'm against the trait based application design, for the same reasons pointed out in #3367 (comment): I would very strongly prefer Winit to be unopinionated.

That said, Winit should allow, and maybe also encourage, a higher-level design that is "nice", where a trait based application sounds fantastic. Indeed a separation of low-level, winit-core, being as unopinionated as possible, and winit, offering a "nice" API, could work quite well. (I'm aware that this is not the intention winit-next is going for)

I don't exactly see how the trait based design addresses any of the issues from "Motivation", except the "No way to exclude functionality" for which I believe there are other, possibly less appealing, solutions.

Ownership

The idea to let Winit hold ownership over handles (Window, Monitor and such), solves the "Objects actually have lifetime" very easily. But, as I pointed out before, I would like to keep Winit's API unopinionated. Putting a lifetime on handles would yield the same limitations and give users the same amount of freedom (very little), but lets them retain ownership.

So far I can only see two solutions to this problem:

  1. Let handles properly outlive the event loop. Which is apparently already the case for Windows.
  2. Put a lifetime on handles tied to the event loop. This could always be accompanied by a conversion to an Owned(Handle) that either is a Weak handle, which would require all functions to possible panic or return a Result, or mimic the 1. solution.

I see this as the biggest factor requiring significant API changes and I don't see how we will bring this unto consensus unless we put significant effort into documenting each backends limitations around this and figure out together how we can proceed.

Unless of course we just pick the 1. solution.

Forced Event Feedback

This can obviously be addressed by the trait based system very conveniently. But this could be easily addressed, even though much uglier, with the current API.
E.g. users have to supply a separate closure to handle certain events that require a return value.
Another possibility is to always require a return value from the run closure, which can only be constructed "the right way", but this might be a highly annoying API.

Again: I'm aware that this might make the API quite ugly, which is why I'm in favor of keeping a very unopinionated and low-level API and having a "nice" high-level API separate (e.g. winit-core vs winit).

Send + Sync

I'm not fully aware of all the problems other backends have around this. Web has unfortunately a lot of complex code in place to support that, but it is literally zero overhead when calling methods from the main thread.

One way we could solve it is to let regular handles be !Send and an option to convert them into Send or Send + Sync handles. This gives users a way to opt-in to the overhead and internal mutability if they wish to while having a cross-platform API that can accommodate and exploit the full potential of each backend.

I can see how this might be complicated to implement though, so this will require discussion.

"Monolithic design" and "No way to exclude functionality"

Both of these issues are very greatly solved by the proposal. Of course there are different solutions to exclude functionality, but the proposed design is very neat. 'Nuff said, it just looks great.


Addressing #3367 (comment)

🦣 General major API level changes

I sympathize of course.
My assumption here is that this comes from the same place as #3367 (comment):

Please consider the most minimal changes required to resolve each of the issues listed, I doubt a complete redesign is required.

Which I have the feeling is correct.

That said, if a much better API can be achieved with serious breaking changes, we should go for it. This is the luxury we have not being v1.0.
(I don't actually think this will be the case though, hopefully I've made this clear by now)

📱 Application trait

Pretty much agree and addressed above.

🧑‍💻 General complexity

Unfortunately I'm unfamiliar with how SDL solves the problems we are facing here.
But I don't believe the current Winit API is complex, so my assumption is that this was directed towards the proposed changes, which I agree with.

The thing that will definitely remain is the closure system, as some things have to be run inside a callback, this is unavoidable.


Addressing the async 🐘 in the room

I'm very strongly in favor of keeping a sync API a first-class citizen in Winit.

That said, I pretty much agree with the point in favor of async in #3367 (comment). But this can be offered as an extension to an existing system and does not have to replace a sync API.

I have to mention that I don't actually think an async API would be worse in performance at all, as it doesn't stop you from using blocking code. Winit wouldn't offer an executor or anything like that, it's about async integration. So wrapping pollster around it would basically yield the same performance or better in the case of some backends (Web in particular).


Proposal

I very much share the opinion other maintainers have already voiced about this topic: a complete rewrite is not necessary.
These issues can be addressed incrementally, but I'm happy to be convinced otherwise.

As a first step I would like to propose moving this issue to a discussion, as I think that format would allow us to split the discussion into different topics and discuss them individually, instead of having to write posts where we discuss disjoint topics all over the place.

Next I would like backend maintainers to much more specifically point out the part of the API that doesn't allow them to solve a very specific problem so we can iterate on that and come to consensus on a step forward.

E.g. I think we should continue discussing #3317 and how we would like to solve that.

@kchibisov
Copy link
Member Author

Introducing an Application style trait (or other traits) implies that we'll need to start structuring our applications in structs which lose control over our loops and over the structure of our apps.

It really depends what you're doing, but the current design really doesn't work with the way Android expects you to work, and also doesn't work with other stuff. I don't see why you won't be able to do what you did before though, like it doesn't really matter whether you borrow values into the massive closure or whether you put something explicitly in the struct (treat it as closure capture list). I'm also not changing the way current event loop API works, you'd be able to still use pump_events or alike if you're into them.

If winit ends up adopting async / await as a core primitive for building it's windowing api on top, I think we and a few others will most likely be forced to stand up a fork of winit 0.29 which we'd rather not do.

I won't go with async/await either, I'll stick to traits since they work, yes winit will take more control, but not asking for more control is already a big issue since you must create window inside the EventLoop already on at least macOS, otherwise it'll start lacking on features, or will break under some circumstances.

I am strongly in agreement with @Jasper-Bekkers that winit changes at this point should consider the large amount of downstream work required to adapt to them. Please consider the most minimal changes required to resolve each of the issues listed, I doubt a complete redesign is required.

It doesn't work for anything more complex sorry, I can't add APIs that are a bit more complex (e.g. popups/subviews/proper dnd). I also have no time and desire to maintain backends in tree that we can't really test.

@kchibisov
Copy link
Member Author

would very strongly prefer Winit to be unopinionated.

I'd also like, but it clearly doesn't work, and I've tried various things over 5 years, and it's just getting worse and worse. But I have an option to just part ways with winit and do something else.

Let handles properly outlive the event loop. Which is apparently already the case for Windows.

Some handles are provided by event loop and only relevant when it's being executed. It doesn't work already with macOS.

Put a lifetime on handles tied to the event loop. This could always be accompanied by a conversion to an Owned(Handle) that either is a Weak handle, which would require all functions to possible panic or return a Result, or mimic the 1. solution.

Yes, and that's what I want to avoid, I don't want to deal with any of that, sorry. imagine having Result on 100 methods because you can have dead windows.

This can obviously be addressed by the trait based system very conveniently. But this could be easily addressed, even though much uglier, with the current API.
E.g. users have to supply a separate closure to handle certain events that require a return value.

So you want me to pass closures which are limited because they must be Send + Sync and which generally require access the state. For example for clipboard you need a closure to pick-mime type and closure to serve data on Wayland, because you may advertise formats which require reencoding.

Like I tried all of that and gave up because it's a giant mess. You need to develop Serial mechanisms, you need to work on cancellation of some stuff and continuously grow your closure.
It's really all over the place if you tried it.

Another possibility is to always require a return value from the run closure, which can only be constructed "the right way", but this might be a highly annoying API.

I can't have it on Wayland that way, so it doesn't work. Like the only way to remove the batching and keep maintainable code is to do what I do with traits.

Please consider the most minimal changes required to resolve each of the issues listed, I doubt a complete redesign is required.

It's actually minimal, you just move your event handler into function on a trait, and capture list into structs. Pretty minimal in my opinion.

Send + Sync

I'm not fully aware of all the problems other backends have around this. Web has unfortunately a lot of complex code in place to support that, but it is literally zero overhead when calling methods from the main thread.

Why winit should force multithreading on everyone? Why should we have so many Mutex stuff because things are Send + Sync while they are clearly not and do call their stuff on the main thread. It's just not a great design, you should use Mutex for the stuff which is actually meant to reside on multiple threads 99% of the time, adding a lot of complexity just because someone may want to send their window is not how you should design the library. Like if you lookend into winit code you'd know how many mutex with_context stuff we have internall to properly release locks for a code that is always executed on the main thread, it's just insane.

@kchibisov
Copy link
Member Author

Regarding multithreading, the only thing winit needs in my opinion is the ability to wake the event loop from another thread (i.e. EventLoopProxy).

That's what I want to do as well and then the users may poll sources they need for that, it's pretty simple and basically solves the issue of squashing events instead of massive channel abuse.

@Ralith
Copy link
Contributor

Ralith commented Jan 7, 2024

As a user of winit in games, the proposed trait API seems inoffensive, and would probably take me about ten minutes to port to. It and the status quo can be implemented in terms of each other, so the amount of disruption has a hard limit. My application code tends to closely resemble the trait pattern in practice anyway, since inlining all your logic into one gigantic callback is a really poor way to structure a project.

The prospect for middleware (e.g. GUI toolkits) integration with winit with reduced complexity/buildtime/semver hazard, is appealing, but I think independent of the question of traits vs. mega-callback vs. etc. Similarly, out-of-tree backend support would be cool, but doesn't obviously need to involve changes to the application-facing API. Perhaps these discussions should be separated?

I disagree with the suggestion that the proposed application-facing traits are more opinionated. There's very little that you can usefully do outside of a match block in today's winit run callback, and when winit owns the main loop anyway, the difference between code in a match block and code in a trait method is marginal.

I also disagree that async is particularly costly: like with most Rust features, you pay for what you use. However, it's not obvious why async support must be built-in to be viable, and for maintenance's sake winit must minimize scope creep.

@kchibisov
Copy link
Member Author

The prospect for middleware (e.g. GUI toolkits) integration with winit with reduced complexity/buildtime/semver hazard, is appealing, but I think independent of the question of traits vs. mega-callback vs. etc.

Yeah, it's separate, but it needs a change in design as in have extension traits in one way or another. The current API kind of wants you to pass event and for some extensions you may want to have a Platform event, which doesn't really work...

Similarly, out-of-tree backend support would be cool, but doesn't obviously need to involve changes to the application-facing API. Perhaps these discussions should be separated?

It does change, because you need traits to set a required functionality for backend to be used, and you'd need to rely on dyn a lot, because the backend could be literally anything, so it does require changes in the API, and in the current.

I disagree with the suggestion that the proposed application-facing traits are more opinionated. There's very little that you can usefully do outside of a match block in today's winit run callback, and when winit owns the main loop anyway, the difference between code in a match block and code in a trait method is marginal.

Yeah, that's my point as well, you're doing that already, I just take away the ability to move Window around, but it doesn't really matter since you'd still be able to move rendering surface around, and you'd be able to execute stuff off the main thread by queueing it into main thread (that's how it's done anyway internally, see libdispatch(macOS)/threaded_executor(Windows), QueueHandle(asks to dispatch on the main thread once done), etc).

However, it's not obvious why async support must be built-in to be viable, and for maintenance's sake winit must minimize scope creep.

Exactly, and that's why I'm going with extensions traits, so backends can independently provide provide special functionality having the core clean.

@kchibisov
Copy link
Member Author

Having a way to write middleware can be good if you want to have accessibility features or plug extra debugging(but don't want any of that in release builds). I don't see why you shouldn't be able to plug dbus in-between on linux when you do GUIs.

It's just allows the scope and core be limited and clean, while allowing extensibility, and allows to fork and tune backend for needs approach.

This is included here because the one common way to approach this issue is to develop interfaces (traits), though it's a different set of traits because one part is Application facing and the other is backend facing. But given that backend my need to extend events you can't really do that with the single massive closure, so you need to split and have trait-like callbacks, which leads that you need closure and trait based callbacks at the same time and you'd like need to share state, it's just not nice.

@Ralith
Copy link
Contributor

Ralith commented Jan 7, 2024

[Application-facing API] does change [in response to out-of-tree backends], because you need traits to set a required functionality for backend to be used

I don't follow. Can you give a small example? Naively, I'd imagine the core <-> backend interface could be 100% isolated from the core <-> application interface, with the core strongly encapsulating calls into backend traits. This is how Quinn abstracts over async runtimes, for example.

@kchibisov
Copy link
Member Author

With the suggested API it's encapsulated yes, with the current API it's not and I don't see how you'd approach that, unless you put event into Box.

I think an example is when the backend wants to have a backend specific event for some backend specific functionality, like URL opening handling on macOS.

In the current state of things in winit if you add something like that you'll infect all the backends (see touchpad magnify, or memory warnings on Android/iOS).

So it does matter what you pick in core, since you really don't want it to expose backends unless you cast to backend stuff.

@Ralith
Copy link
Contributor

Ralith commented Jan 7, 2024

I figured the cost of having rarely-used events in core is comparatively low, but I don't have much context to make a strong judgement there.

How would the core pass an event type not known to it from a backend to the application? I'm leery of type-level solutions to this problem, as they tend to complicate portable code which wants to opportunistically leverage platform-specific features, moreso even than feature gates.

@kchibisov
Copy link
Member Author

How would the core pass an event type not known to it from a backend to the application? I'm leery of type-level solutions to this problem, as they tend to complicate portable code which wants to opportunistically leverage platform-specific features, moreso even than feature gates.

If you know which backend you're using you can have a cfg, but features in the backends itself won't need a cfg, because of IDETs. So you'd just cfg(wayland) and then impl the backend specific traits in that file you want to handle, then the backend will check what you have implemented via dyn_cast(during event loop creation) and will naturally call to you in the impled trait.

The other part is requests from the application to core and I'm not entirely sure what to do with that side yet, surely you can also do IDETs but it could be a bit ugly. Backend could also cast to its raw types and callback to you with the real types so you have everything you may need, but it could split functionality.

So I'd say events are fairly simple and requests is the only thing I'm not sure how to plumb correctly.

@Ralith
Copy link
Contributor

Ralith commented Jan 7, 2024

then the backend will check what you have implemented via dyn_cast

Sorry, what? Rust doesn't have C++-style dynamic casts, and trait methods to downcast to a concrete type would have to be defined in core. You could do something with TypeId and unsafe, but that's rather unergonomic for the applications implementing it.

@kchibisov
Copy link
Member Author

Sorry, what? Rust doesn't have C++-style dynamic casts, and trait methods to downcast to a concrete type would have to be defined in core. You could do something with TypeId and unsafe, but that's rather unergonomic for the applications implementing it.

Hm, yeah, you'd need something like that https://github.com/bch29/traitcast . Which involves type map to bring RTTI.

So the option is to have all extensions listed on Core stuff, but it's fine, I guess.

@DasLixou
Copy link

DasLixou commented Jan 7, 2024

I see pros in both the trait interface and the async interface, but I can also see why this could be awkward to implement for the users. Winit has two main use cases I would say:

  • Games or Real-time apps
  • normal GUI apps

A real time App like a game needs to do much more stuff than just the event loop, there an async/await structure might really be a good solution. For @alice-i-cecile 's comment for bevy, I have to say that the current runner api is a monolithic big thing that controls the whole App and feels more like it's made for winit and not the other way around. When reworking on the whole runner idea and making it async we could maybe also achieve to have multiple runners at once for different stuff.

Normal guis on the other side are more strictly bound to the event loop. In my 5 attempts for a Ui framework I could imagine both a async/await and trait-based api to be used.

Im not too familiar with the insides of winit and also not with the old winit .19, but when it's based on a simple poll_events like structure, why couldn't we have both a trait-based and an async/await api, and when it's not too messy we could also for the applications who don't want to upgrade or really prefer the old api, try to also implement that?

Never the less, I have some questions to the requested API designs:

async/await

  • How will the event loop integrate into that? When is creating a window allowed and when is it not?
  • How modular can we make this? Trait-based has some advantages for modularity with e.g. touch events being another trait to implement.

trait-based

I forgot ._.

@kchibisov
Copy link
Member Author

just fyi poll_events exists in the form of pump_events API if you need it, it works scuffed on macOS and has the same issue poll_events had, but that's about it, and it won't go away, like nothing really stops you from writing code the way it was with 0.19 here, just don't complain that it's not working great with resizes, and that android demanding you to remove not render while suspended.

How you structure your game engine is also subjective and you clearly can have a

let mut event = Vec::new();
loop {
    event_loop.pump_events({
         event.push(event);
    });

    for event in events {
        // Run
    }
}

But be aware that such APIs don't work on e.g. ios because they take over the control, but you may not care here.

@DasLixou
Copy link

DasLixou commented Jan 7, 2024

But when pump_events has so many problems and there are backends who take full control, how will async/await not have those problems? Won't it be just like a pump_events in async context?

@kchibisov
Copy link
Member Author

no one said that we're doing async/await, I replied to your poll_events question.

@fredizzimo
Copy link

@Jasper-Bekkers, in neovide I started to work on a direct3d backend with waitable swapchains neovide/neovide#2215, because after numerous of hours/days trying, I could never get windowed opengl to not drop visible frames on Windows.

I am still using the winit event loop to drive it though through this https://github.com/neovide/neovide/blob/6e92f27544d3275be4fa9a2135cf8c9e43361034/src/renderer/vsync/vsync_win_swap_chain.rs, which emulates the native winit RedrawRequested event.

@kchibisov
Copy link
Member Author

The more limiting factor though, is Wayland itself, the way it forces the render surfaces to be synchronized at all times, basically forcing us to threat the winit event loop as the render loop. It's not a problem, it works, but at the same time,if our loop was more heavy an unreasonable amount of time is lost due to event processing, especially on windows, even with the latest refactoring.

I'd just let it on you so you'd not e.g. crash or anything, you'd just render one extra frame with wrong scale, but that's about it. No sync except that you'd have explicitly confirm the changes to the surface, like all the issues are due to winit doing it for you and not the other way around like it should.

I strongly favor an incremental approach to the changes, if it's possible. At least a proper analysis should be done to see if it is possible. We still remember the keyboard update, which left us in a very bad state for a couple of years, since we needed those changes. But at the same time it wasn't up to date with the latest winit version, so we had a lot of bugs, including lack of wayland support, that had to wait until it was merged. And I don't want to see that happening again, to either us or someone else. Basically there should never be a case where different bugs and features exist in winit-next and regualar winit.

Changes are internal and don't break the user API except that you'll have to move match into traits, but it's not really a big change compared to e.g. keyboard v2, since events will stay the same in the initial revision. And also it can't be done in steps because a lot of things change and for example

@daxpedda
Copy link
Member

daxpedda commented Jan 12, 2024

Preface

I'm just summarizing the current discussions and conclusions and adding my 2¢ on top. This might not contain any new information for some, but will try to include the user-perspective in addition to the maintainer one.

Current Issues

User Feedback on Events

There is currently no good way to supply or require user feedback from events. This would need some return type in the closure, which is hard to accomplish with the current API as not all events require feedback.

Currently this is done by providing a writable type in the event, see WindowEvent::ScaleFactorChanged::inner_size_writer.

Internal usage of Mutex

Backends have to give Window and other types ownership over their context because they can outlive the event loop, where they have to remain valid. E.g. if Window doesn't remain valid after the event loop, WindowHandle would become invalid as well, which would be unsound (see #3317).

This requires the usage of Mutex to make these types Send + Sync, obfuscating the fact that most backends aren't actually Send + Sync, decreasing performance and making the implementation and maintainability much more difficult.

External Backends and Extensions

Currently Winit doesn't support any external backends. This is important because currently any backend has to be added to winit itself, which would significantly increases the maintenance burden.

It should also be possible for external backends to extend current functionality. E.g. it's impossible for an external backend to add a new event with the current API.

Requirements

  • Continuing to support safe window creation through WindowHandle.
  • Separate Winit and it's backends, enabling external backends which are not part of the Winit project.
  • Keep MT support while enabling backends to not require Mutex to implement Send + Sync types.
  • Allow high-level APIs to exist on top of Winit while keeping Winit as low-level and zero-overhead as possible.

Minimal Changes

One way we could address all current requirements with minimal changes to the API is to make applications unable to exit unless all Windows are dropped. See "Nothing Outlives the Event Loop" for more details.

Separating Winit and it's backends can be done internally by adding a winit-core crate as proposed in the OP and adding some extensions to winit to register additional backends and select them. Extensions could be handled by adding platform-specific enum variants that can be accessed through extension traits (see #3064).

Event feedback would still require supplying writers. One way of forcing a return value with the current API could look like this (hopefully we can all agree that this is an awful API):

Click to open
pub struct ReturnValue(ReturnInner);

enum ReturnInner {
    Empty,
    CancelResize(bool),
    // ... other variants for events that want a return value
}

impl ReturnValue {
    pub fn empty(empty: Empty) -> Self {
        Self(ReturnInner::Empty)
    }

    pub fn cancel_resize(cancel_resize: CancelResize, value: bool) -> Self {
        Self(ReturnInner::CancelResize(bool))
    }

    // ... methods for each return value type
}

#[non_exhaustive]
pub struct Empty();

#[non_exhaustive]
pub struct CancelResize();

pub enum WindowEvent {
    Destroyed(Empty),
    Resized {
        size: PhysicalSize<u32>,
        cancel: CancelResize,
    },
    // ... each variant will need a value to construct a `ReturnValue` with
}

impl EventLoop {
    pub fn run<F>(self, event_handler: F) -> Result<(), EventLoopError>
    where
        F: FnMut(Event<T>, &EventLoopWindowTarget<T>) -> ReturnValue,
    {
        todo!()
    }
}

This isn't actually a proposal, just a thought-experiment. Keep in mind that I might have missed something here as well.

Currently Proposed Solutions

Trait-based Event Loop

Description

Changing the event loop into a trait handling each event as a method (as proposed in the OP) would look like this:

pub trait Application {
    fn new_events(&mut self, elwt: &EventLoopWindowTarget, start_cause: StartCause);

    fn resized(
        &mut self,
        elwt: &EventLoopWindowTarget,
        window_id: WindowId,
        size: PhysicalSize<u32>
    ) -> bool;

    // ... a method for each event
}

Upsides

  • Simple way to add platform-specific extensions. See "Split Backends Into Crates".
  • Ability to force a return value.

Downsides

Removing the Event enum makes it difficult to handle all events with a single handler and might cause code duplication and missing events.
E.g. if you want to log any event received with the current API, it's as easy as using log::info!("{event:?}").

With the proposed API you would have to make sure to implement all methods and add a statement in each of them. This will also make some integrations more difficult to use. E.g. egui-winit currently uses a very simple on_window_event() function that takes a WindowEvent.

This might be addressed by adding an additional method (that is called before still calling the "specific-event-method") which works like the old event handler: supplying a huge enum which includes all possible events but doesn't allow any event feedback.

On the other hand this could also be done in a higher-level implementation, but then integrations like egui-winit would want to depend on that higher-level implantation. So unless we have more concrete plans for that I would discount this as a solution for this particular problem.

Another possibility is to add a plugin-like extension point, which would allow integrations to register handlers themselves.

User Migration

Apart from the downside described above this should be simple to migrate to. It also doesn't take away any functionality or control from the user. Any values moved into the closure can be put into the type implementing Application, which users have access to through &mut self.

Maintainer Disposition

I believe all (currently active) maintainers are in favor of introducing this trait-based event loop. See #3386 for actual work on this.

Alternatives

  • Add platform-variants that can be accessed through platform-specific traits to allow backends to extend e.g. events. See Platform-Specific Enum Variants #3064.
  • To force return values see "Minimal Changes" for an awful alternative with the current API.

Nothing Outlives the Event Loop

Description

Window (and other types) outliving the event loop is problematic because of the issues described in "Internal usage of Mutex". The solution proposed here would be pretty simple: the event loop can't be exited without dropping all Windows first.

Upsides

Backends can decide what kind of state Window (and other types) should hold. It would be possible for Window to be a simple handle, allowing it to remain Send + Sync while requiring no Mutex, using a dispatch implementation when they are not on the main thread. When they are on the main thread they can access the event loop through a thread local variable or they can hold a Rc handle to it themselves.

Downsides

  • It might be annoying for users to have to "track down" all Window handles to be able to exit their application.
  • If Window is stateless (just a handle), a good stategy to access the EventLoop would be required. As proposed in "Upsides", that could be done either storing the event loop state in a thread local or by holding a Rc handle, but that would require a RefCell. Alternatively an (unsafe) event loop thread local scope could be introduced:
    thread_local! {
        static ELWT_SCOPE: RefCell<Option<&'static mut EventLoopWindowTarget>> = RefCell::new(None);
    }
    
    ELWT_SCOPE.with(|scope| *scope.borrow_mut() = Some(unsafe { elwt as &'static mut _}));
    // run user code, which contains calling methods on `Window`,
    // which have mutable access to the event loop through this scope
    ELWT_SCOPE.with(|scope| *scope.borrow_mut() = None)

User Migration

This represents no real API change to the user, it's just required to drop all Windows before exiting the event loop. Most applications keep this in mind anyway while using Winit, especially when they support multiple windows, where a window doesn't close until Window is dropped.

Maintainer Disposition

We didn't discuss this yet. It would also still be important to figure out if this is truly possible on all backends. Some backends might require an event loop to be exited immediately without leaving a choice to the user but still not close the process afterwards. This would leave invalid Window values.

Alternatives

  • Doing nothing. At least the current API might be sound already, see WindowHandle is not sound #3317 for more on that.
  • The "Direct Window Access" proposal could be considered an alternative.

Direct Window Access

Description

The original idea here is that there is no user-owned Window type (as proposed in the OP), to access a window and it's methods a user can request a reference to Window like this:

impl EventLoopWindowTarget {
    fn window(&self, window_id: WindowId) -> Option<&Window>;
}

To continue supporting raw_window_handle::WindowHandle, we still need a user-owned type. So a winit::WindowHandle could be introduced to fill that gap. This would have to be combined with the "Nothing Outlives the Event Loop" to keep raw_window_handle::WindowHandle sound.

To enable calling methods on a window in another thread, a dispatching API would have to be added to WindowHandle. Backends could implement fast paths that work from any thread without dispatching directly on WindowHandle.

Upsides

  • As this depends on the "Nothing Outlives the Event Loop" proposal, it has the same advantages.
  • Event loop can be free of RefCell (or weird thread local scopes) now compared to "Nothing Outlives the Event Loop", because methods can be called directly on Window while living in the event loop.

Downsides

  • Not all backends get any advantage over "Nothing Outlives the Event Loop". E.g. if methods on Window still require access to the event loop, it would still not get that unless:
    • the method takes a parameter to the event loop, in which case "Nothing Outlives the Event Loop" proposal would already yield the same result: on the main thread, where they have access to the event loop, they can just pass that; on other threads they will use the dispatch API.
    • Window holds a Rc to event loop, in which case it re-introduces a RefCell again.
  • Compared to "Nothing Outlives the Event Loop", where the actual window state can live either in the event loop or in the Window type, now the window state is forced to live in the event loop.

User Migration

For single-threaded applications this would be straightforward to migrate to. Multi-threaded applications will have to use the dispatch API on other threads. This shouldn't be hard to use and is better anyway: now the user is aware of the overhead this causes.

Maintainer Disposition

At least from my perspective this is still the most controversial change we have to discuss more and figure out all the kinks.

Alternatives

Do nothing. Which might be okay if "Nothing Outlives the Event Loop" is enough.

Split Backends Into Crates

Building on "Trait-based Event Loop", a winit-core crate would be required (as proposed in the OP) that has all the traits necessary for a backend to successfully function in winit.

This still has to be fleshed out entirely in written form and I believe @kchibisov and others have more of a direction in mind here so I will leave any further definition omitted.

Upsides

  • Allowing completely rust-windowing external backends to exist and to integrate into Winit.
  • Hopefully reducing the maintenance burden in Winit directly by allowing some backends to become external.

Downsides

Generally speaking coordinating all these crates will be more work.

User Migration

There should be none required, as this is only an internal change.
If we decide to split out any backend entirely and make it external, users would have to add that backend to winit.

Maintainer Disposition

At this point I believe all (currently active) maintainers agreed that this is a good direction.

Alternatives

Do nothing.

Implementation Process

There's still an ongoing discussion on how we want to proceed here: rewrite vs iterative.

@madsmtm already outlined how this could be done iteratively in #3367 (comment) and made a comprehensive summary of it's up- and downsides in #3367 (comment).

A rewrite mostly has the advantages of being much faster to finish, with it's downsides basically being the upsides of the iterative approach.

I don't want to add much more here as this is mostly an internal discussion that is still ongoing.

@kchibisov
Copy link
Member Author

No Window Type

I've already replied to that, Window will just be WindowHandle or something of the same sort being Send + Sync and having the same semantics as the current Window.

Like #3367 (comment) explicitly address it. The new &Window you borrow is the internal window we have now, so you can use fast path and current Window will go through slow path to sync state, mutexes, delayed dispatching etc, like it's now as well.

@daxpedda
Copy link
Member

I've already replied to that, Window will just be WindowHandle or something of the same sort being Send + Sync and having the same semantics as the current Window.

This is indeed outlined in my post.

The new &Window you borrow is the internal window we have now, so you can use fast path and current Window will go through slow path to sync state, mutexes, delayed dispatching etc, like it's now as well.

I have more explicitly compared both approaches now in the post. The point being is that exposing &Window from the event loop to the user forces backends to store Window inside the event loop. But not exposing &Window you can store it wherever you like.

The fast path can still be accessed through Window or WindowHandle, no matter where the actual window lives, as outlined above.

There are some tradeoffs for each of those proposals which is why I specifically differentiated them. But these tradeoffs are semantic, the outcome should be the same.

Let me know if anything is unclear or if I missed anything.

@kchibisov
Copy link
Member Author

The only point that is missing is that we need to model a failure in destructor, which you can't really do when you own the type, such failure is relevant when you have a lot of weak handles, etc. It can also help with cleaning up resources between the run-on-demand runs and in cases where we want to borrow the Window from the user.

As for the backends, at least most of them store the window on event loop window target or similar. Also, having calls to accept EventLoop is not something bad.

@daxpedda
Copy link
Member

The only point that is missing is that we need to model a failure in destructor, which you can't really do when you own the type, such failure is relevant when you have a lot of weak handles, etc. It can also help with cleaning up resources between the run-on-demand runs and in cases where we want to borrow the Window from the user.

I think you are talking about when the user owned window type actually owns the window state. If we are still talking about why the "No Window Type" proposal might not be necessary, then there is a misunderstanding here.

What I'm trying to say is that even without the "No Window Type" proposal, you can let your actual window state live in the event loop, as outlined above.

As for the backends, at least most of them store the window on event loop window target or similar. Also, having calls to accept EventLoop is not something bad.

It is not! What I am saying is that if you take EventLoop, you could do this without the "No Window Type" proposal as well, yielding the same result.


I think I will rename the "No Window Type" proposal to "Direct Window Access". Because this isn't about window ownership, which is covered by "Nothing Outlives the Event Loop".

@madsmtm
Copy link
Member

madsmtm commented Jan 12, 2024

Thanks for the excellent summary @daxpedda, I've added it to the top of this issue.

I've put up #3386 as a fully backwards-compatible starting point for migrating to a trait-based approach, and have migrated Redox as an example in #3387.

@cybersoulK
Copy link

My app has several layers that handle winit events. These layers are in different crates, they are like modules that receive the winit events and handle certain parts of the windowing, input, UI, rendering or the game logic.
So each of them cannot be called "Application", they are different Modules that handle the events in a cascade like format.

@kchibisov
Copy link
Member Author

@cybersoulK yeah, there will be solution for that, however, keep in mind that some events are sync and will have return type, so it's a bit not clear what the best approach will, be but you'll be able to do what you're doing just fine in 0.30 for sure.

@madsmtm
Copy link
Member

madsmtm commented Jan 27, 2024

An update: @daxpedda, @kchibisov and I have discussed this a few times now, and are slowly itching towards an idea for a future v0.30.

This release is meant to be an iterative step that doesn't actually resolve most of the problems outlined here, but instead focuses on providing an upgrade path for our users from the closure-based design, to the trait-based design.

The point of this is to gather real-life feedback, and account for missing functionality. The existing closure-based API will still be available (although deprecated) in this version, so if you find that the migration is not possible for some reason or other, we will have a place for you to submit that feedback, and you can continue going about your business with the old API.

@madsmtm
Copy link
Member

madsmtm commented Jan 27, 2024

Additionally, as we've agreed to do this work iteratively, I've re-read all the comments on this issue, and attempted to open new, separate issues for each proposed change. The relevant issues are:

I believe I've included the major points from this discussion, but I've definitely missed something, so please look them over, and make sure that the points and ideas you've raised here won't be forgotten.


Since this issue has now accomplished its goal of communicating and getting the maintainers on board with a new direction for Winit, I'm going to close it, since it is not really actionable, and the discussion in here is all over the place and impossible to follow. Check out the milestones to follow what work will be done when.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S - api Design and usability
Development

No branches or pull requests