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

Fix windows not being centered properly when system interface is scaled #8903

Merged
merged 3 commits into from
Jun 21, 2023

Conversation

marlyx
Copy link
Contributor

@marlyx marlyx commented Jun 20, 2023

Objective

Fixes #8765

Solution

When windows are created during plugin setup, the scale_factor of a WindowResolution struct will always be 1.0 (default). The correct scale factor is set later in flow. To get correct center calculations use the monitors scale factor directly instead.

Results

System: Windows 10 Pro (125% scaling)

main

scale_125_without_fix

This PR

scale_125_with_fix

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior A-Windowing Platform-agnostic interface layer to run your app in labels Jun 20, 2023
@alice-i-cecile
Copy link
Member

Good fix, good comments :)

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 20, 2023
@Selene-Amanita
Copy link
Member

Selene-Amanita commented Jun 20, 2023

This doesn't take into account scale_factor_override (maybe it shouldn't? I'm trying to write the documentation for WindowResolution in #8858 and I have a hard time understanding how it works, or what is a bug and what is intended behavior)

For example this doesn't spawn at the center of the screen (when default scale_factor is 1., before and after this PR so not a regression at least):

Window {
    position: WindowPosition::Centered(MonitorSelection::Primary),
    resolution: WindowResolution::new(500., 300.).with_scale_factor_override(2.0),
    ..default()
}

@alice-i-cecile alice-i-cecile removed the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 20, 2023
@marlyx
Copy link
Contributor Author

marlyx commented Jun 21, 2023

This doesn't take into account scale_factor_override (maybe it shouldn't? I'm trying to write the documentation for WindowResolution in #8858 and I have a hard time understanding how it works, or what is a bug and what is intended behavior)

Yeah, that is true. And I thought a bit about this when I worked on this, but to be honest I also had a hard time figuring out what the override should and shouldn't affect so I went with how the original code handled it. (It used .base_scale_factor() instead of .scale_factor() and that explicitly ignores the override)

Now that I have thought about it a bit more, I think the logical thing here should be that the centering code should account for override if the window size code does and vice versa. I will do some more testing and see if that is the case

@Selene-Amanita
Copy link
Member

Selene-Amanita commented Jun 21, 2023

I think scale_factor_override is supposed to mean "zoom on the window [change physical size] without changing the relative size of the elements inside it [don't change logical size]" and with that it would make sense that the window stays centered.

But yeah not sure at all and even if scale_factor_override should be taken into account your PR would still be better as it is than before the PR so. Taking it into account might not be an easy fix too. Maybe we can merge this PR and open an issue for this.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Jun 21, 2023
@alice-i-cecile
Copy link
Member

Even if it's not a complete fix I think this behaviour is better than the existing.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 21, 2023
Merged via the queue into bevyengine:main with commit 98eb1d5 Jun 21, 2023
22 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Windowing Platform-agnostic interface layer to run your app in C-Bug An unexpected or incorrect behavior S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Bevy does not center window properly when system interface is scaled
4 participants