Exposes Common Window Resolutions as an Enum. #14158

Sapein · 2024-07-05T15:56:07Z

Objective

Implements: #14153

Solution

Exposes Common Window Resolutions that are likely to be used through an Enum (Resolution) that can be turned into a UVec2 and an iterable.

Testing

I've tested this briefly locally. I wrote a small test just to double check that things worked as expected and it passed.

github-actions · 2024-07-05T15:56:20Z

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

MiniaczQ

Please run cargo fmt 🙂

crates/bevy_window/src/window.rs

MiniaczQ · 2024-07-05T16:01:25Z

Should we add a Custom variant that can have arbitrary w/h values?
It would be skipped from iter, but would expose a decent interface

… fmt`

MiniaczQ

Looks good to me, we can add the Custom variant if necessary in another PR

Sapein · 2024-07-05T16:07:54Z

Looks good to me, we can add the Custom variant if necessary in another PR

Apologies for that btw. I was working on it while you were reviewing and I didn't notice.

I'm fine with adding a Custom variant as well, since it would be rather nice.

Also there are a few questions regarding the resolutions, as an example exposing a 4k variant would also probably be useful, and 360p can technically be one of two resolutions. That is, 480 x 360 which is 4:3 and 680 x 360 which is 16:9. I went with the latter because that's what I tend to use, and it works better with other resolutions (mostly having to do with scaling better to 720p and higher).

janhohenheim

Having both WindowResolution and Resolution could lead to confusion. How about renaming it to CommonScreenResolution? That's what the doc comment says anyways.

Sapein · 2024-07-05T16:35:53Z

I'm not against CommonScreenResolution, but something about it doesn't feel quiet right to me tbh, although I'm not against it as it is a better name than Resolution especially given context.

Also should it be Resolution or Resolutions? I'm actually not sure what the conventions are here regarding that...

alice-i-cecile · 2024-07-05T17:22:04Z

I quite like this: this is a utility that almost every game will want. To make this better:

More doc comments, explaining why this is useful.
Breadcrumbs to this in at least one example: probably window_settings or game_menu. I would really love to wire this up to a settings menu but that might be too much work for one PR.
CommonScreenResolution is the name I'd use: Resolution is way too broad for a little utility.
A doc test demonstrating how to use it.
A nice Display implementation for use in menus that shows "1920 x 1080" and so on.

Long-term I'd like to see more variants and an CommonAspectRatio enum with a CommonScreenResolution::aspect_ratio method but that's easily split off.

Sapein · 2024-07-05T17:36:28Z

I quite like this: this is a utility that almost every game will want. To make this better:

More doc comments, explaining why this is useful.

Breadcrumbs to this in at least one example: probably window_settings or game_menu. I would really love to wire this up to a settings menu but that might be too much work for one PR.

CommonScreenResolution is the name I'd use: Resolution is way too broad for a little utility.

A doc test demonstrating how to use it.

A nice Display implementation for use in menus that shows "1920 x 1080" and so on.

Long-term I'd like to see more variants and an CommonAspectRatio enum with a CommonScreenResolution::aspect_ratio method but that's easily split off.

CommonScreenResolution it is then. I'll be working down this list as I can. Also, how would you imagine the CommonAspectRatio enum to work? I'm mostly asking because of the aforementioned two 360p resolutions.

FWIW I've tended to implement things similar to this across a few games now, because it's useful to not have to try and remember the exact details about the resolution I'm using.

I'm also more than willing to add in more variants as well. I just started off with what I tend to use (plus 2k).

Also, should I change the usage of the default construction of WindowResolution to use the enum? It's currently set to 1280x720 by default. (Also I just realized this could be used to change the default WindowResolutions through some sort of setting or something if desired in the future)

alice-i-cecile · 2024-07-05T17:41:40Z

I was thinking:

enum CommonAspectRatio {
   FourToThree,
   SixteenToNine
}

We should also have a From<CommonScreenResolution> for WindowResoltion, now that you mention it. I'm fine to leave the default impl alone for now though.

Sapein · 2024-07-05T17:48:51Z

Fair enough. I'll go ahead and implement From<CommonScreenResolution> for WindowResolution as well, and leave the Default for now, it was mostly just something I thought of as a bit more of a side thing.

That would also be rather useful, although I'm not sure how it would entirely work, but it would help with cases where a resolution can be one of two -- dependent upon the Aspect Ratio.

(Technically 360p could be split into "360p" and "nHD" (the former being 4:3 and the latter being 16:9) as those are the Standard Names, according to Wikipedia. However, the class for both is 360p and I don't think a lot of people would recognize nHD tbqh.)

janhohenheim · 2024-07-05T17:59:00Z

Please do use it in WindowResoltion::default; many users click on the implementation of these to find out how they're setup. As such, I consider that a kind of documentation. Having CommonScreenResolution there will point them the way on how to cleanly setup their own resolution :)

Sapein · 2024-07-05T18:10:03Z

A nice Display implementation for use in menus that shows "1920 x 1080" and so on.

Would we want it to display "w x h" or something like the name ("360p", "720p", etc)? I've created an implementation that does the former based on this comment, as I imagine that it would probably be more useful for in game menus. But thought I would ask just to make sure.

alice-i-cecile · 2024-07-05T18:12:47Z

The former is what I've seen in game menus so I think it's better to stick to that.

crates/bevy_window/src/window.rs

…ng change.

janhohenheim · 2024-07-05T19:32:41Z

examples/window/window_settings.rs

+fn toggle_resolutions(input: Res<ButtonInput<KeyCode>>, mut windows: Query<&mut Window>) {
+ if input.just_pressed(KeyCode::KeyR) {
+ let mut window = windows.single_mut();
+ let current_resolution = CommonScreenResolution::iter()
+ .find(|&r| WindowResolution::from(r) == window.resolution);
+ window.resolution = if current_resolution.is_none() {
+ CommonScreenResolution::R360p.into()
+ } else {
+ let resolutions = CommonScreenResolution::iter()
+ .map(std::convert::Into::into)
+ .collect::<Vec<CommonScreenResolution>>();
+ let current_resolution_index = CommonScreenResolution::iter()
+ .position(|r| r == current_resolution.unwrap())
+ .unwrap();
+
+ if current_resolution_index + 1 >= resolutions.len() {
+ resolutions[0].into()
+ } else {
+ resolutions[current_resolution_index + 1].into()
+ }
+ }
+ }
+}


This would be more idiomatic if CurrentResolution was a Resource initialized at CommonScreenResolution::R360p imo

You can use Iterator::cycle(), store that as a resource and call next every time input is pressed

GunboatDiplomat · 2024-07-07T14:05:55Z

crates/bevy_window/src/window.rs

+ R1080p,
+
+ /// 2560 x 1440
+ R2k,


Calling QHD resolution 2K is a misnomer. 2K is closer to 1080p (FullHD) than 2.5k (QHD).

alice-i-cecile · 2024-09-09T16:05:39Z

@Sapein if you get back to this, take a look at the changes in #15091 :) Let us know if you'd like any advice on adapting this PR to work with it.

Sapein · 2024-09-09T16:15:04Z

@Sapein if you get back to this, take a look at the changes in #15091 :) Let us know if you'd like any advice on adapting this PR to work with it.

Hey, thanks for letting me know about that and I would really like some advice on adapting it to work with that as well, since I'm not entirely certain how it should tbh.

alice-i-cecile · 2024-09-09T16:38:21Z

Paging @miniex, you may have ideas here as well :)

miniex · 2024-09-09T17:08:15Z

I like structure that can be dynamically created and extended.

First, what about resolution cateogry?

ex:

pub enum ResolutionCategory {
    HD,
    FHD,
    QHD,
    Mobile,
    Ultrawide,
}

impl CommonScreenResolution {
    pub fn category(&self) -> ResolutionCategory {
        ...
    }
}

And, i like a good idea to allow them to be made in AspectRatio.

Then, that make it easier to add documents or solutions?

My english is bad, so I'm not sure if i understood it. ToT

Sapein · 2024-09-09T17:38:21Z

I like structure that can be dynamically created and extended.

First, what about resolution cateogry?

ex:
pub enum ResolutionCategory {
    HD,
    FHD,
    QHD,
    Mobile,
    Ultrawide,
}

impl CommonScreenResolution {
    pub fn category(&self) -> ResolutionCategory {
        ...
    }
}
And, i like a good idea to allow them to be made in AspectRatio.

Then, that make it easier to add documents or solutions?

My english is bad, so I'm not sure if i understood it. ToT

I'm not sure I understand how this is supposed to work, tbqh. Could you try and elaborate or provide some examples?

IceSentry · 2024-09-09T17:58:11Z

I don't think this should be an enum and I'm not convinced at all it even needs to exists. An ordered list could make sense for a setting menu, but that list should be dynamically generated based on what the hardware supports. It should include ultrawide resolutions for people using an ultrawide.

Monitors are not limited to just 16:9. Games or apps should pretty much never hardcode a resolution and they should adapt their game to work at any aspect ratio and resolution. Some games need a fixed aspect ratio for artistic reasons, while I personally disagree that it should ever be done, if a game has to do it, it should not be done at the window level. The window should still be able to scale to any screen size. If we had to make a list, and I really don't think we should, but we should use something like steam hardware survey. We shouldn't encourage people to test at a fixed 16:9 ratio, it just makes things more painful for any player that doesn't have a 16:9 screen.

miniex · 2024-09-09T18:11:29Z

My opinion is same as @IceSentry.
As example, i made this code. I don't know this is good code.

use bevy_math::AspectRatio;

pub struct Resolution {
    width: u32,
    height: u32,
    category: ResolutionCategory,
}

pub enum ResolutionCategory {
    HD,
    FHD,
    QHD,
    Mobile,
    Ultrawide,
}

impl Resolution {
    pub fn new(width: u32, height: u32, category: ResolutionCategory)
    pub fn aspect_ratio(&self) -> AspectRatio
}

pub struct CommonScreenResolutions {
    resolutions: HashMap<String, Resolution>,
}

impl CommonScreenResolutions {
    // or default
    pub fn new() -> Self {
        let mut resolutions = HashMap::new();
        resolutions.insert("360p_4:3", Resolution::new(480, 360, ResolutionCategory::SD));
        resolutions.insert("360p_16:9", Resolution::new(640, 360, ResolutionCategory::SD));
        resolutions.insert("720p", Resolution::new(1280, 720, ResolutionCategory::HD));
        resolutions.insert("1080p", Resolution::new(1920, 1080, ResolutionCategory::FHD));
        Self { resolutions }
    }
    
    pub fn add(&mut self, name: String, resolution: Resolution)
    pub fn get(&self, name: &str) -> Option<&Resolution>
    pub fn find_by_dimensions(&self, width: u32, height: u32) -> Option<(&String, &Resolution)>
    pub fn find_by_aspect_ratio(&self, aspect_ratio: AspectRatio) -> Vec<(&String, &Resolution)>
}

miniex · 2024-09-09T18:15:52Z

find_by_category

Sapein · 2024-09-09T18:35:54Z

I don't think this should be an enum and I'm not convinced at all it even needs to exists. An ordered list could make sense for a setting menu, but that list should be dynamically generated based on what the hardware supports. It should include ultrawide resolutions for people using an ultrawide.

Monitors are not limited to just 16:9. Games or apps should pretty much never hardcode a resolution and they should adapt their game to work at any aspect ratio and resolution. Some games need a fixed aspect ratio for artistic reasons, while I personally disagree that it should ever be done, if a game has to do it, it should not be done at the window level. The window should still be able to scale to any screen size. If we had to make a list, and I really don't think we should, but we should use something like steam hardware survey. We shouldn't encourage people to test at a fixed 16:9 ratio, it just makes things more painful for any player that doesn't have a 16:9 screen.

I strongly disagree, as this is a common thing that I've had to setup for all of my projects, and it's a pain point that Bevy has, in that this is rather unfriendly IMHO. It's also not uncommon for 2D games in particular to focus on specific resolutions that they support as well, especially if you're doing pixel art.

alice-i-cecile · 2024-09-09T18:53:58Z

Reopening, but marking controversial.

benfrankel · 2024-09-09T18:56:54Z

Games or apps should pretty much never hardcode a resolution and they should adapt their game to work at any aspect ratio and resolution.

FWIW, a lot of games in my Steam library either prevent you from resizing the window to a different aspect ratio, or very clearly were not tested on different aspect ratios. Those are real published games. Sure, it would be nice for a game to support any aspect ratio, but evidently it isn't necessary and many developers consider it not worth the effort and artistic compromise.

IceSentry · 2024-09-09T19:19:52Z

I strongly disagree, as this is a common thing that I've had to setup for all of my projects, and it's a pain point that Bevy has, in that this is rather unfriendly IMHO. It's also not uncommon for 2D games in particular to focus on specific resolutions that they support as well, especially if you're doing pixel art.

We should have an easy way to do letterboxing of course, but hiding this behind something that appears as a "common resolution" is not the way it should be done. Common resolution are a myth and we shoudn't encourage devs to believe there is such a thing as a common resolution. It should be very clear that it is something only used for letterboxing for artistic purposes.

FWIW, a lot of games in my Steam library either prevent you from resizing the window to a different aspect ratio, or very clearly were not tested on different aspect ratios. Those are real published games. Sure, it would be nice for a game to support any aspect ratio, but evidently it isn't necessary and many developers consider it not worth the effort and artistic compromise.

I'm extremely aware of that and it's one of the main reason why I don't want to push people towards a limited set of resolution at a fixed aspect ratio. Most games actually do handle this correctly which makes the ones that don't even more jarring. I really don't think this should be encouraged by making this the easy path and calling it a common resolution.

cart · 2024-09-09T19:56:34Z

but hiding this behind something that appears as a "common resolution" is not the way it should be done

I agree. Any "opinionated" view of available resolutions is a user-facing problem to solve (either directly by the app developer, or as a 3rd party ecosystem plugin).

I'm pretty adamantly against including this upstream, so in the interest of saving time I'm closing this out. @Sapein I encourage you to create a 3rd party plugin for this! (and register it in Bevy Assets)

janhohenheim · 2024-09-09T21:18:41Z

@IceSentry @cart interesting, I didn't know that having such a list hardcoded in the settings was considered bad practice. Thanks for pointing it out :)
Do you have any resources on how to do this the right way in Bevy? How do I dynamically generate such a list based on hardware for the settings menu?
Another thing that is not clear to me is how fixed aspect ratio games that do this for artistic purposes choose an aspect ratio. If common aspect ratios are a myth, do they just pick something arbitrary like 1300x700 because that happens to look good? Assuming best practices, of course.

janhohenheim · 2024-09-09T23:51:38Z

Update: the answer is Monitor::video_modes

cart · 2024-09-10T00:08:25Z

Yup the answer is some combination of querying for supported resolutions, constraining to specific resolutions, and letterboxing.

For letterboxing (including how to do it with Render Targets and Bevy UI), see this issue: #15130

Many games choose to constrain to specific aspect ratios or specific resolutions for artistic or gameplay reasons, especially many 2D pixel-perfect games, but also some 3D games (such as competitive shooters like Valorant).

Bevy's job isn't to lock in what specific resolutions or aspect ratios to support (or recommend). Bevy's job is to provide the full range of support and let developers adopt whatever constraints they want.

Exposes Common Window Resolutions as an Enum.

e08c878

MiniaczQ reviewed Jul 5, 2024

View reviewed changes

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved

Added documentation to the variants and to iter()

38f080f

Sapein added 2 commits July 5, 2024 11:02

Update documentation to fit with suggestions from review. Runs `cargo…

