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

fixed scaling issues #9

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

fixed scaling issues #9

wants to merge 5 commits into from

Conversation

Wicpar
Copy link

@Wicpar Wicpar commented Aug 8, 2022

fixes #8

@derivator
Copy link
Owner

Hey, thanks for the PR! :)

I want to merge this, but it doesn't seem to be a complete fix yet?

According to emilk/egui#172, it should be possible to use Context::set_pixels_per_point to scale the UI (independently of the display's scale factor), but this doesn't work properly. With a scale factor of 2, only the top left quadrant of the display is drawn to.
I think the fix would be to multiply window_size_points by egui_ctx.pixels_per_point() in Painter::draw, but I'm not sure if/how this interacts with high-DPI displays. What do you think?

@Wicpar
Copy link
Author

Wicpar commented Aug 14, 2022

i didn't see that. Having looked at it now there are two disctinct variables, one in egui and another in egui winit there is a conflict. Egui-Winit creates a desync between the screen rect in logical pixels in take_egui_input as it has an internal ppp that desyncs with the context ppp.

also according to documentation:

pub fn set_pixels_per_point
... 
Note that this may be overwritten by input from the integration via RawInput::pixels_per_point. For instance, when using eframe on web, the browsers native zoom level will always be used.

the correct way to implement this would be to fix egui-winit by making it use the system scale factor directly and having a user set multiplier.
This PR will be correct once the bug is fixed as the error stems from a desync between ppp and screen rect set by imgui winit in take_egui_input

@derivator
Copy link
Owner

I now think the correct fix on top of your changes is to let egui tell us the scale factor:

let scale_factor = egui_ctx.pixels_per_point() as f64; // egui_winit sets this to the window's native scale factor
let size = surface.window().inner_size().to_logical(scale_factor);

By default this is the window's scale factor from winit, based on your display settings.
egui_winit::State::handle_platform_output does keep track of modifications to the scale factor:

self.current_pixels_per_point = egui_ctx.pixels_per_point(); // someone can have changed it to scale the UI

The problem is that surface.window().scale_factor() doesn't know if a different value has been set inside egui.

@Wicpar
Copy link
Author

Wicpar commented Aug 15, 2022

indeed it was that, my bad, i was wrong.

@Wicpar
Copy link
Author

Wicpar commented Aug 15, 2022

also we may want to change the functions to do those calculations within the draw function to prevent bugs in user code. Maybe implement a trait that queries the window size if we don't want to depend on winit.

@derivator
Copy link
Owner

Maybe implement a trait that queries the window size if we don't want to depend on winit.

Not sure what you mean exactly. I do want to avoid a dependency on winit, that should be handled by egui_winit.
We could rename the parameter window_size_points to window_size_pixels in draw and document that you should pass in the physical size.

By the way, I would prefer to merge this separately from the update to vulkano 0.30, to keep a clean git history. You could probably do an interactive rebase or use cherry picking to untangle the commits. I could also do it myself if you prefer (keeping you as commit author, of course).

@Wicpar
Copy link
Author

Wicpar commented Aug 15, 2022

if you could do it it would be nice so you can have it your way, i don't care for authorship.

@derivator
Copy link
Owner

Ok, I will take care of it, thanks! :)

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.

Incorrect rendering in example for 5120x2160, scale factor 175%
2 participants