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

Inconsistent usage of LogicalPosition vs PhysicalPosition #1406

Closed
dhardy opened this issue Jan 18, 2020 · 15 comments · Fixed by #1483
Closed

Inconsistent usage of LogicalPosition vs PhysicalPosition #1406

dhardy opened this issue Jan 18, 2020 · 15 comments · Fixed by #1483
Labels
C - needs discussion Direction must be ironed out S - api Design and usability

Comments

@dhardy
Copy link
Contributor

dhardy commented Jan 18, 2020

I notice that sometime between version 0.20.0-alpha5 and 0.20.0, some parts of the event loop started reporting physical positions instead of logical ones: CursorMoved::position and Touch::location (with differing types; this seems okay). Conversely, MouseScrollDelta::PixelDelta still uses a LogicalPostiion (didn't check whether there are others).

I'm actually fine with this either way; converting is easy and the types catch any errors; it just seems like an oversight being inconsistent.

Additionally, my lib mostly uses physical positions internally (to avoid fractional positions / or limited GUI scaling), and using a physical position here would reduce the number of places the lib needs to track the DPI factor.

@thebracket
Copy link

I'm also having problems with hidpi scaling and mouse position in RLTK (the default Glutin + Winit back-end).

Take the following snippet of code from the event loop:

WindowEvent::CursorMoved { position: pos, .. } => {
    let sf = wc.window().scale_factor();
    println!("Scale factor: {:?}, Pos:{:?}, Dimensions: {:?}", sf, pos, wc.window().inner_size());
}

On Windows 10, with scaling set to 100%, when you move the mouse cursor to the bottom right it outputs:

Scale factor: 1.0, Pos:PhysicalPosition { x: 636, y: 397 }, Dimensions: PhysicalSize { width: 640, height: 400 }

The PhysicalPosition is in the 0..640 range for X, and the 0..400 range for Y, which aligns with the PhysicalSize from position.

On Windows 10, with scaling set to 200%, moving to the bottom right outputs:

Scale factor: 2.0, Pos:PhysicalPosition { x: 1279, y: 793 }, Dimensions: PhysicalSize { width: 1280, height: 800 }

The PhysicalPosition is happily scaled between 0..1280 / 0..800, just like the reported PhysicalSize. Awesome.

On Ubuntu (VM), Wayland with scaling set to 100%, moving to the bottom right outputs:

Scale factor: 1.0, Pos:PhysicalPosition { x: 636, y: 397 }, Dimensions: PhysicalSize { width: 640, height: 400 }

Again - that works. Ranges are correctly in the 0..640/0..400 range.

On Ubuntu (VM), Wayland with scaling set to 200%, moving to the bottom right outputs:

Scale factor: 2.0, Pos:PhysicalPosition { x: 638, y: 397 }, Dimensions: PhysicalSize { width: 1280, height: 740 }.

So with a scale factor on Wayland, the PhysicalSize from inner_size() is the actual pixel size, but the PhysicalPosition is still in the 640x400 range pre-scaling.

I'd rather not work-around this by detecting Linux and calculate the position differently.

For reference, the parent RLTK issue is: amethyst/bracket-lib#40

@kchibisov
Copy link
Member

@thebracket Could you retest your Wayland thing with the latest winit? There're was a bug about this a while ago that should be fixed on master.

@thebracket
Copy link

I just updated to glow 0.4.0 and the latest glutin, which reference winit 0.20.0 - and get the same results. Should I be pulling from master?

One possibly related thing I noticed: if you set your scaling to 125% in Ubuntu/Wayland, window().scale_factor() still returns 2.0.

@kchibisov
Copy link
Member

kchibisov commented Jan 22, 2020

Wayland doesn't have a fractional scaling, so you can get only integers from window().scale_factor() e.g. 1,2,3,4. So everything is correct as far as I can see. Compositor can downscale(they just drawing a bit differently IIRC and not downscale, but I don't know match on this) things for you to 125%(depends on impl).

Your Wayland issue was solved in #1385 , so you should use winit from master.

@thebracket
Copy link

Thanks. I guess I'm waiting on other packages (glutin) to update, then.

@kchibisov
Copy link
Member

you can bump winit without bumping glutin IIRC, so you should just use https://doc.rust-lang.org/cargo/reference/manifest.html patch thing from here, and use winit from git.

@thebracket
Copy link

Thanks, I just found that. :-)

@thebracket
Copy link