76ced3f

… fmt`

Fix typo.

c8555a5

MiniaczQ approved these changes Jul 5, 2024

View reviewed changes

Fixes other typo.

8842456

janhohenheim suggested changes Jul 5, 2024

View reviewed changes

alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 5, 2024

Rename Resolution to CommonScreenResolution.

bad94ba

Sapein added 2 commits July 5, 2024 12:50

Update documentation

f8b8dfb

Add in From<CSR> for WindowResolution.

884ed08

Implement display for CommonScreenResolution.

8deccf6

Fix Display for CommonScreenResolution.

a450128

Sapein added 2 commits July 5, 2024 14:15

Fix Formatting.

6b2871a

Add in some linting feedback.

fa8e089

janhohenheim suggested changes Jul 5, 2024

View reviewed changes

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved

crates/bevy_window/src/window.rs Outdated Show resolved Hide resolved

Make CommonScreenResolution non-exhaustive so updates aren't a breaki…

7fb37ee

…ng change.

janhohenheim suggested changes Jul 5, 2024

View reviewed changes

Update window.rs to remove code duplication.

dceeff7

janhohenheim added this to the 0.15 milestone Jul 7, 2024

GunboatDiplomat reviewed Jul 7, 2024

View reviewed changes

janhohenheim mentioned this pull request Sep 9, 2024

Add common aspect ratio constants and improve documentation #15091

Merged

alice-i-cecile closed this Sep 9, 2024

alice-i-cecile mentioned this pull request Sep 9, 2024

Expose common window resolutions #14153

Closed

alice-i-cecile reopened this Sep 9, 2024

alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Sep 9, 2024

cart closed this Sep 9, 2024

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Exposes Common Window Resolutions as an Enum. #14158

Exposes Common Window Resolutions as an Enum. #14158

Sapein commented Jul 5, 2024

github-actions bot commented Jul 5, 2024

MiniaczQ left a comment

MiniaczQ commented Jul 5, 2024

MiniaczQ left a comment

Sapein commented Jul 5, 2024

janhohenheim left a comment

Sapein commented Jul 5, 2024

alice-i-cecile commented Jul 5, 2024

Sapein commented Jul 5, 2024 •

edited

Loading

alice-i-cecile commented Jul 5, 2024

Sapein commented Jul 5, 2024

janhohenheim commented Jul 5, 2024 •

edited

Loading

Sapein commented Jul 5, 2024 •

edited

Loading

alice-i-cecile commented Jul 5, 2024

janhohenheim Jul 5, 2024

MiniaczQ Jul 7, 2024

GunboatDiplomat Jul 7, 2024 •

edited

Loading

alice-i-cecile commented Sep 9, 2024

Sapein commented Sep 9, 2024

alice-i-cecile commented Sep 9, 2024

miniex commented Sep 9, 2024

Sapein commented Sep 9, 2024

IceSentry commented Sep 9, 2024 •

edited

Loading

miniex commented Sep 9, 2024

miniex commented Sep 9, 2024

Sapein commented Sep 9, 2024

alice-i-cecile commented Sep 9, 2024

benfrankel commented Sep 9, 2024

IceSentry commented Sep 9, 2024 •

edited

Loading

cart commented Sep 9, 2024 •

edited

Loading

janhohenheim commented Sep 9, 2024 •

edited

Loading

janhohenheim commented Sep 9, 2024

cart commented Sep 10, 2024

Exposes Common Window Resolutions as an Enum. #14158

Exposes Common Window Resolutions as an Enum. #14158

Conversation

Sapein commented Jul 5, 2024

Objective

Solution

Testing

github-actions bot commented Jul 5, 2024

MiniaczQ left a comment

Choose a reason for hiding this comment

MiniaczQ commented Jul 5, 2024

MiniaczQ left a comment

Choose a reason for hiding this comment

Sapein commented Jul 5, 2024

janhohenheim left a comment

Choose a reason for hiding this comment

Sapein commented Jul 5, 2024

alice-i-cecile commented Jul 5, 2024

Sapein commented Jul 5, 2024 • edited Loading

alice-i-cecile commented Jul 5, 2024

Sapein commented Jul 5, 2024

janhohenheim commented Jul 5, 2024 • edited Loading

Sapein commented Jul 5, 2024 • edited Loading

alice-i-cecile commented Jul 5, 2024

janhohenheim Jul 5, 2024

Choose a reason for hiding this comment

MiniaczQ Jul 7, 2024

Choose a reason for hiding this comment

GunboatDiplomat Jul 7, 2024 • edited Loading

Choose a reason for hiding this comment

alice-i-cecile commented Sep 9, 2024

Sapein commented Sep 9, 2024

alice-i-cecile commented Sep 9, 2024

miniex commented Sep 9, 2024

Sapein commented Sep 9, 2024

IceSentry commented Sep 9, 2024 • edited Loading

miniex commented Sep 9, 2024

miniex commented Sep 9, 2024

Sapein commented Sep 9, 2024

alice-i-cecile commented Sep 9, 2024

benfrankel commented Sep 9, 2024

IceSentry commented Sep 9, 2024 • edited Loading

cart commented Sep 9, 2024 • edited Loading

janhohenheim commented Sep 9, 2024 • edited Loading

janhohenheim commented Sep 9, 2024

cart commented Sep 10, 2024

Sapein commented Jul 5, 2024 •

edited

Loading

janhohenheim commented Jul 5, 2024 •

edited

Loading

Sapein commented Jul 5, 2024 •

edited

Loading

GunboatDiplomat Jul 7, 2024 •

edited

Loading

IceSentry commented Sep 9, 2024 •

edited

Loading

IceSentry commented Sep 9, 2024 •

edited

Loading

cart commented Sep 9, 2024 •

edited

Loading

janhohenheim commented Sep 9, 2024 •

edited

Loading