bevyengine / bevy

A refreshingly simple data-driven game engine built in Rust
https://bevyengine.org
Apache License 2.0
36.13k stars 3.56k forks source link

The `WindowResolution` API is very confusing #11066

Open ickshonpe opened 10 months ago

ickshonpe commented 10 months ago

What problem does this solve or what need does it fill?

The WindowResolution struct:

pub struct WindowResolution {
    /// Width of the window in physical pixels.
    physical_width: u32,
    /// Height of the window in physical pixels.
    physical_height: u32,
    /// Code-provided ratio of physical size to logical size.
    ///
    /// Should be used instead of `scale_factor` when set.
    scale_factor_override: Option<f64>,
    /// OS-provided ratio of physical size to logical size.
    ///
    /// Set automatically depending on the pixel density of the screen.
    scale_factor: f64,
}

It has a method new:

pub fn new(logical_width: f32, logical_height: f32) -> Self {
        Self {
            physical_width: logical_width as u32,
            physical_height: logical_height as u32,
            ..Default::default()
        }
    }

Which looks like nonsense as we are writing values in logical pixels into fields named physical_width and physical_height.

It's not a bug though. What happens is that on window creation the windowing backend retrieves the OS scale factor (assume the user hasn't set a scalefactor override) and writes it to the scale_factor field. Then it multiplies the logical physical_width and physical_height values by the scale factor to get the underlying resolution in physical pixels that the window should be created with.

It's very confusing and difficult to understand from the code, particularly because the window creation backend logic is in a separate module.

What solution would you like?

My preferred API would be something like this:

pub struct Window {
    /// Desired resolution for the window
    pub target_resolution: TargetWindowResolution,
    /// Resolution of the window, automatically set on creation
    pub current_resolution: Option<WindowResolution>,
    // .. rest of Window's fields
}

pub struct TargetWindowResolution {
    pub size: LogicalSize,
    pub scale_factor_override: Option<f32>, 
}

pub struct WindowResolution {
    pub size: PhysicalSize,
    pub scale_factor: f32,
}

What alternative(s) have you considered?

We could make WindowResolution into an enum instead, something like:

pub enum WindowResolution {
    Target {
        size: LogicalSize,
        scale_factor_override: Option<f32>,              
    },
    Current {
        size: PhysicalSize,
        scale_factor: f32,
        scale_factor_override: Option<f32>,
    }
}

Additional context

Some more discussion and context at #11015

entropylost commented 2 months ago

Can we at least rename physical_width and physical_height to logical_width and logical_height since they're.. logical? The current api has pub fn new(physical_width: f32, physical_height: f32) -> Self {. It also casts to u32 immediately which means that it's impossible to make odd-sized windows on double dpi screens I think?