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

sway 1.5 sends spurious pointer motion events to mpv #5594

Closed
Dudemanguy opened this issue Aug 1, 2020 · 5 comments
Closed

sway 1.5 sends spurious pointer motion events to mpv #5594

Dudemanguy opened this issue Aug 1, 2020 · 5 comments
Labels
bug Not working as intended input/pointer

Comments

@Dudemanguy
Copy link
Contributor

Dudemanguy commented Aug 1, 2020

Description:

Sorry I only noticed just now (shows you how much I pay attention to the cursor). I think sway is sending spurious pointer motion events which confuses mpv (and maybe some other applications). I bisected the change to specifically to this commit: 6ea4539.

Debug Logs:

Log with 6ea4539 applied
Log with 6ea4539 reverted

Reproduction

Play a video with mpv (non-fullscreen), make sure that your mouse cursor is in the mpv window when it launches, wait for the cursor to autohide, then go to fullscreen.

Expected Result

The cursor stays hidden.

Actual Result.

The cursor appears on the screen.

I'm not sure exactly why this is happening, but that commit causes sway to send a pointer motion event when mpv goes in/out of fullscreen (regardless of the mouse moving or not; which causes the cursor to appear). From the wayland spec:

<description summary="pointer motion event">
  Notification of pointer location change. The arguments
  surface_x and surface_y are the location relative to the
  focused surface.
</description>

Since the pointer's location isn't changing, I wouldn't expect to receive this event here so I don't think the client should add extra code to handle this case (a motion event with no change in position).

@Dudemanguy Dudemanguy added the bug Not working as intended label Aug 1, 2020
@Dudemanguy Dudemanguy changed the title mpv cursor behavior is different with sway 1.5 sway 1.5 sends spurious pointer motion events to mpv Aug 2, 2020
@Dudemanguy
Copy link
Contributor Author

Updated the issue to hopefully be more clear on what's going on here.

@Xyene
Copy link
Member

Xyene commented Aug 2, 2020

Well, 6ea4539 has two parts, one where it rebases the cursor when its constraint changes, and one where it sends a motion event post-rebase. The first part isn't the problem, since I can't see any constraints being created in your log.

That leaves the second part. wlroots has code to not send duplicate motion events, but the check is performed in surface-local coordinates. When the mpv surface is resized, we (correctly) rebase the cursor, but the surface-local coordinates of the cursor have changed (the surface has gotten bigger), so we send a motion event even though the cursor hasn't technically moved.

I'm not sure this is a bug though, and it sounds like the correct behavior to me. Consider an application that hides the cursor and draws its own (e.g. a game), and a Sway where we don't rebase the cursor. As a concrete example, in the game below the real cursor is hidden and a software cursor is drawn in the bottom right corner where the last motion event received was:

image

On making the game fullscreen, we want the cursor to remain in the same position:

image

If we don't rebase, the game will think the cursor is in the last surface-local location, which doesn't match the actual mouse location:

image

This will cause a jump to the correct location the next time the cursor is moved, which is not ideal.

@Dudemanguy
Copy link
Contributor Author

If I understand the Osu example correctly, if you rebase the cursor but don't send the motion pointer event, the cursor location would still appear to change because surface local coordinates have changed correct?

@Xyene
Copy link
Member

Xyene commented Aug 2, 2020

"Rebasing" just means to "make the cursor consistent with what's underneath it". Rebasing in this case means sending that motion event.

...if you rebase the cursor but don't send the motion pointer event, the cursor location would still appear to change because surface local coordinates have changed correct?

It'll look like my third screenshot, where the real mouse is still in the bottom-right corner, but the cursor is (incorrectly) drawn under the osu logo.

@Dudemanguy
Copy link
Contributor Author

Okay, thanks for the help. Then this should be fixed on our end.

Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue Aug 2, 2020
Sway 1.5 started sending more mouse events to mpv which sort of broke
the autohiding behavior (the cursor would appear again if you
fullscreened). Sway had a good reason to do this because certain
applications had inconsistencies between the hardware cursor and the
software cursor without this[1]. So mpv just needs to take this case
into consideration.

This commit does two things. First, it stores mouse coordinates in the
vo_wayland_state right on the pointer enter event. Probably, this should
have been done regardless of the new sway behavior or not. Secondly when
a pointer motion event is received, the mouse position is only set if
the coordinates have changed from the old value. This ensures that we
aren't setting the position if the surface merely changes without the
mouse actually moving (like when you fullscreen).

[1] swaywm/sway#5594
Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue Aug 2, 2020
Sway 1.5 started sending more mouse events to mpv which sort of broke
the autohiding behavior (the cursor would appear again if you
fullscreened). Sway had a good reason to do this because certain
applications had inconsistencies between the hardware cursor and the
software cursor without rebasing the cursor[1]. So mpv just needs to
take this case into consideration.

This commit does two things. First, it stores mouse coordinates in the
vo_wayland_state right on the pointer enter event. Probably, this should
have been done regardless of the new sway behavior or not. Secondly when
a pointer motion event is received, the mouse position is only set if
the coordinates have changed from the old value. This ensures that we
aren't setting the position if the surface merely changes without the
mouse actually moving (like when you fullscreen).

[1] swaywm/sway#5594
Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue Aug 2, 2020
Sway 1.5 started sending more mouse events to mpv which sort of broke
the autohiding behavior (the cursor would appear again if you
fullscreened). Sway had a good reason to do this because certain
applications had inconsistencies between the hardware cursor and the
software cursor without rebasing the cursor[1]. So mpv just needs to
take this case into consideration.

