Implement the pointer event overhaul #3876

daxpedda · 2024-08-16T22:19:29Z

Description

This implements #3833, please see it for all the details.

This implementation deviates on PointerLeft::position, which is now an Option, because Windows (and maybe MacOS) doesn't have this kind of information.
It also adds FingerId to PointerType, which was an oversight of #3833.

Notably I also removed pen/stylus handling in iOS and Windows, which is now emitted as PointerType::Unknown.
I plan to reintroduce the same code and expand on it in #3810.

#3874 was a side-product of this PR.

Summary of the changes

Rename CursorMoved to PointerMoved.
Rename CursorEntered to PointerEntered.
Rename CursorLeft to PointerLeft.
Rename MouseInput to PointerButton.
Add position to every PointerEvent.
Remove Touch, which is folded into the Pointer* events.
New PointerType added to PointerEntered and PointerLeft, signifying which pointer type is the source of this event.
New PointerSource added to PointerMoved, similar to PointerType but holding additional data.
New ButtonSource added to PointerButton, similar to PointerType but holding pointer type specific buttons. Use ButtonSource::mouse_button() to easily normalize any pointer button type to a generic mouse button.
In the same spirit rename DeviceEvent::MouseMotion to PointerMotion.
Remove Force::Calibrated::altitude_angle.

Follow-ups

Implement pen/stylus input in Pen/Stylus support #3810.
Add touch pressure support to X11.
Add a reliable way to detect pointer types to X11.
Potentially differentiate between TouchPad and TouchScreen. Touch will remain in place for backends who can not differentiate between these pointer types.

Addresses #1555.
Addresses #99.
Fixes #336.
Fixes #883.
Fixes #1909.
Replaces #3001.

src/platform_impl/apple/appkit/view.rs

madsmtm

Awesome! I'm all for getting closer to the web spec, since there's clearly a lot of people that have thought longer about the problem than I ever have, nor will! And thanks for PR-ing it, it's much easier to talk about it when seeing the code!

