Open mtlynch opened 2 years ago
@A2Wolverin3 did a lot of excellent work on an implementation of this:
https://github.com/tiny-pilot/tinypilot/pull/1415
I'd like to iterate on his work and prioritize this feature, but I want to first converge on some architectural questions:
/dev/hidg1
and the relative mouse exists alongside it at /dev/hidg2
/dev/hidg1
with a relative mouse if the user enables relative mouse?RateLimitedMouse
?
RateLimitedMouse
was designed specifically around the assumption that we could drop certain events, which seems not true in the case of a relative mouse. Should we create a separate class for the relative mouse?With regards to storing state... using a relative mouse requires entering pointerlock. No pointerlock -> No relative mouse. So my PR was kind of built around the assumption that the pointerlocked element would be the state for mouse mode. If something else was used to track state, it ultimately would need to stay in sync with the pointerlock element status anyway. There would also be many hoops to jump through to update some other keeper of state since users can't actually manipulate the menus while in pointerlock and the mouse is, um, pointerlocked.
Regarding using one or two mouse modes... the apps web page can't just load up in pointerlock mode on it's own. Some action has to be taken by the user to enter it. Technically this doesn't mean both modes have to be supported in the same server instance. But it does feel like keeping the absolute mouse all the time would be helpful for a 'works-by-default' sort of experience - unless it doesn't, at which point users can toggle over via the menu item. And since action has to be taken to toggle into and out of pointerlock anyway, it doesn't seem like too much trouble to just keep the two interfaces.
(FWIW, my own docker implementation doesn't rely on direct access to /dev/hidg* and instead looks in the gadget config to get device numbers and create usable device nodes in the container according to configfs names. That way there isn't any need to keep track of which devices are enabled and which hidg number they are assigned. That could be an area to explore if you wanted to get more flexible with device configuration.)
On the last point, I just piggybacked on RateLimitedMouse
because it was convenient - especially in a mode-switching paradigm. I did have to add a boolean field to the mouse events being sent around... but otherwise I overlayed everything for relative mouse on top of the existing absolute mouse event fields. Anyway, I don't think it's true that relative mouse events can't be dropped. Conceptually, there is no "current" position for a mouse in pointerlock mode. Just movement events. The mouse won't ever reach the "edge" of the box. It will happily keep generating "move right" events ad infinitum. So it's really not a big deal if some get dropped. I added the code to coalesce events that come quickly to try keep the feel of the mouse in tact. It would be annoying if 9 of every 10 events get dropped because they come too fast... with the result being that the mouse ultimately crawls along at a snail's pace because it only gets a fraction of it's events delivered. But I don't think dropping an event here and there is not the end of the world.
Yeah, I agree that it simplifies state management if pointerlock == relative mouse. But I'm wondering if it would make more sense for it to be something tracked server-side rather than something each client has to declare independently.
I'm imagining that when the server is in relative mouse mode, it renders an overlay on top of the remote screen element that says something like "click to use mouse." And then that captures the pointer. That way, it's a consistent experience for any client accessing TinyPilot rather than everyone having to activate relative mouse from the menu every time. And then the client doesn't have to declare mouse events to the server as relative mode vs. absolute mode because the server would already know.
Anyway, I don't think it's true that relative mouse events can't be dropped. Conceptually, there is no "current" position for a mouse in pointerlock mode. Just movement events.
Yeah, we'd have to think about this more.
For an absolute mouse, the penalty for dropping events is pretty low. If the mouse goes (0, 0) -> (100, 100) -> (150, 125), you can probably drop the (100, 100) event without affecting many people's experiences because the mouse just ended up at (150, 125) anyway. But if the events are relative (+100, +100) -> (+50, +25), the cursor ends up in a different spot if you dropped the (+100, +100), and I'd expect more users to care.
Gotcha on the UI aspect. I'm not a UI dev, so I don't really think about these things. :wink:
But if the events are relative (+100, +100) -> (+50, +25), the cursor ends up in a different spot if you dropped the (+100, +100), and I'd expect more users to care.
Just my two cents... but I don't think that's necessarily true. If you're dropping a lot of events, the experience begins to fall apart. (Thus the total movement accumulation in the rate-limiting code.) But the client-side mouse pointer goes away in pointerlock. So a small number dropped events don't create some sort of client/server pointer mismatch that will drive people nuts. Just a laggier mouse... which is already laggy even without dropped events since only the server-side cursor is shown and there is obviously some latency between client-side mouse event and the eventual rendering of a moved cursor coming across the ustreamer feed.
Btw, in my playing around with this, the pointerlock mouse events came pretty quickly. Movements of 100 pixels at a time don't happen. Usually just 1-9 pixels at a time. On my machine anyway. Large pixel movements would be a function of accumulated events during the rate limit timout period. If you happen to drop one of those large accumulated events somewhere between client and server, I could see the experience being a little glitchy. But overall, I think most people would prefer an absolute mouse anyway if they have a choice. Even without dropped events, it's a nicer experience.
A few (random) notes from my side here, as I was looking into this a bit:
ESC
key for exiting Pointer Lock mode, so during Pointer Lock mode, we cannot forward the ESC
key stroke to the target machine. I’m not sure it’s possible to customize or disable that behaviour.pointerlockchange
event, which seems to reliably report all mode changes (on and off).A user on the forum is interested in relative mouse mode so that they can use their TinyPilot while their target machine is in an extended display mode.
A user on the forum encountered an issue with mouse input to their NVR, there were no errors, so it's likely the issue is with the mouse mode TinyPilot uses. Supporting relative mouse mode will likely improve compatibility with devices that only support relative mouse positioning.
Edit: A potential customer on HelpScout also asked about a use case that would also require relative mouse positioning support.
There are some systems that fail to recognize TinyPilot's mouse because it presents itself as an absolute-positioned mouse (like a touchpad), but they will work with relative mouse (where the mouse only reports deltas).
It's possible for TinyPilot to support relative mouse mode, but we'd have to touch a few places:
HID descriptor (backend)
The Ansible role writes an HID descriptor that's always an absolutely-positioned mouse. We'd have to add a command-line flag like
--mouse-mode=relative
so that these lines would become something like this:Remote screen (frontend)
Use the Pointer Lock API to capture all mouse movement when the user clicks on the remote screen.
mouse.js (frontend)
The
RateLimitedMouse
class assumes an absolute mouse. We'd need to either support a relative mode or abstract away the differences between relative and absolute at the caller end.Mouse event parsing (backend)
All of the backend's mouse event parsing assumes absolute mouse coordinates, so we'd have to adjust to support relative movement.
https://github.com/tiny-pilot/tinypilot/blob/3925bb2adcc2c78611574ea441f96ebf8474dbff/app/request_parsers/mouse_event.py#L37