This commit does two things. First, it stores mouse coordinates in the
vo_wayland_state right on the pointer enter event. Probably, this should
have been done regardless of the new sway behavior or not. Secondly when
a pointer motion event is received, the mouse position is only set if
the coordinates have changed from the old value. This ensures that we
aren't setting the position if the surface changes but there is no mouse
movement (like when you fullscreen).

[1] swaywm/sway#5594
Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue Aug 2, 2020
Sway 1.5 started sending more pointer motion events to mpv which broke
the autohiding behavior. The cursor would appear again if you
fullscreened. Sway had a good reason to do this because certain
applications had inconsistencies between hardware cursor and software
cursor without rebasing on state changes[1]. So mpv needs to take this
special case into consideration.

Initially, simply checking mouse coordinates for changes was considered,
but this doesn't work. All coordinates are surface-local in wayland so
something can appear to move in the local coordinate space but not
globally. You're not allowed to know global mouse coordinates in
wayland, and we don't care about local coordinate changes in mpv so this
approach isn't viable.

Instead, let's just keep track of a local state change. If the toplevel
surface changes in some way (fullscreen, maximized, etc.), then just set
a bool that lets us ignore the mp_input_set_mouse_pos function. This
keeps the cursor from appearing simply because the state was changed
(i.e. fullscreening). For compositors that don't send pointer motion
events on a state change, this does technically mean that the initial
mp_input_set_mouse_pos is never set. In practice, this isn't a
noticeable difference though because moving a mouse generates a ton of
motion events so you'll immediately see it on the second motion event.

[1] swaywm/sway#5594
Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue Aug 2, 2020
Sway 1.5 started sending more pointer motion events to mpv which broke
the autohiding behavior. The cursor would appear again if you
fullscreened. Sway had a good reason to do this because certain
applications had inconsistencies between hardware cursor and software
cursor without rebasing on state changes[1]. So mpv needs to take this
special case into consideration.

Initially, simply checking mouse coordinates for changes was considered,
but this doesn't work. All coordinates are surface-local in wayland so
something can appear to move in the local coordinate space but not
globally. You're not allowed to know global mouse coordinates in
wayland, and we don't care about local coordinate changes in mpv so this
approach isn't viable.

Instead, let's just keep track of a local state change. If the toplevel
surface changes in some way (fullscreen, maximized, etc.), then just set
a bool that lets us ignore the mp_input_set_mouse_pos function. This
keeps the cursor from appearing simply because the state was changed
(i.e. fullscreening). For compositors that don't send pointer motion
events on a state change, this does technically mean that the initial
mp_input_set_mouse_pos is never set. In practice, this isn't a
noticeable difference though because moving a mouse generates a ton of
motion events so you'll immediately see it on the second motion event.

[1] swaywm/sway#5594
cyanreg pushed a commit to cyanreg/mpv that referenced this issue Oct 2, 2020
Sway 1.5 started sending more pointer motion events to mpv which broke
the autohiding behavior. The cursor would appear again if you
fullscreened. Sway had a good reason to do this because certain
applications had inconsistencies between hardware cursor and software
cursor without rebasing on state changes[1]. So mpv needs to take this
special case into consideration.

Initially, simply checking mouse coordinates for changes was considered,
but this doesn't work. All coordinates are surface-local in wayland so
something can appear to move in the local coordinate space but not
globally. You're not allowed to know global mouse coordinates in
wayland, and we don't care about local coordinate changes in mpv so this
approach isn't viable.

Instead, let's just keep track of a local state change. If the toplevel
surface changes in some way (fullscreen, maximized, etc.), then just set
a bool that lets us ignore the mp_input_set_mouse_pos function. This
keeps the cursor from appearing simply because the state was changed
(i.e. fullscreening). For compositors that don't send pointer motion
events on a state change, this does technically mean that the initial
mp_input_set_mouse_pos is never set. In practice, this isn't a
noticeable difference though because moving a mouse generates a ton of
motion events so you'll immediately see it on the second motion event.

[1] swaywm/sway#5594
Dudemanguy added a commit to Dudemanguy/mpv that referenced this issue May 18, 2021
Sway 1.5 started sending more pointer motion events to mpv which broke
the autohiding behavior. The cursor would appear again if you
fullscreened. Sway had a good reason to do this because certain
applications had inconsistencies between hardware cursor and software
cursor without rebasing on state changes[1]. So mpv needs to take this
special case into consideration.

Initially, simply checking mouse coordinates for changes was considered,
but this doesn't work. All coordinates are surface-local in wayland so
something can appear to move in the local coordinate space but not
globally. You're not allowed to know global mouse coordinates in
wayland, and we don't care about local coordinate changes in mpv so this
approach isn't viable.

Instead, let's just keep track of a local state change. If the toplevel
surface changes in some way (fullscreen, maximized, etc.), then just set
a bool that lets us ignore the mp_input_set_mouse_pos function. This
keeps the cursor from appearing simply because the state was changed
(i.e. fullscreening). For compositors that don't send pointer motion
events on a state change, this does technically mean that the initial
mp_input_set_mouse_pos is never set. In practice, this isn't a
noticeable difference though because moving a mouse generates a ton of
motion events so you'll immediately see it on the second motion event.

[1] swaywm/sway#5594
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Not working as intended input/pointer
Development

No branches or pull requests

2 participants