Only the PointerLeft.position definition is a blocking concern for me (the macOS impl now doesn't emit PointerLeft because of it), the rest do not need to be resolved for this to still be an improvement.

src/event.rs

madsmtm · 2024-08-18T20:08:50Z

src/event.rs

+    /// A [`WindowEvent::PointerLeft`] without a [`WindowEvent::PointerButton`] with
+    /// [`ElementState::Released`] event is emitted when the system has canceled tracking this
+    /// touch, such as when the window loses focus, or on mobile devices if the user moves the
+    /// device against their face.


This behaviour of getting Pressed without Released when a touch phase is cancelled, is IMO quite subtle. Perhaps we should rather expose PointerCancel?

We have attempted to resolve this issue in length the other meeting, the outcome was not satisfactory, we should bring it up next meeting as well maybe together we can come up with a better solution.

In the previous proposal we had indeed TouchCancel, because it only applies to touch.
It might be a good idea indeed to implement PointerCancel and actually use it for other pointer types apart from touch?

This is definitely a weakness of this proposal, if we can't address it in this PR I will open an issue for it.

Apologies, summarizing the context:

The reason why we couldn't come up with a good idea/didn't like TouchCancel is because it actually goes against the idea of the unified pointer model, where users don't need special handling to correctly handle any pointer type.

So folding it into PointerLeft without PointerButton with Released would make this "natural".
However for users who actually want to handle touch events explicitly, its harder to make out TouchCancel.

So a PointerCancel event makes much more sense, because it solves this issue.
But I don't know how we can use it for anything else apart from touch.
However, that said, we can just use it for touch only, as long as users handle it regardless of pointer type, it should fulfill the original purpose.

I like it so far.

On another thought, this is also how Web works, another reason to switch to this new event.

I'm fine with leaving this to be figured out later

PointerLeave has nothing to do with focus.
While it is true that if pointer focus is in play you won't get a PointerLeave until you release the pointer focus, this doesn't mean that any other scenario is not relevant.

PointerLeave means the pointer has left the surface, with the exception of pointer focus, in which case you will get PointerLeave upon loosing pointer focus if the pointer is not inside the surface.
PointerCancel means that input should be discarded.

While in the case of touch input, PointerLeave and PointerCancel are the same thing, as @madsmtm pointed out, the problem is that it is currently hard to figure out when that actually happens because it depends on not receiving a PointerButton with Released.
While it is possible that users just assume that PointerLeave means both and so they can handle it the same, it is not the same as pointed out above. Users who need to distinguish between these two scenarios will have a harder time doing so without a dedicated event.

I think the discussion should rather be around if this differentiation is actually needed, personally I don't know about that. On the other hand W3C deemed it necessary, so maybe digging there could yield something useful.

PointerLeave has nothing to do with focus.

pointer focus, sorry.

PointerLeave means the pointer has left the surface, with the exception of pointer focus, in which case you will get PointerLeave upon loosing pointer focus if the pointer is not inside the surface.
PointerCancel means that input should be discarded.

This is not the case for grabs, if you want to do it that way, then you'd need to update all the implementations to account for that, which I don't think worth it, since you generally want a pointer focus, which what it models right now, at least on X11/Wayland.

While it is possible that users just assume that PointerLeave means both and so they can handle it the same, it is not the same as pointed out above. Users who need to distinguish between these two scenarios will have a harder time doing so without a dedicated event.

I'll just say that you can get leave on regular mouse devices without releasing the button meaning that you should cancel out everything.

In general, I don't mind having a cancel since I have a case on wayland where you can remove the pointer all together meaning that you likely have to cancel everything/same for keyboard/touch/etc.

This is not the case for grabs, if you want to do it that way, then you'd need to update all the implementations to account for that, which I don't think worth it, since you generally want a pointer focus, which what it models right now, at least on X11/Wayland.

The way I described is how it works currently for Web and X11.
I assume this is how it works for every other backend as well already.
If not we should figure out how it works currently and how we want it to work!

What do you mean with "grab"? Don't you mean "pointer focus" with that?

I mean that if you press the button and move the pointer out of the window, you won't get the leave until you release the button, the coordinates client will be getting will be outside of the window, but the pointer is still focusing the window, because you're holding the button.

So if you treat leave as left the surface area it means that you should detect when the mouse is not over the window and sent event that it left, but I'd say that all backends prevent pointer leave until you release everything...

Cool, we agree then on what PointerLeave means, seems this was a misunderstanding then!

src/event.rs

src/platform_impl/apple/uikit/view.rs

madsmtm

macOS and iOS impls look good now, thanks!

src/platform_impl/apple/appkit/view.rs

src/event.rs

src/platform_impl/android/mod.rs

MarijnS95 · 2024-08-21T20:02:07Z

src/platform_impl/android/mod.rs

+                                    device_id,
+                                    state: event::ElementState::Pressed,
+                                    position,
+                                    button: event::ButtonSource::Touch { finger_id, force },


I guess we should differentiate the source here as well? Android also supports pens and mouse input, and getting at least mouse input represented accurately seems like a sensible thing to do in this PR?

Mouse handling needs a bit more work, so I left it to a follow-up PR.
But I implemented differentiating the different tool types.
Entirely skipping the mouse tool type here shouldn't be a regression because before this PR it would show up as touch, which would have been wrong anyway.

Let me know what you think!

kchibisov

What I've commented on Wayland should apply to the rest, but in general it looks ok, I'd say.

kchibisov · 2024-08-23T11:39:12Z

src/event.rs

+    /// A [`WindowEvent::PointerLeft`] without a [`WindowEvent::PointerButton`] with
+    /// [`ElementState::Released`] event is emitted when the system has canceled tracking this
+    /// touch, such as when the window loses focus, or on mobile devices if the user moves the
+    /// device against their face.


I mean, your PointerCancel is PointerLeave because it means that you effectively lost the focus and thus shouldn't do anything in reaction to pointer input, it's the same for mouse/tablet and touch cancel being here makes sense, at least to me, since I was the one suggesting to do it that way.

Maybe it's not clear what PointerLeave actually does? Since what it says that you lost focus it's not like you left the surface since it won't be emitted in case you have on-going grabs, so it's effectively a cancel of the events, just with position attached.

If we add a generic PointerCancel it won't be any different than PointerLeave because that's what it does.

src/event.rs

src/platform_impl/linux/wayland/seat/touch/mod.rs

# 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.

kchibisov

I've rebased the PR, however I decided to not go for the PointerEntered/PointerLeft handling that I was suggesting. The main reason for that is that:

When you press a click with a pointer from e.g. unfocused window state, you generally have a sequence of Enter -> Button, and in generally, if you do button input, you have a preceding event of moving it in place. So having only a single Enter/Leave will
make button jump without any other event, which is a bit more inconsistent in my opinion that having more than one PointerEntered.

Fairly, there's nothing more with multiple PointerEntered if you account for multi-seat and similar devices.

--

I've rebased and squashed @daxpedda 's commits and added my small doc patch/changelog patch at the end.

@daxpedda could you look into what I've added, so if it's fine I'll merge this PR, since I have no desire to rebase it further.

daxpedda

Just a small nit, otherwise LGTM.
Thank you @kchibisov for picking this up!

src/event.rs

- Rename `CursorMoved` to `PointerMoved`. - Rename `CursorEntered` to `PointerEntered`. - Rename `CursorLeft` to `PointerLeft`. - Rename `MouseInput` to `PointerButton`. - Add `position` to every `PointerEvent`. - Remove `Touch`, which is folded into the `Pointer*` events. - New `PointerType` added to `PointerEntered` and `PointerLeft`, signifying which pointer type is the source of this event. - New `PointerSource` added to `PointerMoved`, similar to `PointerType` but holding additional data. - New `ButtonSource` added to `PointerButton`, similar to `PointerType` but holding pointer type specific buttons. Use `ButtonSource::mouse_button()` to easily normalize any pointer button type to a generic mouse button. - In the same spirit rename `DeviceEvent::MouseMotion` to `PointerMotion`. - Remove `Force::Calibrated::altitude_angle`. Fixes rust-windowing#3833. Fixes rust-windowing#883. Fixes rust-windowing#336. Co-authored-by: Kirill Chibisov <contact@kchibisov.com>

daxpedda added S - enhancement Wouldn't this be the coolest? S - api Design and usability labels Aug 16, 2024

daxpedda requested review from jackpot51, notgull, kchibisov, madsmtm and MarijnS95 as code owners August 16, 2024 22:19

daxpedda force-pushed the pointer-overhaul branch from 625265c to fafa90d Compare August 16, 2024 22:27

daxpedda commented Aug 16, 2024

View reviewed changes

src/platform_impl/apple/appkit/view.rs Outdated Show resolved Hide resolved

madsmtm requested changes Aug 18, 2024

View reviewed changes

madsmtm reviewed Aug 18, 2024

View reviewed changes

src/event.rs Show resolved Hide resolved

madsmtm reviewed Aug 18, 2024

View reviewed changes

src/platform_impl/apple/uikit/view.rs Show resolved Hide resolved

daxpedda added the C - nominated Nominated for discussion in the next meeting label Aug 18, 2024

daxpedda force-pushed the pointer-overhaul branch from fafa90d to 0d359e4 Compare August 18, 2024 23:16

daxpedda requested a review from madsmtm August 18, 2024 23:18

daxpedda force-pushed the pointer-overhaul branch from 0d359e4 to d7e7e31 Compare August 18, 2024 23:22

madsmtm approved these changes Aug 19, 2024

View reviewed changes

src/platform_impl/apple/appkit/view.rs Outdated Show resolved Hide resolved

src/platform_impl/apple/appkit/view.rs Outdated Show resolved Hide resolved

src/event.rs Show resolved Hide resolved

MarijnS95 reviewed Aug 21, 2024

View reviewed changes

NthTensor mentioned this pull request Aug 22, 2024

Picking event ordering bevyengine/bevy#14862

Merged

daxpedda requested a review from MarijnS95 August 23, 2024 08:56

daxpedda force-pushed the pointer-overhaul branch from aa3509e to 9ecf564 Compare August 23, 2024 09:01

kchibisov requested changes Aug 23, 2024

View reviewed changes

madsmtm mentioned this pull request Aug 26, 2024

Coordinate systems #3891

Open

kchibisov force-pushed the pointer-overhaul branch from 9ecf564 to b1ebafa Compare October 3, 2024 23:12

This was referenced Oct 4, 2024

Pointer API Sketch #3001

Closed

Experiment with an improved event design #1374

Closed

kchibisov force-pushed the pointer-overhaul branch from 5551169 to 4e9719c Compare October 4, 2024 00:42

kchibisov approved these changes Oct 4, 2024

View reviewed changes

kchibisov force-pushed the pointer-overhaul branch from 4e9719c to e02b043 Compare October 4, 2024 00:57

daxpedda commented Oct 8, 2024

View reviewed changes

src/event.rs Outdated Show resolved Hide resolved

kchibisov removed the C - nominated Nominated for discussion in the next meeting label Oct 8, 2024

kchibisov force-pushed the pointer-overhaul branch from 41e939a to b2a61a6 Compare October 8, 2024 12:09

kchibisov merged commit eccd9e4 into rust-windowing:master Oct 8, 2024
58 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement the pointer event overhaul #3876

Implement the pointer event overhaul #3876

daxpedda commented Aug 16, 2024 •

edited

Loading

madsmtm left a comment

madsmtm Aug 18, 2024

daxpedda Aug 18, 2024

daxpedda Aug 18, 2024 •

edited

Loading

daxpedda Aug 18, 2024

madsmtm Aug 19, 2024 •

edited

Loading

daxpedda Aug 23, 2024

kchibisov Aug 23, 2024

daxpedda Aug 23, 2024

kchibisov Aug 23, 2024

daxpedda Aug 23, 2024

madsmtm left a comment

MarijnS95 Aug 21, 2024

daxpedda Aug 23, 2024

kchibisov left a comment

kchibisov Aug 23, 2024

kchibisov left a comment

daxpedda left a comment

Implement the pointer event overhaul #3876

Implement the pointer event overhaul #3876

Conversation

daxpedda commented Aug 16, 2024 • edited Loading

Description

Summary of the changes

Follow-ups

madsmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daxpedda Aug 18, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsmtm Aug 19, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

madsmtm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov left a comment

Choose a reason for hiding this comment

daxpedda left a comment

Choose a reason for hiding this comment

daxpedda commented Aug 16, 2024 •

edited

Loading

daxpedda Aug 18, 2024 •

edited

Loading

madsmtm Aug 19, 2024 •

edited

Loading