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

EventLoop 3.0 Changes #2900

Open
11 of 17 tasks
rib opened this issue Jun 22, 2023 · 84 comments
Open
11 of 17 tasks

EventLoop 3.0 Changes #2900

rib opened this issue Jun 22, 2023 · 84 comments
Labels
C - needs discussion Direction must be ironed out S - api Design and usability

Comments

@rib
Copy link
Contributor

rib commented Jun 22, 2023

Considering the changes being made for run()-> Result<>, pump_events, run_ondemand, and addressing inconsistent ordering guarantees for events like MainEventsCleared and RedrawRequested, as well as removing and renaming several core events, then it feels like a similar level of change as happened for EventLoop 2.0.

The details are a bit scattered though, and although some things have been agreed as part of lengthy issue discussions we still need to track those ideas at least as a reminder as to what is left to do.

I'm going to be lite on details here, but a lot of the background discussion probably happened here #2706 and maybe a few other issues we can link back here too.

I don't mind if we want to collate this information somewhere else, or in a different, better way etc but wanted to a least get a basic task list laid out somewhere

Maybe a milestone would be a nicer way to organize this?

@rib
Copy link
Contributor Author

rib commented Jun 22, 2023

CC: @kchibisov, @msiglreith, @madsmtm, @daxpedda

@notgull
Copy link
Member

notgull commented Jun 23, 2023

  • Maybe add run_noreturn() ->! extension API for iOS and Web

+1 to this. Maybe it could also be implemented for all of the non-Android platforms, which use std::process::exit() instead.

  • Make it clearer that Resumed events are delivered on all platforms (not just mobile) and Resumed events are a good place to initialize an application because it's the first point where it's possible to start creating Windows across all platforms.

Maybe we build this into the type system? I.e. Resumed returns a WindowCreationToken or something that can't be created outside of winit, that is needed to create windows?

The only other thing is that RedrawEventsCleared was nice for submitting the wgpu queue for when all redraws are done. It would be nice if something similar were exposed on platforms that support it.

Looks good to me, otherwise! Can't wait.

@kchibisov
Copy link
Member

+1 to this. Maybe it could also be implemented for all of the non-Android platforms, which use std::process::exit() instead.

This API only required on ios.

The only other thing is that RedrawEventsCleared was nice for submitting the wgpu queue for when all redraws are done. It would be nice if something similar were exposed on platforms that support it.

It'll be named AboutToPoll.

@kchibisov
Copy link
Member

Wrt MainEventsCleared and RedrawEventsCleared, I think MainEventsCleared is here to stay with us. The reason for that is that winit has a guarantee right now is that if you do request_redraw prior to MainEventsCleared including it, it'll be scheduled on this loop iteration.

While throttling adds a bit to that, I think it'll still be nice to have such entry point to potentially schedule the redraw on this loop iteration, so for example when after processing input events, you decided to request a redraw, so if on e.g. Wayland you already had frame callback, you could simply draw on this loop iteration reducing the overall latency.

@kchibisov
Copy link
Member

Wrt to window creation, we know that it's generally async, maybe we should expose it? Though, I'm thinking that it'll too much. But that, however will make it work with Resumed without a type system level token and such, you could request a window outside of the loop, but you'll get it only once you start the loop.

It could be like Window::request_new() -> Result<WindowId, Err>, so you're getting a WindowId ahead of time for your new window, thus you could have a spot for it in the HashMap for example. And then you receive an even with the Window, that way you won't have issues with calling inner_size way too early, because the window will be still in process, and you won't have an issue with callback stacking if we decide to call from e.g. wayland events being dispatched directly to the user.

But I fill like that could be a bit a change in convenience, but generally speaking it'll improve situation wrt correctness, given that you really want to create EGLSurface for Wayland with the right size from the start, and not with some arbitrary size you pick, it's possible right now, but users rely on their sizes and not on inner_size() calls, and on some systems inner_size() is not ready early on.

Besides, macOS has issues in general when it comes to creating windows out side of the loop, so some windowing features stop working (alacritty thus moved to Resumed event instead).

@rib
Copy link
Contributor Author

rib commented Jun 23, 2023

Wrt to window creation, we know that it's generally async, maybe we should expose it?

I think it could be good to expose that window creation is async and fix the awkward issue with Wayland right now with the sync roundtrip in Window::new() - that is having a really disproportionate affect on the whole design of the Wayland backend currently.

