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

Update to glutin 0.26.0 + winit 0.24.0 #334

Merged
merged 5 commits into from
Dec 20, 2020
Merged

Update to glutin 0.26.0 + winit 0.24.0 #334

merged 5 commits into from
Dec 20, 2020

Conversation

iceiix
Copy link
Owner

@iceiix iceiix commented Jun 20, 2020

Combining parts of #317 and #323

@iceiix
Copy link
Owner Author

iceiix commented Jun 20, 2020

This compiles and runs, but introduces a serious problem on my system (macOS, iMac, 2.0 hidpi): cursor tracking is broken, for example, in this screenshot my cursor is on the tip of the "v" in the logo but it highlights the first server list entry:

Screen Shot 2020-06-20 at 2 31 52 PM

If I move the mouse down so it is actually over the first server list entry, then it highlights the last server list entry box shown:

Screen Shot 2020-06-20 at 2 31 54 PM

If I revert this commit, going back to the master branch with glutin 0.22.0 + winit 0.20.0, then the problem no longer occurs (mouse tracking is accurate).

Not clear how the glutin/winit update changed this behavior. The only code change here besides the dependency update is updating for the new ModifiersChange event, which seems unrelated. It also doesn't seem to be related to hidpi because the scaling doesn't appear to be off by 2.0 or 0.5, though it might be.

@iceiix
Copy link
Owner Author

iceiix commented Jun 20, 2020

Actually, this could be hidpi-related. Looks like the mouse coordinates are 2x off.

Screen Shot 2020-06-20 at 2 46 51 PM

@iceiix iceiix mentioned this pull request Jun 20, 2020
@iceiix
Copy link
Owner Author

iceiix commented Jun 20, 2020

Reverting .to_logical::<f64>(game.dpi_factor).into(); to .into(); seems to get closer, but isn't quite right. In this screenshot, the mouse is slightly over halfway down the first server list item (2/3?), but the second server list item is highlighted:

Screen Shot 2020-06-20 at 2 52 16 PM

it also doesn't seem like the right solution. Why does winit change how they handle the dpi scale factor so frequently? Needs more investigation to see what they changed and how to update for winit 0.20 to winit 0.22.

This appears relevant: https://github.com/rust-windowing/winit/blob/master/CHANGELOG.md#0220-2020-03-09

0.22.0 (2020-03-09)
Breaking: Use i32 instead of u32 for position type in WindowEvent::Moved.

0.21.0 (2020-02-04)
Breaking: WindowEvent::CursorMoved changed to f64 units, preserving high-precision data supplied by most backends

@mkroening
Copy link
Collaborator

I can confirm that this breaks cursor tracking on x11. I'm not sure, if I'll find time to dig deeper soon though.

@iceiix
Copy link
Owner Author

iceiix commented Jun 21, 2020

Rebased on top of master with rustfmt changes

@iceiix
Copy link
Owner Author

iceiix commented Jul 4, 2020

Another mouse tracking issue I noticed, while investigating #36 - on Windows on VMware Fusion, even on the master branch (without the changes in this PR), the mouse position is slightly off (about 1/3 of the server list, similar symptoms as described here). This may be related to the virtualization environment, to glutin/winit, or to the DPI scale factor..

@iceiix iceiix changed the title Update to glutin 0.24.1 + winit 0.22.2 Update to glutin 0.25.0 + winit 0.23.0 Oct 10, 2020
@iceiix iceiix force-pushed the glutin24 branch 2 times, most recently from a3d0fe9 to a81d107 Compare October 10, 2020 20:05
@iceiix
Copy link
Owner Author

iceiix commented Oct 10, 2020

Updated to glutin 0.25.1 + winit 0.23.0, to close dependabot #410 #407 (and removed Cargo.lock from this branch to ease merging), the problem persists. Looks like we'll have to figure out why this broke...

@iceiix iceiix changed the title Update to glutin 0.25.0 + winit 0.23.0 Update to glutin 0.26.0 + winit 0.24.0 Dec 20, 2020
@iceiix
Copy link
Owner Author

iceiix commented Dec 20, 2020

Updated again, to glutin 0.26.0 and winit 0.24.0 to close #430 and #431 - same mouse tracking problem persists

@iceiix
Copy link
Owner Author

iceiix commented Dec 20, 2020

Note: updating only to glutin 0.23.0 reproduces the mouse tracking bug (see #418); this transitively updates winit to 0.21.0:

rust-windowing/glutin@f071c72

  • Update winit crate to 0.21.0 (#1275)
  • Update winit crate to 0.21.0
  • Update changelog, fix 0.22.1 entry
  • Remove armv7-apple-ios from GitHub CI
  • Bump glutin to 0.23.0

Updating to only winit 0.21.0 directly, and leaving glutin at 0.22.0 (so it uses its own winit 0.20.0), does not repro the problem. winit 0.24.0 + glutin 0.22.0 also does not repro (same; since glutin 0.22.0 uses winit 0.20.0, which is OK). Ah... winit is in our Cargo.toml is only used for the wasm target to enable stdweb. It can be removed. The only winit that matters at the moment is the version is used by glutin.

Changes in winit 0.21.0: https://github.com/rust-windowing/winit/commits/v0.21.0
(until 0.20.0 = "we did it bois" commit)

Interesting commits:

Most platforms (X11, wayland, macos, stdweb, ...) provide physical positions in f64 units, which can contain meaningful fractional data. For example, this can be empirically observed on modern X11 using a typical laptop touchpad. This is useful for e.g. content creation tools, where cursor motion might map to brush strokes on a canvas with higher-than-screen resolution, or positioning of an object in a vector space.

On macOS, fix CursorMoved event reporting the cursor position using logical coordinates.

which was to fix: rust-windowing/winit#1371 CursorMoved position is returns a PhysicalPosition with logical values inside it (winit 0.20 on MacOS + a 13" MacBook Pro connected to a 1440p non-retina display)

So our current usage depends on a winit 0.20.0 bug?

@iceiix
Copy link
Owner Author

iceiix commented Dec 20, 2020

--- a/src/main.rs
+++ b/src/main.rs
@@ -357,7 +357,7 @@ fn main2() {
         let delta = (diff.subsec_nanos() as f64) / frame_time;
         let physical_size = window.window().inner_size();
         let (physical_width, physical_height) = physical_size.into();
-        let (width, height) = physical_size.to_logical::<f64>(game.dpi_factor).into();
+        let (width, height) = window.window().inner_size().into();
 
         let version = {
             let try_res = game.resource_manager.try_write();

fixes the mouse scaling, but shrinks the UI:

Screen Shot 2020-12-19 at 6 58 27 PM

inner_size() does return a PhysicalSize: https://docs.rs/winit/0.24.0/winit/window/struct.Window.html#method.inner_size
where we want a logical size, so it is converted. But it has to be consistent with hover_at().

WindowEvent::CursorMoved now provides a PhysicalPosition, which we destructure and pass to hover_at(x, y), along with (width, height) from window.window().inner_size(), which is a PhysicalSize. Convert it all to logical?

@iceiix
Copy link
Owner Author

iceiix commented Dec 20, 2020

Have a fix, it works at least on my system (hopefully doesn't break others... but this code now appears "correct", it was only working before due to a library bug)

CursorMoved changed in winit 0.21 to return PhysicalPosition, so we need
to convert it to LogicalPosition. Change the width/height passed to be
LogicalSize instead of a PhysicalPosition, matching the LogicalPosition.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants