Change `PointerHandle.motion` and friends to take relative coords #1223

colinmarc · 2023-11-10T20:00:25Z

The existing API is helpful in that it makes it harder to mess up the math, but it's not flexible enough to support having different windows with different scaling factors. That's relevant if, for example, you want to force a scale factor of 1 for Xwayland windows.

The existing API takes Option<(<D as SeatHandler>::PointerFocus, Point<i32, Logical>)>, with the point being the surface origin, rather than the relative coordinates of the point on that surface. Then, internally, it converts into relative coordinates to generate the wayland event. A user familiar with wayland but not reading the documentation closely might think that the point should be the relative coords, not the surface origin (this happened to me).

Additionally, if you have different scale factors for different windows, it's impossible (or very difficult) to get the calculation right, since smithay is assuming that the coordinate space for the surface is the same as the global compositor space.

colinmarc · 2023-11-10T20:25:23Z

I can't get anvil to run on my machine for (it appears unrelated) reasons, so I haven't tested that part.

It occurs to me that this will probably result in confusing behavior for existing compositors who update without knowing. I don't really know how to mitigate that.

colinmarc · 2023-11-11T12:28:24Z

Hm, this is kinda lackluster. Taking another stab at this with a newtype to flush out cases I missed.

codecov-commenter · 2023-11-12T15:05:33Z

Codecov Report

Attention: 91 lines in your changes are missing coverage. Please review.

Comparison is base (003ca51) 21.76% compared to head (c624329) 21.78%.
Report is 1 commits behind head on master.

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1223      +/-   ##
==========================================
+ Coverage   21.76%   21.78%   +0.02%     
==========================================
  Files         155      156       +1     
  Lines       24373    24407      +34     
==========================================
+ Hits         5305     5318      +13     
- Misses      19068    19089      +21

Flag	Coverage Δ
wlcs-buffer	`18.76% <0.00%> (-0.03%)`	⬇️
wlcs-core	`18.42% <0.00%> (-0.03%)`	⬇️
wlcs-output	`7.62% <0.00%> (-0.02%)`	⬇️
wlcs-pointer-input	`20.56% <32.08%> (+0.02%)`	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Files	Coverage Δ
src/utils/mod.rs	`0.00% <ø> (ø)`
src/utils/relative_point.rs	`100.00% <100.00%> (ø)`
wlcs_anvil/src/main_loop.rs	`69.79% <100.00%> (ø)`
src/wayland/seat/pointer.rs	`23.59% <0.00%> (ø)`
anvil/src/shell/xdg.rs	`2.82% <0.00%> (ø)`
src/input/pointer/grab.rs	`9.29% <50.00%> (ø)`
anvil/src/shell/grabs.rs	`0.00% <0.00%> (ø)`
src/wayland/selection/data_device/dnd_grab.rs	`0.00% <0.00%> (ø)`
...c/wayland/selection/data_device/server_dnd_grab.rs	`0.00% <0.00%> (ø)`
src/desktop/wayland/popup/grab.rs	`0.00% <0.00%> (ø)`
... and 3 more

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

colinmarc · 2023-11-12T15:07:08Z

Ok, I think this is much better now. I introduced a type, RelativePoint, to add semantic meaning to Option<(<D as SeatHandler>::PointerFocus, Point<i32, Logical>)>, which was a bit of a mouthful.

I find the new API is a bit more intuitive (I got confused that motion wanted the surface origin), but I know it's opinionated, so please tell me if you'd prefer a different approach. One disadvantage is that Window.surface_under still returns Option((surface, surface_origin)), but I added RelativePoint::on_focused_surface(pos, surface, surface_origin) to make it easy to follow the happy path without doing any math yourself.

This has the extra benefit that the calculation pos - surface_origin.to_f64() is only done there, instead of all over the library.

colinmarc · 2023-11-12T15:19:34Z

src/desktop/wayland/popup/grab.rs