I don't know about really buffering requests to create a window right from before an event loop is started. It's always seemed a bit odd to me with Winit that you could create windows before starting to run an EventLoop.

The idea of initially just getting a WindowId sounds like it could work.

In terms of sizes to use with EGL there's another big mess imho with the inner_size API being overloaded which is a separate issue really.

It wouldn't necessarily always bad for apps to optimistically use a size that was used to request a new window for an egl surface if it e.g. means they don't have to wait for a round trip - if it's 99% likely they get what they asked for and the other 1% will correct itself via resize events etc.

@rib
Copy link
Contributor Author

rib commented Jun 23, 2023

The only other thing is that RedrawEventsCleared was nice for submitting the wgpu queue for when all redraws are done

There would no longer be any notion of having multiple redraw events within a single iteration of the loop - you just get (throttled) RedrawRequested events in response to calling request_redraw() - so now you would just submit work to the GPU as part of your RedrawRequested event handler (i.e. after you've finished actually drawing). If we kept a RedrawEventsCleared then conceptually it would just be called back-to-back, immediately after the RedrawRequested event - which makes it pretty much redundant.

@daxpedda
Copy link
Member

daxpedda commented Jun 23, 2023

I don't think we want a run_noreturn() -> ! as an extension API for Web if we already have spawn.
Is this desirable somehow?

@kchibisov
Copy link
Member

I also don't quite understand why we have an api which -> ! if web doesn't really need it.

The whole point was to move away of -> !, which is basically ios, and ios in my opinion could live in its own with -> !, given how niche the platform is for rust development, and that users targeting it likely have a separate path regardless.

The only issue is that I think spawn on web doesn't block at all? While the default API we want should block the caller until run finishes.

@daxpedda
Copy link
Member

The only issue is that I think spawn on web doesn't block at all? While the default API we want should block the caller until run finishes.

Yes, that unfortunately can't be fixed in Web.

@kchibisov
Copy link
Member

I think we've discussed in the past and we could have a way to block via the exception handler, basically the way run -> ! was implemented before.

@daxpedda
Copy link
Member

I think we've discussed in the past and we could have a way to block via the exception handler, basically the way run -> ! was implemented before.

Whats the "exception handler"? Right now run -> ! on Web is implemented by throwing a panic, which "works".

@kchibisov kchibisov added S - api Design and usability C - needs discussion Direction must be ironed out labels Jun 23, 2023
@rib
Copy link
Contributor Author

rib commented Jun 23, 2023

I think we've discussed in the past and we could have a way to block via the exception handler, basically the way run -> ! was implemented before.

Whats the "exception handler"? Right now run -> ! on Web is implemented by throwing a panic, which "works".

oh, sorry, I didn't mean 'exception handler' I meant the trick with throwing - just wasn't thinking when typing ;-p

@rib
Copy link
Contributor Author

rib commented Jun 23, 2023

it's not a panic that's thrown though is it? I thought it was done by punching through to javascript to throw an exception which Rust wouldn't see

@rib
Copy link
Contributor Author

rib commented Jun 23, 2023

which was an old hack from emscripten that used to be done to give the illusion of a function in C/C++ that doesn't exit

@rib
Copy link
Contributor Author

rib commented Jun 23, 2023

I don't think we want a run_noreturn() ->! as an extension API for Web if we already have spawn. Is this desirable somehow?

yeah I don't really feel like it's necessary for web.

for web you're going to have a bit of bespoke wasm_bindgen glue anyway so it's just not really an issue to use a different run function

@rib
Copy link
Contributor Author

rib commented Jun 23, 2023

it's pretty much just iOS that wants run_noreturn so I really don't see that it would be that nice to expose it across all platforms just for the sake of making the iOS behaviour into a lowest common denominator. Having every other platform hit process::exit() just feels really yukky to me.

technically though we could implement it for all platforms except Android I think, but that would kind of endorse it more than I feel like it should be.

@daxpedda
Copy link
Member

it's not a panic that's thrown though is it? I thought it was done by punching through to javascript to throw an exception which Rust wouldn't see

So right now we are just using the JS throw mechanism, which is kinda what you are describing. There isn't a big difference in practice between throwing directly and calling panic! though.

yeah I don't really feel like it's necessary for web.

I will remove it from OP then.

@rib
Copy link
Contributor Author

rib commented Jun 23, 2023

I'd guess iOS is probably also like Android and Web where you're very likely going to have a bespoke entrypoint that isn't just a plain fn main() so I'd guess it's also a non-issue for iOS to have a special case run_return() -> ! if they want to make it clearer that the function doesn't return.

@rib
Copy link
Contributor Author

rib commented Jun 23, 2023

There isn't a big difference in practice between throwing directly and calling panic! though.

The trick relies on Rust basically not being aware of the Javascript layer throwing the exception - it's transparent to the Rust compiler, so I think it's maybe quite an important distinction - a panic is something that Rust would definitely know about.

@rib
Copy link
Contributor Author

rib commented Jun 23, 2023

Maybe we build this into the type system? I.e. Resumed returns a WindowCreationToken or something that can't be created outside of winit, that is needed to create windows?

This idea would potentially work better if there was an event that corresponded to FirstResume which doesn't currently exist.

It would be tricky to do with Resumed events that can be emitted multiple times during the life of mobile apps.

It could be interesting to consider since it can be good when the API forces you into a portable pattern - if it doesn't break other important use cases.

@daxpedda
Copy link
Member

daxpedda commented Jun 23, 2023

There isn't a big difference in practice between throwing directly and calling panic! though.

The trick relies on Rust basically not being aware of the Javascript layer throwing the exception - it's transparent to the Rust compiler, so I think it's maybe quite an important distinction - a panic is something that Rust would definitely know about.

Panics on Wasm are implemented very crudely right now. Basically any sort of panic in Wasm just uses the unreachable instruction, which throws an exception in the browser environment. The Rust compiler doesn't do anything else here.

The important thing is that we don't trigger the panic handler, which a call to panic! would, because the user might have registered a custom panic handler, which might have unintended consequences. But the default panic handler and calling panic itself has no side effects except throwing an exception right now.

That is all until Rust gains Wasm unwinding support.

@madsmtm
Copy link
Member

madsmtm commented Jun 24, 2023

The problem in my eyes with run() not returning ! is that people assume that they can do cleanup / other things after the event loop is done, while in reality they cannot do that portably.

Though I didn't know about process::exit() being problematic on Android, I guess the downside I'm noting above is the lesser evil.

@daxpedda
Copy link
Member

The problem in my eyes with run() not returning ! is that people assume that they can do cleanup / other things after the event loop is done, while in reality they cannot do that portably.

That is a good point ... I didn't consider that.
Honestly I don't think we should change the default run() -> ! to run() -> Result then, because that's gonna be fundamentally incompatible with Wasm.

I think @kchibisov mentioned in the PR that we should consider removing run() -> ! for Wasm, which now sounds quite a bit better then offering run() -> Result in Wasm at all.

I am strongly in favor of not making the change to run() -> Result, it won't be cross-platform.

@rib
Copy link
Contributor Author

rib commented Jun 25, 2023

I am strongly in favor of not making the change to run() -> Result, it won't be cross-platform.

:( we've been around in circles so many times on this now.

We can't support run() ->! on Android because it's not acceptable to just kill the whole process as a way to not return and Rust has no API to just terminate a thread.

run() -> Result is the more cross-platform API.

Overall there's no perfect run() signature for all platforms.

Another option could be to rely on extensions for all platforms since there's no perfect solution but as it is run() -> Result can actually be supported on all platforms so we don't need to do that.

The ability to return a status is something that is really desirable and also possible on the majority of platforms except for iOS and Web.

It's notable that run() ->Result API doesn't need to be used on Web or iOS and since it's very common (esp for Web) to have extra glue for the entrypoint for the app it also seems pretty reasonable that for web you could use a special run_noreturn to better communicate to the Rust compiler that run() doesn't return.

It doesn't break Web or iOS for the compiler to not inform apps that the function doesn't return - that's an ergonomic hint and we can provide an extension API like run_noreturn to give you the ergonomics you want on Web and iOS.

@daxpedda
Copy link
Member

:( we've been around in circles so many times on this now.

Apologies, I wasn't here for that. I'm still new here!

We can't support run() ->! on Android because it's not acceptable to just kill the whole process as a way to not return and Rust has no API to just terminate a thread.

Yeah, that's a problem, I agree.

Another option could be to rely on extensions for all platforms since there's no perfect solution but as it is run() -> Result can actually be supported on all platforms so we don't need to do that.

I think this is the most reasonable option here.
As far as I can tell there is no one cross-platform API that fits the bill here. run() -> Result is not acceptable for API, this is a big footgun and the fact that this doesn't return is not reflected by the API, it just doesn't make any sense to me.


I would be in favor of just making this platform specific, or alternatively, just expose run() -> Result on all platforms except Web.

@Stargateur
Copy link

Stargateur commented Sep 21, 2023

doesn't change anything from user side of view, I don't know who call exit but anyway that what winit do by calling whatever that call exit. I know a bit of wayland and wayland-rs and I don't think they call exit either. Calling exit break so many thing from user, imagine a game where you would like try to save before a crash, you couldn't cause something call exit.

@kchibisov
Copy link
Member

I'm saying that no-one calls exit, but you can believe what you believe, since you somehow concluded that winit calls exit now.

@wleslie
Copy link

wleslie commented Sep 21, 2023

That said, the docs for EventLoop::run in 0.29.1-beta say "X11 / Wayland: The program terminates with exit code 1 if the display server disconnects."

One reasonable interpretation of that sentence is that winit calls std::process::exit in that situation. Since that's not true, then what is the right way to interpret that sentence? What's responsible for terminating the program with exit code 1, if not winit?

@rib
Copy link
Contributor Author

rib commented Sep 21, 2023

fwiw Winit did use to call process::exit in run and this was actually one of the specific things I was aiming to address for the Android backend since it was especially problematic that run() would assume it has taken ownership of the whole process, which might not be reasonable on Android if there are other services running in other threads (re: #2709)

Now that Winit's run() API can return a Result it no longer needs to call process::exit().

In general the previous use of process:exit() was specifically done so that run() could be declared to never return (i.e. like run() -> !) to (effectively) make every platform consistent with iOS which technically can't return from it's main loop once its started.

@kchibisov
Copy link
Member

That said, the docs for EventLoop::run in 0.29.1-beta say "X11 / Wayland: The program terminates with exit code 1 if the display server disconnects."

It could be just forgotten to update, since we don't and we always forward errors now, even from other functions. It's best to remove such references all together if you still see them in the docs. You can open a separate issue and I fix the docs once I have time.

It's just the claim was that we straight terminate, said on that issue, given that this issue was solely about removing any sort of such termination, which I treat as complete dismissal of what was said before on the issue and they just decided to attack without understanding and reading into the issue and following anything.

@Stargateur
Copy link

Stargateur commented Sep 22, 2023

It's just the claim was that we straight terminate, said on that issue, given that this issue was solely about removing any sort of such termination, which I treat as complete dismissal of what was said before on the issue and they just decided to attack without understanding and reading into the issue and following anything.

I didn't attack on anything, I say I also prefer winit without exit and ask if it was still the case cause the doc on beta still say there is exit for x11 and wayland. I asked you to read twice my comment and you proved you didn't. I literally quote the doc of beta where it's return Result, ask if it was still up to date, link to the beta doc (oops I didn't lol). What more do you want from me ? You make no effort to answer me.

@kchibisov
Copy link
Member

See this #3111 .

@kchibisov
Copy link
Member

process::exit() make me run away from winit. It's so evil to put an exit in a library, panic if you wish but don't exit in place of user.

I'd just say that the first sentence was made me decide to discard and not read much into the other part of message for the following reasons:

  1. Winit always had options to run without std::process::exit even when said function was used in run. The exit was justified and explicitly mentioned in the docs (not anymore) and why it was used (mostly ios, and I have no clue how it works now, the control flow will never return to the user of the run on ios).
  2. The std::process::exit is a common thing in the GUI world (look at gtk) and not entirely bad(only _exit is bad) to call when you can't really return control back. You can also run your functions on std::process::exit and that's how some libraries work (nvidia libs run their cleanup like that, panic = "abort" will prevent them to do so for example).
  3. Saying that panic is better is not correct, because it won't run any atexit hooks when the panic is basically abort. And panic = "abort" is common when you want to get backtraces from your GUI program unless you redirect and collect the logs. So you'd at least get the dump if something failed.

I'm sorry for being offensive, it's just, hard to understand what folks really want when they start their comments with a sentence making me not want to read the rest, and that the thing you said was only half the truth, given that winit always offered alternatives which never terminated with exit or panic and normally returned (Old polling stuff, then run_return, and now run becomes like that). It's hard to understand when ppl just extrapolate their feeling or just trying to offense, since as a maintainer, I get comments like that on daily basis not only in winit, so over the time your brain starts to ignore them and not even making you to read through them.

Just simply linking to the exact place in docs where you have question is always better and you could save a lot of time removing all the misunderstanding. I fixed it after @wleslie provided relevant link to the docs.

@epimeletes
Copy link
Contributor

Apologies for what may seem like bikeshedding, but run_ondemand should really be called run_on_demand, "ondemand" is not a word in the English language.

@rib
Copy link
Contributor Author

rib commented Sep 25, 2023

Yeah, would probably make sense. The name really just came from terms that were being used in the original github issues being addressed (#2431). It became the term we used for consistency to refer to that use case.

I guess I won't get a chance to make a PR to change it myself but it does sound like it would be reasonable to rename.

@kchibisov
Copy link
Member

fine with renaming, do you want to send a patch doing so?

github-merge-queue bot pushed a commit to bevyengine/bevy that referenced this issue Dec 21, 2023
# Objective

- Update winit dependency to 0.29

## Changelog

### KeyCode changes

- Removed `ScanCode`, as it was [replaced by
KeyCode](https://github.com/rust-windowing/winit/blob/master/CHANGELOG.md#0292).
- `ReceivedCharacter.char` is now a `SmolStr`, [relevant
doc](https://docs.rs/winit/latest/winit/event/struct.KeyEvent.html#structfield.text).
- Changed most `KeyCode` values, and added more.

KeyCode has changed meaning. With this PR, it refers to physical
position on keyboard rather than the printed letter on keyboard keys.

In practice this means:
- On QWERTY keyboard layouts, nothing changes
- On any other keyboard layout, `KeyCode` no longer reflects the label
on key.
- This is "good". In bevy 0.12, when you used WASD for movement, users
with non-QWERTY keyboards couldn't play your game! This was especially
bad for non-latin keyboards. Now, WASD represents the physical keys. A
French player will press the ZQSD keys, which are near each other,
Kyrgyz players will use "Цфыв".
- This is "bad" as well. You can't know in advance what the label of the
key for input is. Your UI says "press WASD to move", even if in reality,
they should be pressing "ZQSD" or "Цфыв". You also no longer can use
`KeyCode` for text inputs. In any case, it was a pretty bad API for text
input. You should use `ReceivedCharacter` now instead.

### Other changes
- Use `web-time` rather than `instant` crate.
(rust-windowing/winit#2836)
- winit did split `run_return` in `run_onDemand` and `pump_events`, I
did the same change in bevy_winit and used `pump_events`.
- Removed `return_from_run` from `WinitSettings` as `winit::run` now
returns on supported platforms.
- I left the example "return_after_run" as I think it's still useful.
- This winit change is done partly to allow to create a new window after
quitting all windows: emilk/egui#1918 ; this
PR doesn't address.
- added `width` and `height` properties in the `canvas` from wasm
example
(#10702 (comment))

## Known regressions (important follow ups?)
- Provide an API for reacting when a specific key from current layout
was released.
- possible solutions: use winit::Key from winit::KeyEvent ; mapping
between KeyCode and Key ; or .
- We don't receive characters through alt+numpad (e.g. alt + 151 = "ù")
anymore ; reproduced on winit example "ime". maybe related to
rust-windowing/winit#2945
- (windows) Window content doesn't refresh at all when resizing. By
reading rust-windowing/winit#2900 ; I suspect
we should just fire a `window.request_redraw();` from `AboutToWait`, and
handle actual redrawing within `RedrawRequested`. I'm not sure how to
move all that code so I'd appreciate it to be a follow up.
- (windows) unreleased winit fix for using set_control_flow in
AboutToWait rust-windowing/winit#3215 ; ⚠️ I'm
not sure what the implications are, but that feels bad 🤔

## Follow up 

I'd like to avoid bloating this PR, here are a few follow up tasks
worthy of a separate PR, or new issue to track them once this PR is
closed, as they would either complicate reviews, or at risk of being
controversial:
- remove CanvasParentResizePlugin
(#10702 (comment))
- avoid mentionning explicitly winit in docs from bevy_window ?
- NamedKey integration on bevy_input:
rust-windowing/winit#3143 introduced a new
NamedKey variant. I implemented it only on the converters but we'd
benefit making the same changes to bevy_input.
- Add more info in KeyboardInput
#10702 (review)
- #9905 added a workaround on a
bug allegedly fixed by winit 0.29. We should check if it's still
necessary.
- update to raw_window_handle 0.6
  - blocked by wgpu
- Rename `KeyCode` to `PhysicalKeyCode`
#10702 (comment)
- remove `instant` dependency, [replaced
by](rust-windowing/winit#2836) `web_time`), we'd
need to update to :
  - fastrand >= 2.0
- [`async-executor`](https://github.com/smol-rs/async-executor) >= 1.7
    - [`futures-lite`](https://github.com/smol-rs/futures-lite) >= 2.0
- Verify license, see
[discussion](#8745 (comment))
  - we might be missing a short notice or description of changes made
- Consider using https://github.com/rust-windowing/cursor-icon directly
rather than vendoring it in bevy.
- investigate [this
unwrap](#8745 (comment))
(`winit_window.canvas().unwrap();`)
- Use more good things about winit's update
- #10689 (comment)
## Migration Guide

This PR should have one.
@simonask
Copy link

simonask commented Dec 30, 2023

(Not sure if this is the right place to ask, so feel free to hide/delete this comment.)

First off, thanks for all the hard work!

I'm migrating my codebase (a game) to use winit 0.29, and I'm really confused by some of the guidance in the docs, but I'm also unsure how much of it is intentional or lacking design details. Here are some questions that popped up:

  1. I noticed that resize behavior changed on Windows (ControlFlow is ignored while window is being resized on Windows #3272), seemingly due to less magic happening around the resize modal loop, causing ControlFlow::Poll to not trigger AboutToWait during a resize. This maps well to the platform behavior, but AFAICT there is no longer any way to get continuous updates during the resize modal loop. You can hook into the Resized event, but it is only emitted when the window is actually resized (the mouse moved while dragging the corners or the window). Is there a generally suggested workaround?

  2. The most obvious workaround is to simply update/draw on a dedicated thread, independent of the event loop. This simplifies some things, but adds machinery to schlep events between threads. But the documentation for pump_events() discourages buffering events, because some events on some platforms need to be handled within the callback. This in turn discourages threading as a solution for the problem described in (1), whether or not pump_events() is being used or not.

  3. Ok fine, so let's handle events on the main thread, and just render on a background thread. But one of the events that pump_events() mentions needs to be handled in the callback is RedrawRequested, in order to avoid visual tearing. This seems like a much too strong claim for the use case of rendering to a surface using for example wgpu, where my assumption would be that the presentation machinery (specifically VSync) would take care of this synchronization. In other words, unless I'm mistaken, the guidance should be to either render in RedrawRequested OR ensure some other kind of synchronization, like GPU presentation modes. I could be wrong here, but wgpu should be using displayLink internally on macOS/iOS to achieve this, leaving winit out of the picture. Furthermore, it's a really common design in game engines to have a dedicated render thread, that isn't the main thread, which takes care of command buffer submission and presentation.

  4. Another potential workaround to (1) is to call request_redraw() inside the handling of RedrawRequested while resizing. This seems super ugly, and kind of distributes the responsibility of maintaining the desired behavior of ControlFlow::Poll to multiple places in the code for a very platform-specific quirk.

  5. (EDIT) The docs also generally discourage using AboutToWait to do anything useful, and instead using RedrawRequested to do continuous rendering, implying that request_redraw() is called at the end of every frame to schedule the next frame for rendering. But then what is the intended use of ControlFlow::Poll?

Really sorry if I'm missing something obvious. I'm not really sure what is actionable about all this - the changes seem really good overall, and I like that there is less magic going on around the quirks of the Windows event loop. Might just be a documentation issue. :-)

@kchibisov
Copy link
Member

. This maps well to the platform behavior, but AFAICT there is no longer any way to get continuous updates during the resize modal loop. You can hook into the Resized event, but it is only emitted when the window is actually resized (the mouse moved while dragging the corners or the window). Is there a generally suggested workaround?

that's a bug that should be fixed given that it's a windows specific issue.

where my assumption would be that the presentation machinery (specifically VSync) would take care of this synchronization. In other words, unless I'm mistaken, the guidance should be to either render in RedrawRequested OR ensure some other kind of synchronization, like GPU presentation modes. I could be wrong here, but wgpu should be using displayLink internally on macOS/iOS to achieve this, leaving winit out of the picture. Furthermore, it's a really common design in game engines to have a dedicated render thread, that isn't the main thread, which takes care of command buffer submission and presentation.

This is only on macOS since it really wants you to draw right from this event otherwise you may get a bit undesired artifacts, but it's not tearing, not.

Another potential workaround to (1) is to call request_redraw() inside the handling of RedrawRequested while resizing. This seems super ugly, and kind of distributes the responsibility of maintaining the desired behavior of ControlFlow::Poll to multiple places in the code for a very platform-specific quirk.

Calling redraw_requested from RedrawRequested itself is supported and guaranteed to work on all the platforms. It'll properly send it on the next frame.

(EDIT) The docs also generally discourage using AboutToWait to do anything useful, and instead using RedrawRequested to do continuous rendering, implying that request_redraw() is called at the end of every frame to schedule the next frame for rendering. But then what is the intended use of ControlFlow::Poll?

To spin the loop as fast as you can? Like not sure why it shouldn't exist given that it's pretty common for event loop to have wait, wait with time out, and instant return when no events.

@simonask
Copy link

that's a bug that should be fixed given that it's a windows specific issue.

Thank you for clarifying this!

This is only on macOS since it really wants you to draw right from this event otherwise you may get a bit undesired artifacts, but it's not tearing, not.

It's still unclear to me what the guidance actually is here. Is it the case for all use cases that any drawing on the window's surface, including presentation to a GPU surface, should happen within RedrawRequested on macOS? Maybe this question actually belongs in wgpu.

Really sorry if this is hijacking the issue, please let me know if there is a better place to put these questions!

@kchibisov
Copy link
Member

It's still unclear to me what the guidance actually is here. Is it the case for all use cases that any drawing on the window's surface, including presentation to a GPU surface, should happen within RedrawRequested on macOS?

it's recommended and how apply expects you.

Really sorry if this is hijacking the issue, please let me know if there is a better place to put these questions!

you should ask such questions on matrix or discussions.

@kchibisov kchibisov added this to the Version 0.31.0 milestone Jan 15, 2024
@yanchith
Copy link
Contributor

Hi all! Came here after attempting to upgrade to 29. I am wondering, how should one structure a loop for videogames these days.

In 28, we had MainEventsCleared and RedrawRequested. I used to do game simulation code in MainEventsCleared and then manually fire window.request_redraw(). Now that MainEventsCleared got removed, where does one put the simulation code?

I notice there's AboutToWait, but the docs say I shouldn't need to use it?

Most applications shouldn’t need to hook into this event since there is no real relationship between how often the event loop needs to wake up and the dispatching of any specific events.

Thanks!

@daxpedda
Copy link
Member

If you want to draw or update in VSync, you can do that inside WindowEvent::RedrawRequested and call Window::request_redraw() (see #3406) there as well.

If you want to draw or update in a specific interval, you can use ControlFlow::WaitUntil to schedule that interval, and either do your thing in Event::NewEvents or Event::AboutToWait.

If you want to draw or update as fast possible, which is not recommended as it will just consume 100% CPU, you can use ControlFlow::Poll and do your thing in Event::NewEvents or Event::AboutToWait as well.

@JoeOsborn
Copy link

This contradicts the guidance of https://docs.rs/winit/latest/winit/event_loop/enum.ControlFlow.html , and I guess in WGPU at least waiting for the swapchain image can block which is how vsync throttling typically is done for that API. Is your note here about the case where you don't have a 3D API, or are the docs wrong here?

@daxpedda
Copy link
Member

The documentation is poor and wrong in some places.

Unfortunately there are some inconsistencies here, unfortunately Wgpu doesn't always block for VSync, e.g. Web doesn't, so if you use ControlFlow::Poll on Web with Wgpu it will just break your application (throttle it into oblivion).

The fact is that there is no ideal solution here until either Wgpu improves (gfx-rs/wgpu#2869, gfx-rs/wgpu#3283) or Winit finishes #2412.

But if you have a reliable method blocking until VSync and you don't want to handle any events while doing that, ControlFlow::Poll can work fine as well.

Subserial pushed a commit to Subserial/bevy_winit_hook that referenced this issue Feb 20, 2024
# Objective

- Update winit dependency to 0.29

## Changelog

### KeyCode changes

- Removed `ScanCode`, as it was [replaced by
KeyCode](https://github.com/rust-windowing/winit/blob/master/CHANGELOG.md#0292).
- `ReceivedCharacter.char` is now a `SmolStr`, [relevant
doc](https://docs.rs/winit/latest/winit/event/struct.KeyEvent.html#structfield.text).
- Changed most `KeyCode` values, and added more.

KeyCode has changed meaning. With this PR, it refers to physical
position on keyboard rather than the printed letter on keyboard keys.

In practice this means:
- On QWERTY keyboard layouts, nothing changes
- On any other keyboard layout, `KeyCode` no longer reflects the label
on key.
- This is "good". In bevy 0.12, when you used WASD for movement, users
with non-QWERTY keyboards couldn't play your game! This was especially
bad for non-latin keyboards. Now, WASD represents the physical keys. A
French player will press the ZQSD keys, which are near each other,
Kyrgyz players will use "Цфыв".
- This is "bad" as well. You can't know in advance what the label of the
key for input is. Your UI says "press WASD to move", even if in reality,
they should be pressing "ZQSD" or "Цфыв". You also no longer can use
`KeyCode` for text inputs. In any case, it was a pretty bad API for text
input. You should use `ReceivedCharacter` now instead.

### Other changes
- Use `web-time` rather than `instant` crate.
(rust-windowing/winit#2836)
- winit did split `run_return` in `run_onDemand` and `pump_events`, I
did the same change in bevy_winit and used `pump_events`.
- Removed `return_from_run` from `WinitSettings` as `winit::run` now
returns on supported platforms.
- I left the example "return_after_run" as I think it's still useful.
- This winit change is done partly to allow to create a new window after
quitting all windows: emilk/egui#1918 ; this
PR doesn't address.
- added `width` and `height` properties in the `canvas` from wasm
example
(bevyengine/bevy#10702 (comment))

## Known regressions (important follow ups?)
- Provide an API for reacting when a specific key from current layout
was released.
- possible solutions: use winit::Key from winit::KeyEvent ; mapping
between KeyCode and Key ; or .
- We don't receive characters through alt+numpad (e.g. alt + 151 = "ù")
anymore ; reproduced on winit example "ime". maybe related to
rust-windowing/winit#2945
- (windows) Window content doesn't refresh at all when resizing. By
reading rust-windowing/winit#2900 ; I suspect
we should just fire a `window.request_redraw();` from `AboutToWait`, and
handle actual redrawing within `RedrawRequested`. I'm not sure how to
move all that code so I'd appreciate it to be a follow up.
- (windows) unreleased winit fix for using set_control_flow in
AboutToWait rust-windowing/winit#3215 ; ⚠️ I'm
not sure what the implications are, but that feels bad 🤔

## Follow up 

I'd like to avoid bloating this PR, here are a few follow up tasks
worthy of a separate PR, or new issue to track them once this PR is
closed, as they would either complicate reviews, or at risk of being
controversial:
- remove CanvasParentResizePlugin
(bevyengine/bevy#10702 (comment))
- avoid mentionning explicitly winit in docs from bevy_window ?
- NamedKey integration on bevy_input:
rust-windowing/winit#3143 introduced a new
NamedKey variant. I implemented it only on the converters but we'd
benefit making the same changes to bevy_input.
- Add more info in KeyboardInput
bevyengine/bevy#10702 (review)
- bevyengine/bevy#9905 added a workaround on a
bug allegedly fixed by winit 0.29. We should check if it's still
necessary.
- update to raw_window_handle 0.6
  - blocked by wgpu
- Rename `KeyCode` to `PhysicalKeyCode`
bevyengine/bevy#10702 (comment)
- remove `instant` dependency, [replaced
by](rust-windowing/winit#2836) `web_time`), we'd
need to update to :
  - fastrand >= 2.0
- [`async-executor`](https://github.com/smol-rs/async-executor) >= 1.7
    - [`futures-lite`](https://github.com/smol-rs/futures-lite) >= 2.0
- Verify license, see
[discussion](bevyengine/bevy#8745 (comment))
  - we might be missing a short notice or description of changes made
- Consider using https://github.com/rust-windowing/cursor-icon directly
rather than vendoring it in bevy.
- investigate [this
unwrap](bevyengine/bevy#8745 (comment))
(`winit_window.canvas().unwrap();`)
- Use more good things about winit's update
- bevyengine/bevy#10689 (comment)
## Migration Guide

This PR should have one.
@kchibisov kchibisov unpinned this issue May 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - needs discussion Direction must be ironed out S - api Design and usability
Development

No branches or pull requests