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

Windows: set_control_flow has no effect from AboutToWait event #3215

Closed
ogoffart opened this issue Nov 7, 2023 · 1 comment · Fixed by #3216
Closed

Windows: set_control_flow has no effect from AboutToWait event #3215

ogoffart opened this issue Nov 7, 2023 · 1 comment · Fixed by #3216
Labels
B - bug Dang, that shouldn't have happened DS - windows

Comments

@ogoffart
Copy link
Contributor

ogoffart commented Nov 7, 2023

Consider this modified version of the timer.rs example:

#![allow(clippy::single_match)]

use std::time::Duration;

use simple_logger::SimpleLogger;
use winit::{
    event::{Event, WindowEvent},
    event_loop::{ControlFlow, EventLoop},
    window::WindowBuilder,
};

#[path = "util/fill.rs"]
mod fill;

fn main() -> Result<(), impl std::error::Error> {
    SimpleLogger::new().init().unwrap();
    let event_loop = EventLoop::new().unwrap();

    let window = WindowBuilder::new()
        .with_title("A fantastic window!")
        .build(&event_loop)
        .unwrap();

    event_loop.run(move |event, elwt| {
        println!("{event:?}");

        match event {
            // New iteration: reset the control flow to Wait as we don't know if we will need to wait
            Event::NewEvents(_) => elwt.set_control_flow(ControlFlow::Wait),
            // ... some other event change internal data structures ...
            Event::AboutToWait => {
                // About to wait:  query internal data structure to know how long we need to sleep
                elwt.set_control_flow(ControlFlow::wait_duration(Duration::from_secs(1)));
            }

            Event::WindowEvent {
                event: WindowEvent::CloseRequested,
                ..
            } => elwt.exit(),
            Event::WindowEvent {
                event: WindowEvent::RedrawRequested,
                ..
            } => {
                fill::fill_window(&window);
            }
            _ => (),
        }
    })
}

The gist here is that we set ControlFlow::Wait from NewEvents.
In our application some events such as mouse or keyboard event may start timers in a timer or actions. So at the end of the iteration, in AboutToWait, we set the control flow to the computed waiting time.

That works on Linux: we get new event every second from that example.
But on Windows, this blocks forever unless you move the mouse or something.

I've tried debugging this quickly, and it seems like the wait_and_dispatch_message function queries the control_flow here:

let control_flow_timeout = match runner.control_flow() {

And then sends the AboutToWait shortly after via
runner.prepare_wait();

I believe this line should be moved before the line 363. Is that right?

@kchibisov
Copy link
Member

The change you've proposed sounds fine to me. Could you send a patch?

@kchibisov kchibisov added this to the Version 0.29.4 milestone Nov 7, 2023
kchibisov pushed a commit that referenced this issue Nov 8, 2023
In case the AboutToWait event sets the control flow to another value
it's not being used on this iteration.

Fixes #3215.
kchibisov pushed a commit to kchibisov/winit that referenced this issue Nov 24, 2023
In case the AboutToWait event sets the control flow to another value
it's not being used on this iteration.

Fixes rust-windowing#3215.
kchibisov pushed a commit that referenced this issue Nov 24, 2023
In case the AboutToWait event sets the control flow to another value
it's not being used on this iteration.

Fixes #3215.
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.
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
B - bug Dang, that shouldn't have happened DS - windows
Development

Successfully merging a pull request may close this issue.

2 participants