Using the latest HEAD, I'm still having all kinds of problems on Wayland. It's pretty consistent on X11 and Windows 10.

To be sure I'm doing it right, I'm overriding the winit version in cargo.toml with:

[patch.crates-io]
winit = { git = "https://github.com/rust-windowing/winit" }
  1. So the program starts, requesting a 640x400 window.
  2. It immediately receives a Resize event, stating that the new physical size is 640x370. This leaves a "gap" at the top of the window after I resize the GL viewport; if I capture and measure the actual window, the inner size is 640x400.
  3. If I query inner_size() on the window, it shows 640x370. If I query outer_size() - that's 640x370 also.
  4. Cursor moved events come in, and the range is within 0..640 x 0..400 - not the reported 370!
  5. Requesting other window sizes doesn't make a difference. I'm requesting with .with_inner_size and a LogicalSize. Requesting a PhysicalSize makes the window not scale, but still receives the resize request with the wrong sizes, and inner/outer size are still equal.
  6. If I comment out the handling of resize events (which basically tells RLTK the new physical dimensions and changes the GL viewport), then mouse position is correct - because it's assuming I got the 640x400 window I asked for (and have, whatever the size queries say).
  7. On every other platform I've tested, resizing changing the GL viewport is enough to scale the window. On Wayland, I get a big "hole" in the window while the initial portion of the viewport gets larger. That may be a glutin problem, I'm not sure.

If I had to hazard a guess, the inner/outer size is probably the culprit on Wayland. It doesn't make any sense for them to be exactly the same (on a window with default decorations), and neither is correct! On X11, I see an inner size of 640x400 and an outer size of 640x437. Windows 10 gives me an inner of 640x400 and an outer of 640x439.

@kchibisov
Copy link
Member

kchibisov commented Jan 23, 2020

Are you testing on GNOME Wayland with window decorations?

@thebracket
Copy link

Yes. Ubuntu in a VM, normal 100% scaling (scaling to 200% is also wrong). Pretty much out-of-the-box defaults, picking "Ubuntu on Wayland" at login. "Ubuntu" (normal via X11) and dwm work great.

@kchibisov
Copy link
Member

kchibisov commented Jan 23, 2020

this 30 more for y is for decorations IIRC, since decorations are client side on GNOME Wayland.

@kchibisov
Copy link
Member

Could test with creating winit window without decorations?

@thebracket
Copy link

Turning off window decorations yields the correct sizes and mouse positioning works.

I kinda suspect that the Wayland decoration initialization is going the wrong way with the decorations resize. The outer size grew by 30 pixels, but the inner size remained 640x400 as requested (I took a screenshot and measured in an art program; the displayed window is 640x400 but the size functions reply with 370).

@dhardy
Copy link
Contributor Author

dhardy commented Jan 23, 2020

@thebracket next time, try not to hijack a completely different issue.

@thebracket
Copy link

@dhardy - Sorry. I misread the bug report and it looked like a similar physical vs. logical issue.

kchibisov pushed a commit to chrisduerr/winit that referenced this issue Jul 26, 2020
This removes the `LogicalPosition` from the `PixelDelta`, since all
other APIs have been switched to use `PhysicalPosition` instead too.

It also seems like `LineDelta` was using both logical and physical
sizes, depending on platform, and so it was changed to explicitly take a
`PhysicalPosition` parameter to force a uniform return value.

Fixes rust-windowing#1406.
kchibisov pushed a commit that referenced this issue Jul 26, 2020
This removes the `LogicalPosition` from the `PixelDelta`, since all
other APIs have been switched to use `PhysicalPosition` instead too.

Fixes #1406.
kchibisov pushed a commit to chrisduerr/winit that referenced this issue Jul 26, 2020
This removes the `LogicalPosition` from the `PixelDelta`, since all
other APIs have been switched to use `PhysicalPosition` instead too.

Fixes rust-windowing#1406.
kchibisov pushed a commit that referenced this issue Jul 26, 2020
This removes the `LogicalPosition` from the `PixelDelta`, since all
other APIs have been switched to use `PhysicalPosition` instead too.

Fixes #1406.
kchibisov pushed a commit that referenced this issue Jul 26, 2020
This removes the `LogicalPosition` from the `PixelDelta`, since all
other APIs have been switched to use `PhysicalPosition` instead.

Fixes #1406.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C - needs discussion Direction must be ironed out S - api Design and usability
Development

Successfully merging a pull request may close this issue.

4 participants