Add WinitEvent aggregate event for synchronized window event reading #12100

UkoeHB · 2024-02-24T20:39:05Z

Objective

Allow users to read window events in the sequence they appeared. This is important for precise input handling when there are multiple input events in a single frame (e.g. click and release vs release and click).

Solution

Add a mega-enum WinitEvent that collects window events, and send those alongside the existing more granular window events.

Changelog

Added WinitEvent event that aggregates all window events into a synchronized event stream.

alice-i-cecile · 2024-02-24T20:44:00Z

I think that even if we add timestamps to input events, this is still worth having around. It saves users from having to reconstitute this event stream themselves.

UkoeHB · 2024-02-24T20:57:05Z

I am getting libc++abi: terminating with uncaught foreign exception on the alien_cake_addict example (which is failing in CI). No idea why this PR would cause that. Edit: this was caused by not registering the WinitEvent event in the app.

james7132 · 2024-02-24T21:00:50Z

crates/bevy_winit/src/lib.rs

                WindowEvent::KeyboardInput { ref event, .. } => {
                    if event.state.is_pressed() {
                        if let Some(char) = &event.text {
                            let char = char.clone();
-                            app.send_event(ReceivedCharacter { window, char });
+                            winit_events.send(ReceivedCharacter { window, char });


If we're no longer using those other event types, can we remove them? or are we to double-send events along both queues?

Yeah events are double-sent now, events are dispatched before each app update.

crates/bevy_winit/src/lib.rs

crates/bevy_winit/src/winit_event.rs

aevyrie

This is a very welcome change, thank you.

My only suggestions would be to send the events directly into the app, instead of allocating an independent Vec and running the newly added flush function. That should allow the app's internal event resource to only pay price of allocation once, instead of every event loop. This adds an entry to the existing event_writer_system_state, and cuts about 70 LOC. I opened a PR against this PR with those changes here: UkoeHB#1

UkoeHB · 2024-03-03T02:28:41Z

That should allow the app's internal event resource to only pay price of allocation once, instead of every event loop.

The Vec allocated here is only allocated once, and then cleared every iteration.

This adds an entry to the existing event_writer_system_state, and cuts about 70 LOC

The PR you opened removes the step where you forward WinitEvents as individual events to Bevy, which will not pass CI.

I agree that your solution feels a bit cleaner, but I'm not sure how you would forward winit events properly using that approach.

aevyrie · 2024-03-03T21:26:12Z

The Vec allocated here is only allocated once, and then cleared every iteration.

Yup, you're right, I always get a bit confused following the control flow of the runner/event loop. 😄

The PR you opened removes the step where you forward WinitEvents as individual events to Bevy, which will not pass CI.

Maybe I missed something, but in the PR, I'm accessing the EventWriter<WinitEvent> directly using the existing system state, and sending events into it.

https://github.com/UkoeHB/bevy/pull/1/files#diff-b1447f30819280931099e7498b2ced388a98de5cda1b631b493107bb96193cc0R304

UkoeHB · 2024-03-04T00:10:29Z

Maybe I missed something, but in the PR, I'm accessing the EventWriter directly using the existing system state, and sending events into it.

Yep and that works great for the aggregate WinitEvent, but WinitEvent also needs to be unwrapped and each internal variant dispatched as independent events. That's what forward_winit_events() was doing, which your PR deleted.

# Objective - CI is green ## Solution - Fix typos (#12045) - Correctly declare features (#12100)

aevyrie · 2024-03-04T19:02:36Z

That's what forward_winit_events() was doing, which your PR deleted.

Yes, I missed that. Thanks again for getting this change in, looking forward to properly ordered input events!

…evyengine#12100) # Objective - Allow users to read window events in the sequence they appeared. This is important for precise input handling when there are multiple input events in a single frame (e.g. click and release vs release and click). ## Solution - Add a mega-enum `WinitEvent` that collects window events, and send those alongside the existing more granular window events. --- ## Changelog - Added `WinitEvent` event that aggregates all window events into a synchronized event stream.

# Objective - CI is green ## Solution - Fix typos (bevyengine#12045) - Correctly declare features (bevyengine#12100)

extrawurst · 2024-03-12T13:13:39Z

In what release will this land? The milestone is not set

alice-i-cecile · 2024-03-12T13:54:53Z

By default merged PRs are shipped in the next major version of Bevy. In this case, 0.14 :)

extrawurst · 2024-03-12T15:11:14Z

Gotcha!

UkoeHB added 2 commits February 24, 2024 14:35

add WinitEvent for synchronized window event reading

84b29bd

fmt

c9f09ab

alice-i-cecile requested review from aevyrie and maniwani February 24, 2024 20:43

alice-i-cecile added C-Feature A new feature, making something new possible A-Input Player input via keyboard, mouse, gamepad, and more labels Feb 24, 2024

toml format

5733ba4

james7132 reviewed Feb 24, 2024

View reviewed changes

UkoeHB commented Feb 24, 2024

View reviewed changes

crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved

bug fixes

09fd855

UkoeHB commented Feb 24, 2024

View reviewed changes

crates/bevy_winit/src/lib.rs Show resolved Hide resolved

UkoeHB added 3 commits February 24, 2024 15:29

clippy

8053c26

redundantly drain events to ensure work is spread out

eb8e2a0

justify redundant event draining

2797f3f

alice-i-cecile reviewed Feb 24, 2024

View reviewed changes

crates/bevy_winit/src/winit_event.rs Outdated Show resolved Hide resolved

alice-i-cecile reviewed Feb 24, 2024

View reviewed changes

crates/bevy_winit/src/winit_event.rs Show resolved Hide resolved

clarifications

637b015

alice-i-cecile approved these changes Feb 24, 2024

View reviewed changes

alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Feb 24, 2024

shave some work

a9cd54b

alice-i-cecile mentioned this pull request Feb 25, 2024

Ui Editable TextBox #12109

Closed

aevyrie reviewed Mar 2, 2024

View reviewed changes

james7132 approved these changes Mar 3, 2024

View reviewed changes

alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 3, 2024

alice-i-cecile added this pull request to the merge queue Mar 3, 2024

Merged via the queue into bevyengine:main with commit ef8a617 Mar 4, 2024
27 of 28 checks passed

mockersf mentioned this pull request Mar 4, 2024

Fix failures of typos and check-cfg #12293

Merged

github-merge-queue bot pushed a commit that referenced this pull request Mar 4, 2024

Fix failures of typos and check-cfg (#12293)

b1a5aab

# Objective - CI is green ## Solution - Fix typos (#12045) - Correctly declare features (#12100)

alice-i-cecile mentioned this pull request Mar 6, 2024

Add the ability to have multiple callbacks in a single listener aevyrie/bevy_eventlistener#21

Open

UkoeHB deleted the winit_event branch March 8, 2024 00:53

spectria-limina pushed a commit to spectria-limina/bevy that referenced this pull request Mar 9, 2024

Fix failures of typos and check-cfg (bevyengine#12293)

14f4c56

# Objective - CI is green ## Solution - Fix typos (bevyengine#12045) - Correctly declare features (bevyengine#12100)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add WinitEvent aggregate event for synchronized window event reading #12100

Add WinitEvent aggregate event for synchronized window event reading #12100

UkoeHB commented Feb 24, 2024

alice-i-cecile commented Feb 24, 2024

UkoeHB commented Feb 24, 2024 •

edited

Loading

james7132 Feb 24, 2024

UkoeHB Feb 24, 2024 •

edited

Loading

aevyrie left a comment •

edited

Loading

UkoeHB commented Mar 3, 2024

aevyrie commented Mar 3, 2024 •

edited

Loading

UkoeHB commented Mar 4, 2024

aevyrie commented Mar 4, 2024

extrawurst commented Mar 12, 2024

alice-i-cecile commented Mar 12, 2024

extrawurst commented Mar 12, 2024

Add WinitEvent aggregate event for synchronized window event reading #12100

Add WinitEvent aggregate event for synchronized window event reading #12100

Conversation

UkoeHB commented Feb 24, 2024

Objective

Solution

Changelog

alice-i-cecile commented Feb 24, 2024

UkoeHB commented Feb 24, 2024 • edited Loading

james7132 Feb 24, 2024

Choose a reason for hiding this comment

UkoeHB Feb 24, 2024 • edited Loading

Choose a reason for hiding this comment

aevyrie left a comment • edited Loading

Choose a reason for hiding this comment

UkoeHB commented Mar 3, 2024

aevyrie commented Mar 3, 2024 • edited Loading

UkoeHB commented Mar 4, 2024

aevyrie commented Mar 4, 2024

extrawurst commented Mar 12, 2024

alice-i-cecile commented Mar 12, 2024

extrawurst commented Mar 12, 2024

UkoeHB commented Feb 24, 2024 •

edited

Loading

UkoeHB Feb 24, 2024 •

edited

Loading

aevyrie left a comment •

edited

Loading

aevyrie commented Mar 3, 2024 •

edited

Loading