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

Input event order information is lost #5984

Closed
aevyrie opened this issue Sep 14, 2022 · 7 comments · Fixed by #14862 · May be fixed by #9034
Closed

Input event order information is lost #5984

aevyrie opened this issue Sep 14, 2022 · 7 comments · Fixed by #14862 · May be fixed by #9034
Labels
A-Input Player input via keyboard, mouse, gamepad, and more C-Bug An unexpected or incorrect behavior
Milestone

Comments

@aevyrie
Copy link
Member

aevyrie commented Sep 14, 2022

Bevy version

All versions

What you did

Read bevy mouse events and determine the order of the incoming events

What went wrong

Because input streams are split up, it's impossible to order these events correctly. This is especially important if there is a long pause in rendering for some reason, and you still want to ensure behavior is consistent. If there are multiple mouse moves and button presses, it's important to know which came first.

If a mouse move precedes a mouse up, then dragging occurred followed by a drop. If the mouse move happens after the mouse up, then a drop occurred before the move. If a mouse up happened between two mouse move events in bevy, it's impossible to know where the mouse actually was at the time of the drop.

This is solved in other places by merging all events into a single event stream, disambiguating event order.winit does this as well, but bevy splits these events up before passing them into the bevy event system.

Maybe it would be helpful to pass in the raw winit window event stream?

@aevyrie aevyrie added C-Bug An unexpected or incorrect behavior A-Input Player input via keyboard, mouse, gamepad, and more labels Sep 14, 2022
@alice-i-cecile
Copy link
Member

+1 on this; this prevents my input playback work from accurately capturing within-frame ordering.

@DevinLeamy
Copy link
Contributor

Since GamepadEventRaw is sourced from gilrs and the other input events (e.g. KeyboardInput and MouseInput) are sourced from winit, it makes aggregating all pressable input events into one event stream (maintaining their order) challenging.

We could just focus on ordering all winit events, using some generic PressableInputEvent but this event stream would only have raw input events (KeyboardInput) rather than the "sanitized" versions (KeyCode/ScanCode), because this "sanitization" takes place "down the line".

Another idea would be to tag each input event with a "timestamp" denoting the place in the input stream where it was received. E.g. the first event would have ts = 1, the second ts = 2, third ts = 3, etc.. We could then handle the raw input events in their own separate systems, like we do now, but add them to a generic Res<Input> that would manage all pressable input events and would allow iterating over them in an ordered fashion.

fn ordered_input_system(input: Res<Input>) {
    // Iterate over events in an ordered fashion.
    for input_event in input.iter() {
       match input_event {
         JustPressed(KeyCode::Space) => todo!()
         JustPressed(MouseButton::Left) => todo!()
         JustReleased(MouseButton::Left) => todo!()
       };
    }
}

fn standard_input_handling(input: Res<Input>) {
    // Keyboard example
    let spacebar_pressed = input.just_pressed(KeyCode::Space)
		
    // Gamepad example 
    let gamepad = gamepads.iter().next().unwrap();
    let gamepad_button = GamepadButton::new(gamepad, GamepadButtonType::South);
    let gamepad_button_pressed = input.pressed(gamepad_button);

    // Mouse button example
    let mouse_clicked = input.just_released(MouseButton::Left);
}

I like this idea because it doesn't force us to change the input-specific event handling code (GamepadEventRaw -> GamepadButton or KeyboardInput -> KeyCode/ScanCode), it also works for all pressable input events (we would just add this event timestamp to gamepad input events; their relative ordering would be maintained but their ordering relative to winit events would remain unknown), it unifies pressable inputs into one type which simplifies the API (and leads to less confusion for beginners!), and provides an intuitive way to read out input events in an ordered way.

I would appreciate feedback on this design, and any considerations I might have overlooked.

@alice-i-cecile
Copy link
Member

Ordering only winit events is definitely the most important goal here.

[A]dd them to a generic Res that would manage all pressable input events and would allow iterating over them in an ordered fashion.

I like this idea! We should carefully consider whether this unified stream is extensible externally, and how to do so without losing serious performance (latency matters).

I'm also a bit nervous about keeping Yet Another Copy of input event data around, both for "single source of truth" and memory impact reasons.

@believeinlain
Copy link

believeinlain commented Apr 2, 2023

This is relevant to what I'm working on as well. I'm mainly concerned with preserving the order of winit events, so I'm not sure about how they would be unified with gamepad events, but looking at the bevy source, ordering just the winit events seems fairly straightforward since they're all processed in the same function.

However, I'm not sure whether adding a unified WindowEvent type or adding a timestamp to each window event would be better. A Duration is 16 bytes on my system, so adding one to each event would more than double the size of many of them, but unifying the events with an enum would make them all the same size and CursorMoved is 16 bytes already.