-                focus: Some((root.into(), (0, 0).into())),
+                focus: Some(RelativePoint {
+                    surface: root.into(),
+                    loc: (0f64, 0f64).into(),


I wanted to highlight this as something I really wasn't sure about.

Drakulix · 2023-11-13T12:53:32Z

That's relevant if, for example, you want to force a scale factor of 1 for Xwayland windows.

Before giving this a proper review, note that I would love to have proper API in smithay to support overriding scale factors of windows and/or globally for Xwayland.

Drakulix

Couple of nitpicks. I like the general approach, this seems more flexible without introducing too much friction in the api.

Drakulix · 2023-11-13T12:57:33Z

src/utils/relative_point.rs

+/// Represents a point on a surface, relative to that surface's origin in
+/// global compositor space.
+#[derive(Debug, Clone)]
+pub struct RelativePoint<S> {


I am not a huge fan of this name, as it is used to denote the focus of the pointer and the relative location. I don't think specifying the relative aspect in the name is most important, if surface and loc have good documentation.

I am open to some bike shedding over the name, but I am leaning towards something like FocusedPoint or FocusPoint?

That's what I originally had, but I realized I had to move it out of the input::pointer mod because it's used by grab code. Once I moved it into utils, it seemed appropriate to give it more of a general name (Focus being an input-specific concept).

Happy to place/name it whatever you like, just wanted to add that context. My suggestion given your constraints would be SurfaceCoordinate or SurfacePoint or similar.

SurfacePoint

That is also a good one, but given a focused element doesn't have to be a surface, RelativePoint does seem to capture this quite well. 😅

src/utils/relative_point.rs

anvil/src/winit.rs

Drakulix · 2023-11-13T13:06:29Z

src/input/pointer/mod.rs

            let event = MotionEvent {
-                location: event.location - surface_location.to_f64(),
+                location: focus.loc.to_f64(),


So we are not using the MotionEvent location anymore at all? I quite dislike that to be honest, because that means we pass in multiple locations, but ignore some of them. In that case please change the signature of motion to not take a MotionEvent, but directly the serial and time.

event.location is still used to set self.location, which is provided by PointerHandle.current_location, for example.

event.location is still used to set self.location

Right... Maybe we should then at least rename MotionEvent::location to global_location or absolute_location to further differenciate the two values?

Done in e3a9927.

The existing API was helpful in that it made it harder to mess up the math, but not flexible enough to support having different windows with different scaling factors. That's relevent if, for example, you want to force a scale factor of 1 for Xwayland windows.

colinmarc · 2023-11-28T11:24:27Z

Sorry for the delay! Updated to address comments.

colinmarc · 2023-11-28T13:09:04Z

Renaming MotionEvent.location made me realize that this line may no longer be correct:

smithay/src/desktop/wayland/layer.rs

Line 729 in 859ed44

new_event.location -= loc.to_f64();

But I'm not sure.

Drakulix · 2023-11-28T13:49:14Z

Renaming MotionEvent.location made me realize that this line may no longer be correct:

smithay/src/desktop/wayland/layer.rs

Line 729 in 859ed44

new_event.location -= loc.to_f64();

But I'm not sure.

It is correct, the issue is that PointerTarget::enter/motion both get a MotionEvent with relative coordinates, while the PointerHandle and grabs don't. But both take the same MotionEvent-type, which means the last commit made the situation both better and worse, depending on which part of the API you look at.

And I think this the core issue, that makes this api feel so weird. We probably don't want to use the same type here, as this is confusing at best. This fits quite nicely with a discussion we recently had of introducing Local or Relative coordinates as opposed to Global coordinates in addition to the Logical/Physical coordinate spaces. I'll get back to you on that one.

colinmarc · 2024-03-04T19:20:30Z

I took a stab at making MotionEvent generic over relative/global coordinate spaces:

#[derive(Debug)]
pub struct GlobalOrigin;

#[derive(Debug)]
pub struct SurfaceOrigin;

/// Pointer motion event
#[derive(Debug)]
pub struct MotionEvent<Origin> {
    /// Location of the pointer. If `Origin` is `SurfaceOrigin`, the coordinates
    /// are relative to the origin of the surface. If `Origin` is `GlobalOrigin`, it is a
    /// location in global compositor space.
    pub location: Point<f64, Logical>,
    /// Serial of the event
    pub serial: Serial,
    /// Timestamp of the event, with millisecond granularity
    pub time: u32,

    _origin: std::marker::PhantomData<Origin>,
}

The problem is still that lots of functions take an optional focus, but the MotionEvent is not optional:

    /// Notify that the pointer moved
    ///
    /// You provide the new location of the pointer, in the form of:
    ///
    /// - The surface that the cursor is on top of, and
    /// - The coordinates of the cursor relative to the origin of that surface
    ///
    /// A value of None indicates that no surface is focused, and the
    /// coordinates of the MotionEvent will be ignored.
    ///
    /// This will internally take care of notifying the appropriate client
    /// objects of enter/motion/leave events.
    #[instrument(level = "trace", parent = &self.span, skip(self, data, focus))]
    pub fn motion(&self, data: &mut D, focus: Option<D::PointerFocus>, event: &MotionEvent<SurfaceOrigin>) {
        // ...
    }

Here, if focus is None, then the location field of the event is completely ignored. It's the same problem, but subtly different, and even more finicky, because you need to construct a MotionEvent in the backend code of your compositor (anvil example), and it's not clear what should be set for location if it's a relative one and there's no focus.

I see two possibilities:

Ditching MotionEvent altogether, adding serial etc as arguments in their own right, and keeping RelativePoint (from the current version of this PR)
Removing location from MotionEvent but otherwise keeping this PR as-is.

I think I like 2 the best. MotionEvent keeps its semantic meaning with a timestamp and serial, which is to say it's semi-opaque and gets passed through everywhere. But the RelativePoint gets more relative as we go down the tree.

Drakulix · 2024-03-05T11:52:02Z

I see two possibilities:
1. Ditching `MotionEvent` altogether, adding `serial` etc as arguments in their own right, and keeping `RelativePoint` (from the [current version](https://github.com/Smithay/smithay/pull/1223/commits/9b5411ac287e595b18444ffcfa574700a5d5bed6#diff-4d2f1d428c6ceedf69db74c16fda79aa7ce3e8317c08456336a67f67b7ebcd13) of this PR)

2. Removing `location` from `MotionEvent` but otherwise keeping this PR as-is.
I think I like 2 the best. MotionEvent keeps its semantic meaning with a timestamp and serial, which is to say it's semi-opaque and gets passed through everywhere. But the RelativePoint gets more relative as we go down the tree.

I agree, that 2. sounds like a good solution.

ryanabx · 2024-04-30T23:40:17Z

Removing location from MotionEvent but otherwise keeping this PR as-is.

I think I like 2 the best. MotionEvent keeps its semantic meaning with a timestamp and serial, which is to say it's semi-opaque and gets passed through everywhere. But the RelativePoint gets more relative as we go down the tree.

Naively trying to understand this -- If MotionEvent doesn't have location, how do we gather where the pointer moved in fn motion...

colinmarc · 2024-05-01T07:30:50Z

Naively trying to understand this -- If MotionEvent doesn't have location, how do we gather where the pointer moved in fn motion...

The RelativePoint has a location, but it's not in the global compositor space, it's relative to the surface. Turns out some of the APIs (grab intercepting for example) rely on getting the global cursor location out of the PointerHandle, so I got stuck.

chrisduerr · 2024-05-07T06:10:48Z

I don't think RelativePoint contributes much here, especially since every point is relative.

With MotionEvent things are still just as confusing anyway, since what you want is probably position relative to the touch down surface, while the focus will potentially be on a separate surface. So I'd say location should only be in the event and not coupled to the focus.

colinmarc force-pushed the pointer-motion-coords branch 3 times, most recently from 4236e1e to f7fb1c4 Compare November 10, 2023 20:20

colinmarc force-pushed the pointer-motion-coords branch 2 times, most recently from e574574 to 5ceb562 Compare November 12, 2023 14:57

colinmarc changed the title ~~Change PointerHandle.motion to take relative coords~~ Change PointerHandle.motion and friends to take relative coords Nov 12, 2023

colinmarc commented Nov 12, 2023

View reviewed changes

colinmarc force-pushed the pointer-motion-coords branch 2 times, most recently from 0880050 to c624329 Compare November 12, 2023 15:23

Drakulix reviewed Nov 13, 2023

View reviewed changes

colinmarc added 2 commits November 28, 2023 12:14

Support tuple arguments to on_focused_surface

180b08e

colinmarc force-pushed the pointer-motion-coords branch from c624329 to 180b08e Compare November 28, 2023 11:18

Rename MotionEvent.location to global_location

e3a9927

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change `PointerHandle.motion` and friends to take relative coords #1223

Change `PointerHandle.motion` and friends to take relative coords #1223

colinmarc commented Nov 10, 2023 •

edited

colinmarc commented Nov 10, 2023

colinmarc commented Nov 11, 2023

codecov-commenter commented Nov 12, 2023 •

edited

colinmarc commented Nov 12, 2023 •

edited

colinmarc Nov 12, 2023

Drakulix commented Nov 13, 2023

Drakulix left a comment

Drakulix Nov 13, 2023 •

edited

colinmarc Nov 28, 2023

Drakulix Nov 28, 2023

Drakulix Nov 13, 2023

colinmarc Nov 28, 2023

Drakulix Nov 28, 2023

colinmarc Nov 28, 2023

colinmarc commented Nov 28, 2023

colinmarc commented Nov 28, 2023 •

edited

Drakulix commented Nov 28, 2023

colinmarc commented Mar 4, 2024

Drakulix commented Mar 5, 2024 •

edited

ryanabx commented Apr 30, 2024

colinmarc commented May 1, 2024

chrisduerr commented May 7, 2024

Change PointerHandle.motion and friends to take relative coords #1223

Are you sure you want to change the base?

Change PointerHandle.motion and friends to take relative coords #1223

Conversation

colinmarc commented Nov 10, 2023 • edited

colinmarc commented Nov 10, 2023

colinmarc commented Nov 11, 2023

codecov-commenter commented Nov 12, 2023 • edited

Codecov Report

colinmarc commented Nov 12, 2023 • edited

colinmarc Nov 12, 2023

Choose a reason for hiding this comment

Drakulix commented Nov 13, 2023

Drakulix left a comment

Choose a reason for hiding this comment

Drakulix Nov 13, 2023 • edited

Choose a reason for hiding this comment

colinmarc Nov 28, 2023

Choose a reason for hiding this comment

Drakulix Nov 28, 2023

Choose a reason for hiding this comment

Drakulix Nov 13, 2023

Choose a reason for hiding this comment

colinmarc Nov 28, 2023

Choose a reason for hiding this comment

Drakulix Nov 28, 2023

Choose a reason for hiding this comment

colinmarc Nov 28, 2023

Choose a reason for hiding this comment

colinmarc commented Nov 28, 2023

colinmarc commented Nov 28, 2023 • edited

Drakulix commented Nov 28, 2023

colinmarc commented Mar 4, 2024

Drakulix commented Mar 5, 2024 • edited

ryanabx commented Apr 30, 2024

colinmarc commented May 1, 2024

chrisduerr commented May 7, 2024

Change `PointerHandle.motion` and friends to take relative coords #1223

Change `PointerHandle.motion` and friends to take relative coords #1223

colinmarc commented Nov 10, 2023 •

edited

codecov-commenter commented Nov 12, 2023 •

edited

colinmarc commented Nov 12, 2023 •

edited

Drakulix Nov 13, 2023 •

edited

colinmarc commented Nov 28, 2023 •

edited

Drakulix commented Mar 5, 2024 •

edited