rust-windowing / winit

Window handling library in pure Rust
https://docs.rs/winit/
Apache License 2.0
4.89k stars 914 forks source link

Mouse Motion incorrect when using Remote Desktop #2884

Open joverwey opened 1 year ago

joverwey commented 1 year ago

winit Version 0.28.6

Here is a small app that just prints the Mouse Motion. There is a large discrepancy between running this app locally or on a remote machine via remote desktop. See output below:

When using remote desktop, these values for even the smallest movement is orders of magnitude larger.

Here is the test program:

use winit::{
    event::{DeviceEvent, Event, WindowEvent},
    event_loop::{ControlFlow, EventLoop},
    window::WindowBuilder,
};

fn main() {
    let event_loop = EventLoop::new();
    let _window = WindowBuilder::new().build(&event_loop).unwrap();

    event_loop.run(move |event, _, control_flow| {
        *control_flow = ControlFlow::Wait;

        match event {
            Event::DeviceEvent { event, .. } => match event {
                DeviceEvent::MouseMotion { delta } => {
                    println!("Mouse moved by {:?}", delta);
                }
                _ => (),
            },
            Event::WindowEvent { event, .. } => match event {
                WindowEvent::CloseRequested => *control_flow = ControlFlow::Exit,
                _ => (),
            },
            _ => (),
        }
    });
}

Output locally:

Mouse moved by (-1.0, 0.0)
Mouse moved by (-1.0, 0.0)
Mouse moved by (-1.0, -1.0)
Mouse moved by (-1.0, 0.0)
Mouse moved by (-1.0, 0.0)
Mouse moved by (0.0, -1.0)
Mouse moved by (-1.0, 0.0)
Mouse moved by (-1.0, -1.0)

Output when running on remote machine via Remote Desktop:

Mouse moved by (14150.0, 8476.0)
Mouse moved by (14163.0, 8453.0)
Mouse moved by (14163.0, 8430.0)
Mouse moved by (14163.0, 8430.0)
Mouse moved by (14163.0, 8430.0)
Mouse moved by (14163.0, 8408.0)
Mouse moved by (14163.0, 8408.0)
Mouse moved by (14163.0, 8385.0)
Mouse moved by (14163.0, 8385.0)
Mouse moved by (14176.0, 8385.0)
Mouse moved by (14176.0, 8385.0)
Mouse moved by (14176.0, 8385.0)

For context I am working on a 3D game using the Bevy Engine, which uses the winit crate. Camera movement is completely broken when using the app over remote desktop, making it unusable. The root cause is incorrect mouse delta values.

I am using Remote Desktop from a Mac M1, remoting into a Windows 11 machine.

rib commented 1 year ago

Not sure if this might be a handy reference for checking what we do in the Windows backend atm: http://www.petergiuntoli.com/parsing-wm_input-over-remote-desktop

rib commented 1 year ago

This line at least looks like it's probably wrong: https://github.com/rust-windowing/winit/blob/47488909353bb6d8e52801b8824b0e2352e4ab63/src/platform_impl/windows/event_loop.rs#L2393

MOUSE_MOVE_RELATIVE isn't a flag that can be masked, it equals 0 so considering util::has_flag is a utility for bitset & flag == flag it looks like Winit will unconditionally consider events to be relative here.

rib commented 1 year ago

