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

Added use_dpi setting to WindowDescriptor #1131

Merged
merged 2 commits into from
Dec 28, 2020

Conversation

TheRawMeatball
Copy link
Member

Maybe closes #1118

@memoryruins memoryruins added C-Enhancement A new feature A-Rendering Drawing game state to the screen labels Dec 22, 2020
Copy link
Member

@DJMcNab DJMcNab left a comment

Choose a reason for hiding this comment

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

Overall looks good - there are a few changes which could be useful, but nothing obviously blocking.

examples/window/window_settings.rs Outdated Show resolved Hide resolved
crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved
crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_winit/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_ui/src/flex/mod.rs Outdated Show resolved Hide resolved
crates/bevy_winit/src/winit_windows.rs Outdated Show resolved Hide resolved
examples/ui/ui.rs Outdated Show resolved Hide resolved
@razaamir
Copy link

Can I be removed from this thread

@dev-inigmas
Copy link

Wow, I wasn't expecting such quick action on my request. This is awesome. Thanks guys.

@DJMcNab
Copy link
Member

DJMcNab commented Dec 22, 2020

Can I be removed from this thread

You need to click unsubscribe.

@DJMcNab
Copy link
Member

DJMcNab commented Dec 22, 2020

On my machine the new UI from your example is somewhat broken in higher scaling modes, which doesn't make sense. Also I feel like we should call some stuff use_scaling etc instead of use_dpi, but I'm not sure.

@TheRawMeatball
Copy link
Member Author

Can you post some details and a screenshot?

@DJMcNab
Copy link
Member

DJMcNab commented Dec 22, 2020

So in theory, because the UI is all done based on logical pixels, the images should be identical (barring scaling artifacts). However, 100% gives me:
image
and 125% gives me
image
where for some reason the left panel stays the same width, and the font doesn't move down.

@DJMcNab
Copy link
Member

DJMcNab commented Dec 22, 2020

That is definitely better, although there's still some jank - if you go through this video frame by frame for example you can see a few examples.

Examples.2020.12.22.-.19.25.50.01.mp4

That being said, it's not super important, because we don't really expect people to be changing window scaling on/off that often ™️

@TheRawMeatball
Copy link
Member Author

Yeah, I'm fine with it stuttering when its being cycled at such a frequency. Given that it requires a full UI redraw on every switch, its not very surprising.

@blunted2night
Copy link
Contributor

A couple of thoughts. Would system_scale_factor be a better name that true_scale_factor? Would it be simpler to make scale_factor the current scale factor instead of having to check use_dpi every where? This would either be the user selected value, or automatically updated when the system changes. Does it make sense to include the scale factor as part of the window resized event since these are tightly coupled?

@TheRawMeatball
Copy link
Member Author

Would it be simpler to make scale_factor the current scale factor instead of having to check use_dpi every where? This would either be the user selected value, or automatically updated when the system changes.

I don't exactly understand what you're offering here. Do you mean you want to change Window.scale_factor so it stores 1 when using physical pixels? In such a case, we'd have to store the scaling factor from winit in a seperate variable, effectively caching the result of scale_factor(). I dont think its worth the additional complexity.

Does it make sense to include the scale factor as part of the window resized event since these are tightly coupled?

They aren't tightly coupled. Both are triggered by seperate events, and are handled by diffrent pieces of code.

@DJMcNab
Copy link
Member

DJMcNab commented Dec 22, 2020

I think they mean that when scale_factor is ignored, the scale_factor method should return 1.

i.e. the current scale_factor method should be

pub fn scale_factor(&self)->f32{
    if self.use_dpi { 1 } else{ self.scale_factor }
}

and there should be a

pub fn requested_scale_factor(&self)->f32 {self.scale_factor}

@TheRawMeatball
Copy link
Member Author

This is the current behaviour, thats why I am confused.

@blunted2night
Copy link
Contributor

Would it be simpler to make scale_factor the current scale factor instead of having to check use_dpi every where? This would either be the user selected value, or automatically updated when the system changes.

I don't exactly understand what you're offering here. Do you mean you want to change Window.scale_factor so it stores 1 when using physical pixels? In such a case, we'd have to store the scaling factor from winit in a seperate variable, effectively caching the result of scale_factor(). I dont think its worth the additional complexity.

Yeah, I guess it might be nice to specify a custom scale factor in addition to disabling the system value. And the scale_factor variable would always hold the current value, with another holding the system scale factor if required. While subjective, to me having scale_factor() perform logic to know what the real current scale factor is more complex than a separate variable recording the current system scale_factor independent of the currently selected scale factor.

Does it make sense to include the scale factor as part of the window resized event since these are tightly coupled?

They aren't tightly coupled. Both are triggered by seperate events, and are handled by diffrent pieces of code.

I was thinking that changes to the scaling factor will effect the resolution reported and from the users perspective it might be better to receive both of these at the same time.

@dev-inigmas
Copy link

Being able to supply your own "scale_factor" could be useful for testing.

@TheRawMeatball
Copy link
Member Author

I made a major rework to implement @blunted2night s suggestions and clean everything up.

@cart
Copy link
Member

cart commented Dec 27, 2020

rebased on master

@cart
Copy link
Member

cart commented Dec 28, 2020

I made a few tweaks. Feel free to contest any of them:

  • Removed window.set_physical_resolution(). its a nice shorthand, but i think it over-complicates things. Too much implicit behavior when it comes state changes (could change scale factor or requested resolution). Id prefer to just let people override scale factor to 1.0 then use set_resolution
  • Moved the scale factor override example logic to its own example. I want to keep the window_settings example simple
  • Renamed "true scale factor" cases to "backend scale factor". "true" felt a bit too arbitrary / undefined to me.
  • Renamed instances of DPI to "scale factor". DPI != scale factor. DPI is a measure of pixel density whereas scale factor is a scale preference.
  • simplified event handling logic
  • style tweaks

@TheRawMeatball
Copy link
Member Author

Sounds good!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen C-Enhancement A new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The ability to disable the recently added hidpi window resizing feature
7 participants