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

Improve support for High Dpi displays #1

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

lucasmerlin
Copy link

Hi! I made some changes to improve high dpi support (tested on macos and linux / pop os) and made some changes to make the api more similar to egui_winit (elapsed time is now handled by the crate).

DpiMode is used to differentiate between platforms with native hdpi support by sdl2 (e.g. MacOS) and platforms where it's manually done by the crate.

@kaphula
Copy link
Owner

kaphula commented Dec 12, 2022

Hello, thanks for the contribution!

I tried to apply these changes to my test application but there were few issues.

First issue is that I need a way to access the raw f32 dpi value from my example program. With these changes I cannot access the dpi value unless I generate it from scratch using get_dpi, and I think that's not very convenient.

Second thing that comes to my mind is that there's no easy way to apply custom dpi scaling because the struct constructor accepts only DpiMode value. Now since the library makes the decision which enum value to use, Sdl or Manual, the user would have to pattern match get_dpi and construct a new suitable scaled dpi value based on that, which again sounds a bit messy to me.

Is the main issue on MacOS here where the cursor position gets messed up if the dpi is anything else than 1.0? Do you have any idea what happens on Windows, I have no Windows machine at the moment.

Maybe instead of exposing the DpiMode enum as public to the user, we just pick the correct dpi value automatically on the main struct constructor and give the user a way to pass custom dpi scaling value as constructor parameter? In addition to this there could be a getter function for the raw dpi f32 value in case someone (like me) needs it.

What do you think?

@lucasmerlin
Copy link
Author

Without my changes, if I don't call allow_highdpi, it just looks normal but pixelated. If I do call it, it looks like this:

Bildschirmaufnahme.2022-12-19.um.11.45.45.mov

And I cant get it to fill the whole screen.
I also tried calculating the dpi with the value from video_subsystem.display_dpi but it always returns a weird value.

I think making the DpiMode private and internal only could work. Do you think a custom scaling value by the user should overwrite the systems dpi value or should it be multiplied with it?

@kaphula
Copy link
Owner

kaphula commented Dec 23, 2022

I think ideally an option for scaling and custom value would be the best. Perhaps this could be handled with an enum similar to the DpiMode.

You would choose one of these:

  • automatic dpi
  • automatic dpi with custom scale value
  • custom dpi value

I'll try to take another look at this on the upcoming holidays. Not sure if I can commit directly to this PR, but if you want to give it a go I can also review it.

Add support for dpi that can be:
- calculated automatically
- calculated automatically and scaled
- set from direct value
@kaphula
Copy link
Owner

kaphula commented Dec 25, 2022

I added support for automatic dpi, scaled automatic dpi and direct dpi value. Also included my previous example program as part of the repository. The example program is probably not ideal but it gets the job done. Can you check the code so that it looks alright to you and test this on Mac by running the example program from the repository root with this command cargo run --bin example?

@kaphula
Copy link
Owner

kaphula commented Jan 25, 2023

I think I am gonna merge this in the upcoming days unless you want to add something.

Copy link
Author

@lucasmerlin lucasmerlin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! Sorry for not getting back earlier. I gave your example a test on MacOS and first it failed because of a linker error.
Now it's compiling but if I run it it panics:
thread 'main' panicked at 'window does not have a valid contentView', /Users/lucasprivat/.cargo/registry/src/github.com-1ecc6299db9ec823/wgpu-hal-0.13.2/src/metal/surface.rs:85:13

I was able to run it with my project though and it worked great!
I did a couple changes though:

  • in the resize event I recalculate dpi, in case the window was moved to a different screen with different dpi
  • updated egui to 0.21 (had to change the keyboard events to add a "repeat" property

Unfortunately I was not able to get your example to work with egui 0.21, and the updated wgpu version that comes with it, maybe you are able to update it?

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[dependencies]
sdl2 = {version= "0.35.2", features = ["raw-window-handle", "static-link", "use-pkgconfig"] }
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
sdl2 = {version= "0.35.2", features = ["raw-window-handle", "static-link", "use-pkgconfig"] }
sdl2 = {version= "0.35.2", features = ["raw-window-handle"] }

Had to remove these to make it compile on MacOS

@kaphula
Copy link
Owner

kaphula commented Feb 16, 2023

I also tried updating the example program to egui=0.21 but could not figure out how to create a surface from the SDL2 window:

    let sdl_context = sdl2::init().expect("Cannot initialize SDL2!");
    let video_subsystem = sdl_context.video().expect("Cannot get SDL2 context!");
    let window: Window = video_subsystem
        .window("egui-sdl2-event-example", width, height)
        .position_centered()
        .resizable()
        .build()
        .map_err(|e| e.to_string())
        .expect("Cannot create SDL2 window!");


    let instance = wgpu::Instance::new(InstanceDescriptor {
        backends: Backends::all(),
        dx12_shader_compiler: Default::default(),
    });
    #[allow(unsafe_code)]
    let surface = unsafe { instance.create_surface(&window) }.unwrap(); // compiler error

I'll fix the errors once I figure out how to make rust-sdl2 and wgpu play nicely with the latest versions.

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