I guess we want something like this:

                    // WARNING: Don't try and test `MOUSE_MOVE_RELATIVE` as if it's a flag
                    // because it equals zero and `has_flag` would unconditionally
                    // return `true`.
                    if util::has_flag(mouse.usFlags as u32, MOUSE_MOVE_ABSOLUTE) {
                        let is_virtual_desktop = util::has_flag(mouse.usFlags, MOUSE_VIRTUAL_DESKTOP as u16);
                        let screen_xy = if is_virtual_desktop { (SM_CXVIRTUALSCREEN, SM_CYVIRTUALSCREEN) } else { (SM_CXSCREEN, SM_CYSCREEN) };
                        let (width, height) = unsafe {
                            let width = GetSystemMetrics(screen_xy.0) as f64;
                            let height = GetSystemMetrics(screen_xy.1) as f64;
                            (width, height)
                        };
                        let x = mouse.lLastX as f64 / 65535.0f64 * width;
                        let y = mouse.lLastY as f64 / 65535.0f64 * height;

                        let delta = if let Some((last_x, last_y)) = userdata.last_mouse_pos {
                            (x - last_x, y - last_y)
                        } else {
                            (0.0f64, 0.0f64)
                        };
                        userdata.last_mouse_pos = Some((x, y));

                        if delta.0 != 0.0 {
                            userdata.send_event(Event::DeviceEvent {
                                device_id,
                                event: Motion { axis: 0, value: delta.0 },
                            });
                        }

                        if delta.1 != 0.0 {
                            userdata.send_event(Event::DeviceEvent {
                                device_id,
                                event: Motion { axis: 1, value: delta.1 },
                            });
                        }
                        if delta.0 != 0.0 || delta.1 != 0.0 {
                            userdata.send_event(Event::DeviceEvent {
                                device_id,
                                event: MouseMotion { delta },
                            });
                        }
                    } else {

but it's a bit fiddly adding mutable state to userdata so leaving that for now

dbartussek commented 6 months ago

Has there been any progress on this in the meantime? This issue affects me all the time because I like to develop and play games remotely on my PC from my Laptop or Steam Deck. Veloren for instance is completely unplayable.

I've made a fork with essentially the fix above to develop my own stuff with, but I'd really like to see this fixed upstream. Is there a better solution than adding a new Cell to userdata? I'd be up for implementing it if you have any.

rib commented 6 months ago

I haven't had a chance to look at improving this to upstream but we're using this patch here currently: https://github.com/EmbarkStudios/winit/commit/9f47e7bad9bd98b8a12f8e920a9f650b63ad7c6a which is probably equivalent to what you have.

One thing I was guessing would be good to change is to have the last position be associated with the device ID, instead of being global.

dbartussek commented 6 months ago

So I've tested around a bit and at least when trying it with Parsec and TeamViewer for remote input as well as an XP Pen painting tablet, DeviceId is always zero.

The windows documentation explicitly says that hDevice, which the DeviceId is based on, may be zero "if an input is received from a precision touchpad": RAWINPUTHEADER structure remarks

All in all, I don't think it is possible in most cases to handle this perfectly on Windows. Devices that generate MOUSE_MOVE_ABSOLUTE native events apparently don't have an ID, so we can't differentiate where the event came from.

dbartussek commented 6 months ago

I've done some quick and dirty experiments with other games and haven't found any that don't get very confused when multiple sources send absolute mouse inputs. I'd be happy to still write an implementation using a RefCell containing a Map to handle multiple devices just in case, but as far as I can tell, there really is no way of telling different sources apart, so I believe the best solution may be to just accept this and document it as a quirk of Windows.

rib commented 6 months ago

Interesting, thanks for experimenting to check the state of the device ID - yeah I had only figured that it intuitively seemed like it could make sense to check the device Id, but if it's not actually useful in practice then it sounds OK to me to stick with a global 'last_position' state.

Maybe I can just open a PR with my patch above - with a comment as you say, noting that the device ID isn't useful here.

dbartussek commented 6 months ago

I've also gone and looked at how SDL handles this: Their implementation first checks if a remote desktop is attached or not and in either case just straight up discards large position jumps: https://github.com/libsdl-org/SDL/blob/main/src/video/windows/SDL_windowsevents.c#L498

All in all, a PR with your patch and a comment is probably the best we can do here unless we also want to introduce such metrics. It's a lot prettier than my hacked together code.

jumerckx commented 4 months ago

I believe this is a duplicate of https://github.com/rust-windowing/winit/issues/946. I'm also running into this issue on WSL. Does anyone have a workaround?