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 scale_factor_override not used in window creation #17208

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

Conversation

chompaa
Copy link
Member

@chompaa chompaa commented Jan 7, 2025

Objective

Fixes #17188. When scale_factor_override is set on WindowResolution, the resulting WindowResolution is incorrect for the first frame. This is because when windows are first created in bevy::winit::create_windows, the resolution is set without respect to WindowResolution::scale_factor_override:

window
    .resolution
    .set_scale_factor_and_apply_to_physical_size(winit_window.scale_factor() as f32);

Afterwards, bevy::winit::changed_windows updates the resolution at the end of the first frame.

Solution

Check if scale_factor_override is set before using OS resolution settings on window creation.

Testing

Tested using the code from the linked issue.

@chompaa chompaa added C-Bug An unexpected or incorrect behavior A-Windowing Platform-agnostic interface layer to run your app in D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jan 7, 2025
@BenjaminBrienen
Copy link
Contributor

Should there be an else that sets it using the scale_factor_override? I know you mentioned it gets set later in the first frame, but maybe it makes sense to set it here?

@chompaa
Copy link
Member Author

chompaa commented Jan 7, 2025

Should there be an else that sets it using the scale_factor_override? I know you mentioned it gets set later in the first frame, but maybe it makes sense to set it here?

I don't think so. By getting set later, I meant that the initially (incorrect) set `physical_width` and `physical_height` of the `WindowResolution` is corrected. Now it is initially set correctly.

As an aside, I am a bit confused as to why set_scale_factor_and_apply_to_physical_size exists. From my understanding:

$$\text{logical\_size} = \text{physical\_size} \times \text{scale\_factor}$$

Maybe someone else can shed light on this, but from my point of view it should just set scale_factor and not the physical size.

Edit: I was wrong, fixing now

Copy link
Contributor

@BenjaminBrienen BenjaminBrienen left a comment

Choose a reason for hiding this comment

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

example works

@BenjaminBrienen BenjaminBrienen requested a review from Aceeri January 8, 2025 03:28
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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Window size isn't correct until second frame when scale factor is overridden
2 participants