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

[Merged by Bors] - Add Window Resize Constraints #1409

Closed
wants to merge 20 commits into from

Conversation

comet-flare
Copy link

You should be able to set the minimum and maximum desired resolution of a system window.
This also fixes a bug on Windows operating system: When you try to resize to 0 on the height it crashes.

min_width: 360.,
min_height: 240.,
max_width: 23040.,
max_height: 8640.,
Copy link
Contributor

Choose a reason for hiding this comment

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

I think there should be no default maximum size (so no .with_max_inner_size call by default) and a smaller default minimum size.

Copy link
Author

@comet-flare comet-flare Feb 6, 2021

Choose a reason for hiding this comment

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

What minimum resolution do you propose for a window?

Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe 180x120?

@@ -25,6 +27,37 @@ impl WinitWindows {
#[cfg(not(target_os = "windows"))]
let mut winit_window_builder = winit::window::WindowBuilder::new();

let default_resize_constraints = WindowResizeConstraints::default();
Copy link
Contributor

Choose a reason for hiding this comment

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

This should use the WindowDescriptor, right?

Copy link
Author

Choose a reason for hiding this comment

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

This is just for some checks to force the minunum size and fix that crash at 0 height

@@ -54,6 +73,7 @@ pub struct Window {
requested_height: f32,
physical_width: u32,
physical_height: u32,
resize_constraints: WindowResizeConstraints,
Copy link
Contributor

Choose a reason for hiding this comment

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

There is no command to change this.

Copy link
Author

Choose a reason for hiding this comment

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

Should I remove this or create a command?

Copy link
Member

Choose a reason for hiding this comment

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

You should probably add a command to change it

@bjorn3
Copy link
Contributor

bjorn3 commented Feb 6, 2021

I just found #545 which solves the crash in a completely different way.

@cart
Copy link
Member

cart commented Feb 6, 2021

Yupyup. I think its worth doing both. This pr solves the problem for most cases, but if someone manually overrides the min size, then it would be good to handle the error gracefully.

Comment on lines 32 to 51
let default_resize_constraints = WindowResizeConstraints::default();
let mut final_resize_constraints = window_descriptor.resize_constraints.clone();
if final_resize_constraints.min_width < default_resize_constraints.min_width {
final_resize_constraints.min_width = default_resize_constraints.min_width;
}
if final_resize_constraints.min_height < default_resize_constraints.min_height {
final_resize_constraints.min_height = default_resize_constraints.min_height;
}
if final_resize_constraints.max_width > default_resize_constraints.max_width {
final_resize_constraints.max_width = default_resize_constraints.max_width;
}
if final_resize_constraints.max_height > default_resize_constraints.max_height {
final_resize_constraints.max_height = default_resize_constraints.max_height;
}
if final_resize_constraints.max_width < final_resize_constraints.min_width {
final_resize_constraints.max_width = default_resize_constraints.max_width;
}
if final_resize_constraints.max_height < final_resize_constraints.min_height {
final_resize_constraints.max_height = default_resize_constraints.max_height;
}
Copy link
Member

Choose a reason for hiding this comment

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

Ignoring the user's minimum size when it's less than the provided defaults is probably not the behaviour we wish to indroduce.

Suggested change
let default_resize_constraints = WindowResizeConstraints::default();
let mut final_resize_constraints = window_descriptor.resize_constraints.clone();
if final_resize_constraints.min_width < default_resize_constraints.min_width {
final_resize_constraints.min_width = default_resize_constraints.min_width;
}
if final_resize_constraints.min_height < default_resize_constraints.min_height {
final_resize_constraints.min_height = default_resize_constraints.min_height;
}
if final_resize_constraints.max_width > default_resize_constraints.max_width {
final_resize_constraints.max_width = default_resize_constraints.max_width;
}
if final_resize_constraints.max_height > default_resize_constraints.max_height {
final_resize_constraints.max_height = default_resize_constraints.max_height;
}
if final_resize_constraints.max_width < final_resize_constraints.min_width {
final_resize_constraints.max_width = default_resize_constraints.max_width;
}
if final_resize_constraints.max_height < final_resize_constraints.min_height {
final_resize_constraints.max_height = default_resize_constraints.max_height;
}
let WindowResizeConstraints { mut min_width, mut min_height, mut max_width, mut max_height } = window_descriptor.resize_constraints.clone();
min_width = min_width.max(1.);
min_height = min_height.max(1.);
if max_width < min_width {
warn!("The given maximum width {} is smaller than the minimum width {}", max_width, min_width);
max_width = min_width;
}
if max_height < min_height {
warn!("The given maximum height {} is smaller than the minimum height {}", max_height, min_height);
max_height = min_height;
}

Comment on lines 53 to 61
let min_inner_size = LogicalSize {
width: final_resize_constraints.min_width,
height: final_resize_constraints.min_height,
};

let max_inner_size = LogicalSize {
width: final_resize_constraints.max_width,
height: final_resize_constraints.max_height,
};
Copy link
Member

Choose a reason for hiding this comment

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

I agree with the use of LogicalSize here - nice job 👍

Suggested change
let min_inner_size = LogicalSize {
width: final_resize_constraints.min_width,
height: final_resize_constraints.min_height,
};
let max_inner_size = LogicalSize {
width: final_resize_constraints.max_width,
height: final_resize_constraints.max_height,
};
let min_inner_size = LogicalSize {
width: min_width,
height: min_height,
};
let max_inner_size = LogicalSize {
width: max_width,
height: max_height,
};

Comment on lines 98 to 99
let mut winit_window_builder = if final_resize_constraints.max_width != f32::INFINITY
&& final_resize_constraints.max_height != f32::INFINITY
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
let mut winit_window_builder = if final_resize_constraints.max_width != f32::INFINITY
&& final_resize_constraints.max_height != f32::INFINITY
let mut winit_window_builder = if max_width.is_finite() && max_height.is_finite()

@@ -18,6 +18,7 @@ impl WindowId {
}
}

use core::f32;
Copy link
Member

Choose a reason for hiding this comment

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

I'm not sure what the purpose of this line here is - the constants are also to the primitive types, which are already in the prelude

Copy link
Author

Choose a reason for hiding this comment

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

Ups, the Rust plugin for VSCode added that.

@@ -10,6 +13,7 @@ pub struct WinitWindows {
}

impl WinitWindows {
#[allow(clippy::float_cmp)]
Copy link
Member

Choose a reason for hiding this comment

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

With my other changes this should no longer be needed

Copy link
Author

Choose a reason for hiding this comment

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

But there is: 'if max_width < min_width" and "if max_height < min_height"

Copy link
Member

Choose a reason for hiding this comment

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

Oh true, yeah

I think it's fine, although what does clippy want us to do instead?

@comet-flare
Copy link
Author

I will make the changes asap. Thanks!

@comet-flare
Copy link
Author

There is still an issue. When you maximize your window it doesn't care if it is larger than your max resize constraints.

@DJMcNab
Copy link
Member

DJMcNab commented Feb 10, 2021

Related: rust-windowing/winit#588

In this case, I'm not actually sure what the use case for a window with a maximum size but which can be resized to be made smaller than that is - that is, it's good that we support it but I don't want to make the process perfect. That is, if people don't want their window to be maximised they should use a ResizeConstraint with the same minimum and maximum

I'd be inclined to just document it as a winit limitation, and if someone decides their game needs it they can push the plumbing through themselves

@comet-flare
Copy link
Author

comet-flare commented Feb 10, 2021

Related: rust-windowing/winit#588

In this case, I'm not actually sure what the use case for a window with a maximum size but which can be resized to be made smaller than that is - that is, it's good that we support it but I don't want to make the process perfect. That is, if people don't want their window to be maximised they should use a ResizeConstraint with the same minimum and maximum

I'd be inclined to just document it as a winit limitation, and if someone decides their game needs it they can push the plumbing through themselves

I propose this:
bevy_window::WindowCommand::SetMaximized { maximized } => { let window = winit_windows.get_window(id).unwrap(); let maximized_resolution = window. //?????? logical if can_maximize(bevy_window.resize_constraints(), maximized_resolution) { window.set_maximized(maximized) } else { warn!( "Coudn't maximize Window with Id {:?} due to resolution not supported", winit_window_id, ); } }
but not sure how to get the maximized resolution

@comet-flare
Copy link
Author

Related: rust-windowing/winit#588
In this case, I'm not actually sure what the use case for a window with a maximum size but which can be resized to be made smaller than that is - that is, it's good that we support it but I don't want to make the process perfect. That is, if people don't want their window to be maximised they should use a ResizeConstraint with the same minimum and maximum
I'd be inclined to just document it as a winit limitation, and if someone decides their game needs it they can push the plumbing through themselves

I propose this:
bevy_window::WindowCommand::SetMaximized { maximized } => { let window = winit_windows.get_window(id).unwrap(); let maximized_resolution = window. //?????? logical if can_maximize(bevy_window.resize_constraints(), maximized_resolution) { window.set_maximized(maximized) } else { warn!( "Coudn't maximize Window with Id {:?} due to resolution not supported", winit_window_id, ); } }
but not sure how to get the maximized resolution

Related: rust-windowing/winit#588
In this case, I'm not actually sure what the use case for a window with a maximum size but which can be resized to be made smaller than that is - that is, it's good that we support it but I don't want to make the process perfect. That is, if people don't want their window to be maximised they should use a ResizeConstraint with the same minimum and maximum
I'd be inclined to just document it as a winit limitation, and if someone decides their game needs it they can push the plumbing through themselves

I propose this:
bevy_window::WindowCommand::SetMaximized { maximized } => { let window = winit_windows.get_window(id).unwrap(); let maximized_resolution = window. //?????? logical if can_maximize(bevy_window.resize_constraints(), maximized_resolution) { window.set_maximized(maximized) } else { warn!( "Coudn't maximize Window with Id {:?} due to resolution not supported", winit_window_id, ); } }
but not sure how to get the maximized resolution

image

@comet-flare
Copy link
Author

comet-flare commented Feb 10, 2021

Or maybe just add an maximizable boolean and a set_maximizable command.

@comet-flare
Copy link
Author

maximizable

As I can see winit doesn't support it yet so I think this PR is done.

@DJMcNab
Copy link
Member

DJMcNab commented Feb 10, 2021

I'd say we don't need to capture the set maximised command. Ideally, we'd detect maximisation to greater than the constratints and warn. But I don't think we should automatically demaximise, because that feels likely to be very janky.

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.

I'm happy - a few small follow up changes but once those are made it's good to go.

(@cart, in this case if we had that set up [and I had the permissions] I'd issue an @bors delegate+ command)

@@ -32,6 +32,25 @@ impl Default for WindowId {
}
}

#[derive(Debug, Clone)]
Copy link
Member

Choose a reason for hiding this comment

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

Since this is just plain old data and not especially big, I think it might be worth just making it Copy

Copy link
Author

Choose a reason for hiding this comment

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

Copy requires Clone

Comment on lines 6 to 13
resize_constraints: WindowResizeConstraints,
) -> WindowResizeConstraints {
let WindowResizeConstraints {
mut min_width,
mut min_height,
mut max_width,
mut max_height,
} = resize_constraints;
Copy link
Member

Choose a reason for hiding this comment

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

There's a clever thing you can do here, which is to use irrefutable pattern matching in the function argument. That is

Suggested change
resize_constraints: WindowResizeConstraints,
) -> WindowResizeConstraints {
let WindowResizeConstraints {
mut min_width,
mut min_height,
mut max_width,
mut max_height,
} = resize_constraints;
WindowResizeConstraints {
mut min_width,
mut min_height,
mut max_width,
mut max_height,
}: WindowResizeConstraints,
) -> WindowResizeConstraints {

Not sure how rustfmt will format it, but you get the idea.

Copy link
Author

Choose a reason for hiding this comment

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

Nice, I didn't know about irrefutable pattern.

use bevy_log::warn;
use bevy_window::WindowResizeConstraints;

#[allow(clippy::float_cmp)]
Copy link
Member

Choose a reason for hiding this comment

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

I think the float_cmp lint was only about the infinity check - removing it no longer gives an error.

Ideally, we'd have some way to handle unused lint supression

@@ -59,6 +61,30 @@ impl WinitWindows {
.with_decorations(window_descriptor.decorations),
};

let WindowResizeConstraints {
Copy link
Member

Choose a reason for hiding this comment

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

I'd be tempted to use a 4 tuple here, but I do think this is better code 👍

Comment on lines 79 to 80
#[allow(unused_mut)]
let mut winit_window_builder = if max_width.is_finite() && max_height.is_finite() {
Copy link
Member

Choose a reason for hiding this comment

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

In this case I don't think the mut will ever actually be used at all, since the builder is always consumed in the next clause

Suggested change
#[allow(unused_mut)]
let mut winit_window_builder = if max_width.is_finite() && max_height.is_finite() {
let winit_window_builder = if max_width.is_finite() && max_height.is_finite() {

@@ -8,6 +8,12 @@ fn main() {
width: 500.,
height: 300.,
vsync: true,
resizable: true,
Copy link
Member

Choose a reason for hiding this comment

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

Not sure if this should be merged into WindowResizeConstraints - however that would be in a seperate PR anyway.

I suspect it shouldn't be just because we don't want to merge too many settings into one command.

@comet-flare
Copy link
Author

I think it is ready to go now.

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.

Just one suggestion for documentation.

@@ -32,6 +32,25 @@ impl Default for WindowId {
}
}

#[derive(Debug, Clone, Copy)]
pub struct WindowResizeConstraints {
Copy link
Member

Choose a reason for hiding this comment

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

A small documentation comment here would be nice. Just something like

/// The size limits on a `Window`. These values are in logical pixels, so the user's scale
/// factor does affect the size limits on the `Window`
/// Please note that if the window is resizable, then when the window is maximised it 
/// may have a size outside of these limits. The functionality required to disable maximising
/// is not yet exposed by winit

@DJMcNab
Copy link
Member

DJMcNab commented Feb 13, 2021

@cart I think this is ready for sign off

@alice-i-cecile alice-i-cecile added C-Bug An unexpected or incorrect behavior O-Windows Specific to the Windows desktop operating system A-Windowing Platform-agnostic interface layer to run your app in labels Feb 17, 2021
@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 Feb 17, 2021
Base automatically changed from master to main February 19, 2021 20:44
@cart
Copy link
Member

cart commented Mar 3, 2021

This looks good to me. I made one minor adjustment (removed the "util" mod/method in favor of WindowResizeConstraints::check_constraints()). I think in general util / helper / etc are overly generic and try to avoid them when possible.

@cart
Copy link
Member

cart commented Mar 3, 2021

bors r+

bors bot pushed a commit that referenced this pull request Mar 3, 2021
You should be able to set the minimum and maximum desired resolution of a system window.
This also fixes a bug on Windows operating system: When you try to resize to 0 on the height it crashes.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors
Copy link
Contributor

bors bot commented Mar 3, 2021

@bors bors bot changed the title Add Window Resize Constraints [Merged by Bors] - Add Window Resize Constraints Mar 3, 2021
@bors bors bot closed this Mar 3, 2021
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 O-Windows Specific to the Windows desktop operating system 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.

7 participants