So I'm tempted to fix this myself and put in a pull request, but I'm not sure which solution to pursue - the memory footprint for either approach seems similar, but I think adding a timestamp to all winit events would have a slightly smaller footprint, as unifying the events and making a copy would take up 32 bytes for each event at minimum, whereas adding a timestamp would only add 16 bytes to each event, so the smaller events would be much less than 32 bytes each.

Edit: on second thought I'm not sure about the memory footprint here - I was thinking we replace the event types with an enum, but if we only add an enum for the secondary unified queue, leaving the existing event queues unchanged, then it would have a comparable memory footprint (adding about 16 bytes per event). There's still the "single source of truth" issue with this approach though.

Perhaps instead of a timestamp, we just enumerate the winit events processed each frame? I imagine a usize would be sufficient, so that would add at most add 8 bytes to each event. That would potentially be trickier to unify with other types of events, but the timestamp method wouldn't be perfect for that either, since we can only record the time each event is polled (unless we get it from the OS - I believe Windows API has support for this, but I'm not sure about other OSs).

@stefnotch
Copy link

Please do correct me if I'm wrong, I'm somewhat new to low level languages.

The memory footprint of either approach seems very small, since input events get discarded away after 1 or 2 frames. And a user would typically not produce more than say 10 input events in one frame.

So I'd suggest picking whichever option is simpler to implement while also being as correct as possible.

@alice-i-cecile
Copy link
Member

I really want this, but there's no open PR for this and it's not trivial. Moving to 0.12 milestone instead, sorry!

@cfognom
Copy link

cfognom commented Nov 1, 2023

If winit and gilrs have timestamps for their events I think it should be exposed in bevy too. Not only is order important but also the exact timing so we accurately can calculate "time held" and so on, even in low framerate situations.

@alice-i-cecile alice-i-cecile modified the milestones: 0.13, 0.14 Jan 24, 2024
@alice-i-cecile alice-i-cecile modified the milestones: 0.14, 0.15 May 13, 2024
github-merge-queue bot pushed a commit that referenced this issue Sep 4, 2024
# Objective

Correctly order picking events. Resolves
#5984.

## Solution

Event ordering [very long standing
problem](aevyrie/bevy_mod_picking#294) with
mod picking, stemming from two related issues. The first problem was
that `Pointer<T>` events of different types couldn't be ordered, but we
have already gotten around that in the upstream by switching to
observers. Since observers run in the order they are triggered, this
isn't an issue.

The second problem was that the underlying event streams that picking
uses to create it's pointer interaction events *also* lacked ordering,
and the systems that generated the points couldn't interleave events.
This PR fixes that by unifying the event streams and integrating the
various interaction systems.

The concrete changes are as follows:
+ `bevy_winit::WinitEvent` has been moved to `bevy_window::WindowEvent`.
This provides a unified (and more importantly, *ordered*) input stream
for both `bevy_window` and `bevy_input` events.
+ Replaces `InputMove` and `InputPress` with `PointerInput`, a new
unified input event which drives picking and interaction. This event is
built to have drop-in forward compatibility with [winit's upcoming
pointer abstraction](rust-windowing/winit#3876).
I have added code to emulate it using the current winit input
abstractions, but this entire thing will be much more robust when it
lands.
+ Rolls `pointer_events` `send_click_and_drag_events` and
`send_drag_over_events` into a single system, which operates directly on
`PointerEvent` and triggers observers as output.

The PR also improves docs and takes the opportunity to
refactor/streamline the pointer event dispatch logic.

## Status & Testing

This PR is now feature complete and documented. While it is
theoretically possible to add unit tests for the ordering, building the
picking mocking for that will take a little while.

Feedback on the chosen ordering of events is within-scope.

## Migration Guide

For users switching from `bevy_mod_picking` to `bevy_picking`:
+ Instead of adding an `On<T>` component, use `.observe(|trigger:
Trigger<T>|)`. You may now apply multiple handlers to the same entity
using this command.
+ Pointer interaction events now have semi-deterministic ordering which
(more or less) aligns with the order of the raw input stream. Consult
the docs on `bevy_picking::event::pointer_events` for current
information. You may need to adjust your event handling logic
accordingly.
+ `PointerCancel` has been replaced with `Pointer<Cancled>`, which now
has the semantics of an OS touch pointer cancel event.
+ `InputMove` and `InputPress` have been merged into `PointerInput`. The
use remains exactly the same.
+ Picking interaction events are now only accessible through observers,
and no `EventReader`. This functionality may be re-implemented later.

For users of `bevy_winit`:
+ The event `bevy_winit::WinitEvent` has moved to
`bevy_window::WindowEvent`. If this was the only thing you depended on
`bevy_winit` for, you should switch your dependency to `bevy_window`.
+ `bevy_window` now depends on `bevy_input`. The dependencies of
`bevy_input` are a subset of the existing dependencies for `bevy_window`
so this should be non-breaking.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Input Player input via keyboard, mouse, gamepad, and more C-Bug An unexpected or incorrect behavior
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants