-
Notifications
You must be signed in to change notification settings - Fork 903
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
DPI for everyone #548
DPI for everyone #548
Conversation
CHANGELOG.md
Outdated
@@ -3,6 +3,16 @@ | |||
- On X11, the `Moved` event is no longer sent when the window is resized without changing position. | |||
- `MouseCursor` and `CursorState` now implement `Default`. | |||
- `WindowBuilder::with_resizable` implemented for Windows. | |||
- On X11, exiting fullscreen no longer leaves the window in the monitor's top left corner. | |||
- **Breaking:** `Window::get_hidpi_factor` has been renamed to `Window::get_hidpi_factor` for better consistency. `WindowEvent::HiDPIFactorChanged` has been renamed to ``WindowEvent::HiDpiFactorChanged``. DPI factors are always represented as `f64` instead of `f32` now. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it should be "Window::hidpi_factor
has been renamed"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
It turns out I'd already fully implemented min/max dimension DPI adjustment on Windows, but just forgot to remove the |
@vberger I migrated pf sandbox to use this branch https://github.com/rukai/PF_Sandbox/tree/super-dpi
The issue probably occurs with any vulkano project but I havnt had time to test. EditJust confirmed we get the same error with the vulkano triangle example:
|
You have no idea how much of a relief it is when a notification is for something that isn't my fault. Shouldn't this have been caught by this test? https://github.com/tomaka/winit/blob/19dd961752319276fa170c07e46963de9e3b589c/tests/send_objects.rs#L11-L15 |
running
|
Weird, @vberger feel free to just push directly to this branch. |
To confirm the issue, I ran the following commands on a Digital Ocean vps of mine
I still get the proxy.rs build failures I posted above. Maybe you need to Doesnt explain why the travis is passing though!?! |
Aaaah, ok, this is an unforseen consequence of my last release of wayland-client, sorry for that. I definitly need better testing... >< I'll correct this asap. |
Alright, wayland-client 0.20.9 is yanked, wayland-client 0.20.10 is released with the fix, and I added tests for that. All should be good again. |
Thanks, that fixes it. |
But I think coordinates of mouse events will still be computed correctly while rendering is incorrect. This will create a mismatch |
src/platform/macos/window.rs
Outdated
unsafe { | ||
let (width, height) = dimensions.unwrap_or((0, 0)); | ||
nswindow_set_min_dimensions(self.window.0, width.into(), height.into()); | ||
let dimensions = dimensions.unwrap_or_else(|| (!0, !0).into()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello, I think this line should be:
let dimensions = dimensions.unwrap_or_else(|| (0, 0).into());
Otherwise window.set_min_dimensions(None)
crashes the application by changing the window size to a huge value.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Whoops, good catch. Looks like I got carried away with copy-pasting.
@kryptan that's really unfortunate. Hopefully Intel fixes this soon, since AFAIK there's nothing we can do about it, and I don't think it makes sense to postpone this for something that hopefully only affects a small subset of users. |
src/platform/linux/x11/util/randr.rs
Outdated
(width_px as f64 * height_px as f64) / (width_mm as f64 * height_mm as f64) | ||
).sqrt(); | ||
// Quantize 1/12 step size | ||
((ppmm * (12.0 * 25.4 / 96.0)).round() / 12.0).max(1.0) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't really have a very deep understanding of this topic, but wouldn't the 96.0
here need to by the actual x11 screen DPI to properly support DPI on x11?
As far as I understand it 96.0 is only the default screen DPI on x11 and it could change based on a user's configuration. I'm not sure if in this context it's a different value that's always constant, but thought I'd just ask to properly understand this topic.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually it might be possible that this 96.0
here is just used to calculate the DPI factor you'd need to apply to a screen with 96 DPI to get the "proper" DPI scaling. So this is probably all working as intended.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I recommend reading the relevant issue #169 along with this blog post: http://wok.oblomov.eu/tecnologia/mixed-dpi-x11/
After that, you'll have an understanding with the same depth as mine, or probably better, since I know for a fact that I glossed over some things. I might not get a chance to look into that constant today, since I still have a Windows bug to investigate and a burning desire to relax for the rest of the day.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah, GitHub only sometimes wants to update the page automatically, so I didn't see that. I do encourage being skeptical of this math, since I don't know who added it or when; I just know that it makes my eyes not hurt when using winit apps on my 1440p monitor.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks a lot for the clarification and the helpful links to the documentation. Until now I've mainly just made guesses based on the math in alacritty and winit and took a short read through the freetype documentation.
Looks like this will give me some more insights, very much appreciated!
🎉 🎉 |
You may need to remove your Cargo.lock file to be able to build. Thus you will start with fresh dependencies. Require the WINIT_HIDPI_FACTOR=1 environment variable set to 1 to work flawlessly on Linux X11. This was caused by a fix in WINIT. It seems that until now, reporting DPI was buggy on X11 [1]. DPI was already reported as 1. aflak is actually a special case for which a DPI of 1 is desired (at least for now with the current design). [1] rust-windowing/winit#548
You may need to remove your Cargo.lock file to be able to build. Thus you will start with fresh dependencies. Require the WINIT_HIDPI_FACTOR=1 environment variable set to 1 to work flawlessly on Linux X11. This was caused by a fix in WINIT. It seems that until now, reporting DPI was buggy on X11 [1]. DPI was already reported as 1. aflak is actually a special case for which a DPI of 1 is desired (at least for now with the current design). [1] rust-windowing/winit#548
…#548 NgsPF benefits at great extent from this PR <rust-windowing/winit#548> as it fixes various quirks regarding the HiDPI handling and window resizing that prevail in the current master branch of `winit`.
…rement This is finally possible thanks to the PR rust-windowing/winit#548!
Fixes #62
Fixes #105
Fixes #169
Fixes #315
Fixes #332 (big thanks to @kryptan for the initial work on Windows!)
Fixes #337
Fixes #469
Fixes rust-windowing/glutin#1025
Alright, it was quite the undertaking, but I believe I've finished this on X11, Windows, macOS, iOS, Android, and Emscripten.
Some caveats:
EventsLoopExt
.src/platform/linux/mod.rs
and say "@vberger will figure this out later"That said, this PR is functionally complete. The only reason I've labeled it WIP is because it's still completely undocumented! I just wanted an opportunity for feedback before I start fussing over doc comments. (I also noticed that I left a
// TODO: Adjust min+max size
in the Windows backend, whoops)I won't reiterate what I wrote in
CHANGELOG.md
. Besides there, the other files I recommend looking at aresrc/dpi.rs
andsrc/window.rs
. You can obviously also test this, and I've provided some partial migrations of various things to help with that:GlWindow::resize
doesn't yet explicitly takePhysicalSize
.Migrating can be tedious, but isn't hard, and the more explicit API resulted in me fixing some bugs in my own applications. Overall, I'm pretty satisfied, but if anyone has objections, I'm in no rush to merge this. I want to release a 0.15.1 with various regression fixes before introducing any breaking changes like this.