Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

add pointer location hint #1551

Merged
merged 5 commits into from
Sep 27, 2024

Conversation

sodiboo
Copy link
Contributor

@sodiboo sodiboo commented Sep 26, 2024

I noticed that at the moment, there seems to be absolutely no way to utilize the value of zwp_locked_pointer_v1::set_cursor_position_hint, not least because there is no good way to be notified of when its state changes, but Smithay's pointer handling seems to have no way to actually inform it about a cursor position hint.

This PR fixes that, by adding a method to PointerConstraintsHandler to notify the downstream compositor, and a method to PointerHandle to update the location field. Because it keeps track of the old focus as a separate field, i reckon this isn't going to cause any issues, and if it does cause issues, it's entirely opt-in.

PointerConstrainsHandler::cursor_position_hint receives surface-local coordinates. PointerHandle::set_location_hint expects global coordinates. It is up to the compositor to correctly map the surface-local coordinates to global coordinates.

I've already implemented this in niri. That pull request contains a video comparison of a real client (which also happens to be niri) and a concrete benefit to having the pointer location hints in Smithay.

Some care had to be taken when calling PointerConstraintsHandler::cursor_position_hint to make sure we don't deadlock. I may or may not have caused deadlocks several times when implementing this, where the only remedy was a hard reset of my hardware. Oops.

Copy link
Member

@PolyMeilex PolyMeilex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds like a good idea in general.

@sodiboo sodiboo force-pushed the relative-pointer-position-hint branch from 62cd8a9 to 111f676 Compare September 26, 2024 23:42
@sodiboo
Copy link
Contributor Author

sodiboo commented Sep 26, 2024

note: anvil build failing the CI lol. i didn't test with anvil, only niri. i'll implement it, just taking a while 'cause dealing with repos that have no flake.nix is ANnoying™ on my distro, and i need to jump through a bunch more hoops to make anvil build at all on my system

@sodiboo
Copy link
Contributor Author

sodiboo commented Sep 27, 2024

honestly i couldn't get anvil working properly. i got it to start, and got a terminal within, but my niri wayland backend (to test the cursor constraints) doesn't seem to show up at all. However, the code is fairly simple so i've implemented what i think should be necessary. ⚠️ It is, however, untested ⚠️

@YaLTeR
Copy link
Contributor

YaLTeR commented Sep 27, 2024

I'm fairly sure that Blender uses the pointer constraints location hint whenever you do a middle mouse or other drag.

Copy link
Member

@Drakulix Drakulix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems good to me.

@PolyMeilex Any additional remarks?

Copy link
Member

@PolyMeilex PolyMeilex left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good 👍

@PolyMeilex PolyMeilex merged commit debffed into Smithay:master Sep 27, 2024
13 checks passed
@YaLTeR
Copy link
Contributor

YaLTeR commented Sep 30, 2024

Sorry for chiming in late, but what happens if the window is partially off-screen, and the location hint ends up off-screen? I guess it should be clipped to output bounds (this is compositor-specific, e.g. in niri you'd clip to the current output only, whereas in anvil you'd clip to all outputs), then if the pointer position ended up different from the location hint, emit the motion event? Alternatively, if the location hint is off-screen just don't follow it? Not sure what's the best approach here.

@sodiboo
Copy link
Contributor Author

sodiboo commented Sep 30, 2024

Sorry for chiming in late, but what happens if the window is partially off-screen, and the location hint ends up off-screen? I guess it should be clipped to output bounds (this is compositor-specific, e.g. in niri you'd clip to the current output only, whereas in anvil you'd clip to all outputs)

Here are my ramblings about what we should do for "weirdly behaving clients" that send out of bounds cursor position hints. That wasn't your question, but it's what I thought the question was, until the very end of this segment.

Yeah. That'll be compositor-specific, how it clips the pointer position. Most likely, it should be clipped to the window bounds? Since it doesn't really make sense protocol-wise (it's supposed to be for if the client is rendering its own cursor; so where should the compositor cursor "resume from" once the lock is released? how can a client render the cursor out of its own surface bounds?)

The way I implemented it in anvil here and also in niri just doesn't clip it. This is maybe unexpected, at the very least because it doesn't clip to output bounds which it probably should? I simply didn't think of that. I know in niri, the cursor will snap in bounds the next time we get a motion event, so it's technically probably fine, but I have no idea what anvil does here. Sorry if I broke anvil (but I did warn it was untested). Besides the output clipping issues, blindly trusting the position hint as both my initial implementations do can lead to a bad client (a "malicious" one, even) placing the cursor at arbitrary coordinates on screen. Because of how niri handles pointer locks, this immediately deactivates it and further position hints are ignored. So it can't sustain a motion, it can only teleport the cursor once relative to its own surface origin.

One practical application I see of this behaviour is that when the pointer should leave the surface (e.g. a client that "feels like" it uses absolute pointer positioning), the client can account for the last relative motion event it received and appropriately move the cursor whatever leftover pixels out of bounds, for a slightly smoother experience. In practice, this makes absolutely no difference. My Wayland backend for niri clips to its own borders and manually releases the lock if the cursor is on the outer pixels, and this feels flawless despite technically discarding that single relative pointer event in between.

then if the pointer position ended up different from the location hint, emit the motion event?

No. A motion event is not a valid response to a position hint. While a cursor lock is held, we can only send relative motion events. The cursor position hint is just a hint, the compositor can do whatever. The purpose is to inform the compositor where the cursor should be once the pointer lock is deactivated, to prevent the cursor from seemingly teleporting (see videos in the niri PR for how this looks visually). The compositor shouldn't respond to this request in any way ever with how it chose to interpret the position.

Alternatively, if the location hint is off-screen just don't follow it? Not sure what's the best approach here.

I think this is a very reasonable response for "weird" location hints (out of monitor bounds especially). It's just a hint; the compositor isn't required to do anything in response. We can just ignore them. A well behaved client has no good reason to send position hints far enough outside its own surface that it passes a monitor boundary. but what if the surface is on a monitor boundary? oh wait that's what you were asking about initially lol

hey so I totally misread the question and answered a different one. yeah so in this case I think for niri at least it makes most sense to clip to monitor bounds; we should NOT send a motion event but still try to keep the Y axis aligned. Ultimately, the position hint is off screen because the client is genuinely rendering a cursor that is off screen. There's no getting around the fact that the cuesor is genuinely off-screen, and clipping its location is actually just wrong, always, even if convenient. The "best" a compositor generally could do is actually remembering the position hint in surface-local coords, so when the window is scrolled back on screen we can place the cursor in the genuinely correct semantic position, without that client needing to send another position hint.

Or we could, y'know, yeah ignore it. The most important part is that the cursor stays on the surface when the lock is held. I don't think a user would know what to expect from this edge case; like it's just Weird. Pretty much anything we could do would be somewhat unexpected. Ignoring is probably fine. But, you know what? Even if it's not what a user might intuitively expect, I think I know exactly what is obviously the right choice for niri. It's a scrolling compositor! Duh! If the position hint is off-screen, we just scroll that part of the window on-screen. If the window is wider than the screen allows, it'd be especially cool to automatically center the cursor so it can "smoothly" pan over a very wide surface. Wait, is that panning ever practical? Probably not. At least, we should try to make sure the position hint is always within the struts of the layout.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants