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!

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

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.

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

src/event.rs

+        finger_id: FingerId,
+        force: Option<Force>,
+    },
+    Unknown(u16),


What this Unknown is reserved for and why it's limited to u16?

It means its an unknown pointer type, the u16 is the button ID, ergo the same as in MouseButton::Other.

kchibisov · 2024-08-23T11:41:01Z

src/event.rs

@@ -1071,19 +1167,41 @@ mod tests {
                with_window_event(HoveredFile("x.txt".into()));
                with_window_event(HoveredFileCancelled);
                with_window_event(Ime(Enabled));
-                with_window_event(CursorMoved { device_id: did, position: (0, 0).into() });
+                with_window_event(PointerMoved {
+                    device_id: did,


I start to wonder whether device ID should belong here in general, since what kind of device id Touch input has? Maybe it's a property of a specific input device, like Mouse(DeviceId)?

I know at least one use-case: if your drawing tablet has touch support and your screen as well, these would be two different devices.

kchibisov · 2024-08-23T11:44:13Z

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

        self.events_sink.push_window_event(
-            WindowEvent::Touch(Touch {
-                device_id: crate::event::DeviceId(crate::platform_impl::DeviceId::Wayland(
-                    DeviceId,
-                )),
-                phase: TouchPhase::Started,
-                ___location: ___location.to_physical(scale_factor),
-                force: None,
-                finger_id: crate::event::FingerId(crate::platform_impl::FingerId::Wayland(
-                    FingerId(id),
-                )),
-            }),
+            WindowEvent::PointerEntered {
+                device_id,
+                position,
+                kind: PointerKind::Touch(finger_id),
+            },
+            window_id,
+        );


Weren't we discussing that this should be emitted only for the first any finger touching the surface, so it'll look more like a pointer focus stuff?

Don't remember 😰.

I'm not sure what the right move is here, but I would argue that this makes more sense because it is simply more accurate. Unsure how not emitting PointerEntered would make it look more like pointer focus.

Currently I'm in favor of leaving it as-is because its accurate and I'm not sure what the advantage would be of changing it.

In the current state of things there's no need for PointerButton for touch at all, since you can not touch without enter/leave meaning that enter/leave already model the touch down/up.

It's just events are redundant, since they end up doing the exact same thing, indicating touch down/up...

From what I can tell, maybe on some platforms you want to react when you get pointer focus/leave focus, but with touch you'll run this state change for each finger, which doesn't look right. Imagine decorations lightening, etc.

In the current state of things there's no need for PointerButton for touch at all, since you can not touch without enter/leave meaning that enter/leave already model the touch down/up.

It's just events are redundant, since they end up doing the exact same thing, indicating touch down/up...

Hm ... agreed!
Well, lets hash it out at the meeting today.

From what I can tell, maybe on some platforms you want to react when you get pointer focus/leave focus, but with touch you'll run this state change for each finger, which doesn't look right. Imagine decorations lightening, etc.

Please elaborate.
Why would it be wrong to do "decorations lightening" on the second or third finger?
Wouldn't that also mean that you shouldn't do "decorations lightening" on PointerButton?

You react to PointerEnter not PointerButton and for mouse you can not get mulitple. With touch you already have decorations lightened from the first touch input, the rest you should just ignore since user already interacting with the window, since you haven't got any leave events.

Though, generally applications have it delivered via separate event these days.

You react to PointerEnter not PointerButton and for mouse you can not get mulitple.

The point is that users should specifically not react to PointerButton in this context.
This is questionable because its not obvious to a user why they should not.

With touch you already have decorations lightened from the first touch input, the rest you should just ignore since user already interacting with the window, since you haven't got any leave events.

It might help if you explain what you mean with "decorations" here.
But the logic you are talking about here seems to build upon some rules that don't seem obvious to me, I don't know why users should react to this but not to that.

Maybe this is also something we should hash out in the meeting so you can explain this whole matter holistically.

kchibisov · 2024-08-23T11:44:48Z

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

+        self.events_sink.push_window_event(
+            WindowEvent::PointerLeft {
+                device_id,
+                position: Some(position),
+                kind: PointerKind::Touch(finger_id),
+            },


The same as enter but for the last to leave finger.

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

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 added 7 commits August 23, 2024 11:00

Address review

8e9b1bc

Rename PointerType to PointerKind

116c265

Remove unnecessary checks in MacOS

b6d956c

Document ButtonSource::mouse_button()

2a62376

Remove TouchPhase from Android touch handling

b07d223

Handle tool type on Android

9ecf564

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

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 Aug 23, 2024

daxpedda Aug 23, 2024

kchibisov Aug 23, 2024

daxpedda Aug 23, 2024

kchibisov Aug 23, 2024

daxpedda Aug 23, 2024

kchibisov Aug 23, 2024

daxpedda Aug 23, 2024

kchibisov Aug 23, 2024 •

edited

Loading

daxpedda Aug 23, 2024

kchibisov Aug 23, 2024

Implement the pointer event overhaul #3876

Are you sure you want to change the base?

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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

kchibisov Aug 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this 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

kchibisov Aug 23, 2024 •

edited

Loading