[Merged by Bors] - Add Window Resize Constraints by comet-flare · Pull Request #1409 · bevyengine/bevy

comet-flare · 2021-02-06T10:16:27Z

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.

bjorn3 · 2021-02-06T11:39:53Z

+            min_width: 360.,
+            min_height: 240.,
+            max_width: 23040.,
+            max_height: 8640.,


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

What minimum resolution do you propose for a window?

Maybe 180x120?

bjorn3 · 2021-02-06T11:40:19Z

        #[cfg(not(target_os = "windows"))]
        let mut winit_window_builder = winit::window::WindowBuilder::new();

+        let default_resize_constraints = WindowResizeConstraints::default();


This should use the WindowDescriptor, right?

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

bjorn3 · 2021-02-06T11:40:48Z

    requested_height: f32,
    physical_width: u32,
    physical_height: u32,
+    resize_constraints: WindowResizeConstraints,


There is no command to change this.

Should I remove this or create a command?

You should probably add a command to change it

bjorn3 · 2021-02-06T20:36:58Z

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

cart · 2021-02-06T21:22:47Z

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.

DJMcNab · 2021-02-09T15:24:43Z

+        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;
+        }


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;

}

DJMcNab · 2021-02-09T15:25:36Z

+        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,
+        };


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,

};

DJMcNab · 2021-02-09T15:26:46Z

+        let mut winit_window_builder = if final_resize_constraints.max_width != f32::INFINITY
+            && final_resize_constraints.max_height != f32::INFINITY


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()

DJMcNab · 2021-02-09T15:29:32Z

    }
 }

+use core::f32;


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

Ups, the Rust plugin for VSCode added that.

DJMcNab · 2021-02-09T15:30:09Z

 }

 impl WinitWindows {
+    #[allow(clippy::float_cmp)]


With my other changes this should no longer be needed

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

Oh true, yeah

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

comet-flare · 2021-02-09T16:06:54Z

I will make the changes asap. Thanks!

comet-flare · 2021-02-10T10:25:55Z

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

DJMcNab · 2021-02-10T10:39:58Z

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 · 2021-02-10T10:41:29Z

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 · 2021-02-10T10:44:50Z

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 · 2021-02-10T10:47:03Z

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

comet-flare · 2021-02-10T10:52:05Z

maximizable

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

DJMcNab · 2021-02-10T11:00:45Z

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.

DJMcNab

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)

DJMcNab · 2021-02-10T11:49:00Z

    }
 }

+#[derive(Debug, Clone)]


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

Copy requires Clone

DJMcNab · 2021-02-10T11:51:28Z

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


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.

Nice, I didn't know about irrefutable pattern.

DJMcNab · 2021-02-10T11:57:35Z

+use bevy_log::warn;
+use bevy_window::WindowResizeConstraints;
+
+#[allow(clippy::float_cmp)]


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

DJMcNab · 2021-02-10T11:58:40Z

            .with_decorations(window_descriptor.decorations),
        };

+        let WindowResizeConstraints {


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

DJMcNab · 2021-02-10T12:02:42Z

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


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() {

DJMcNab · 2021-02-10T12:04:05Z

            width: 500.,
            height: 300.,
            vsync: true,
+            resizable: true,


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 · 2021-02-12T14:26:18Z

I think it is ready to go now.

DJMcNab

Just one suggestion for documentation.

DJMcNab · 2021-02-12T15:39:43Z

 }

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


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 · 2021-02-13T20:41:26Z

@cart I think this is ready for sign off

…aints

cart · 2021-03-03T02:56:17Z

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 · 2021-03-03T02:56:35Z

bors r+

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 · 2021-03-03T03:11:04Z

Pull request successfully merged into main.

Build succeeded:

doublegarts added 3 commits February 6, 2021 12:11

Add Window Resize Constraints

72f795e

Fix format

bb4f1bb

Fix resize_constraints docs

6140363

bjorn3 reviewed Feb 6, 2021

View reviewed changes

doublegarts added 4 commits February 6, 2021 14:47

Change minimum resolution to 180x120

0599ae4

Make max inner size infinite by default

9785627

Fix format

b3e9147

Remove Resize Constraints from Window struct

f15e8dc

DJMcNab suggested changes Feb 9, 2021

View reviewed changes

doublegarts added 6 commits February 9, 2021 22:44

Update Window Resize Constraints

6e32651

Add Window Set Resize Constraints Command

5480a40

Fix format and warnings

4e52e5f

Fix format in utils

6d834e5

Fix clippy errors

015883f

Move float_cmp to check_resize_constraints

0f9d101

DJMcNab approved these changes Feb 10, 2021

View reviewed changes

doublegarts added 4 commits February 10, 2021 15:33

Code refactoring

0cf3b88

Remove .clone() call on resize constraints data

4cfd81d

Remove another .clone()

d7d4632

Remove unused imports

07e14e9

Fix format

e84d4cb

DJMcNab approved these changes Feb 12, 2021

View reviewed changes

Add docs to WindowResizeConstraints

5c888e8

mockersf mentioned this pull request Feb 16, 2021

Decreasing window height to 0 causes crash #170

Closed

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 mentioned this pull request Feb 17, 2021

Don't resize when viewport 0x0 #545

Closed

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

mockersf approved these changes Feb 17, 2021

View reviewed changes

Base automatically changed from master to main February 19, 2021 20:44

remove utils method in favor of WindowResizeConstraints::check_constr…

31fa5f4

…aints

bors Bot changed the title ~~Add Window Resize Constraints~~ [Merged by Bors] - Add Window Resize Constraints Mar 3, 2021

bors Bot closed this Mar 3, 2021

mockersf mentioned this pull request Mar 20, 2021

wgpu error: Validation Error - Dimension X is zero #1664

Closed

trolleyman mentioned this pull request Jun 3, 2021

window: (minimize, 0 size, oom) crashes #2299

Closed

ickshonpe mentioned this pull request Dec 8, 2022

Remove the style component from ui widgets #6886

Closed

		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 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()

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

	#[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() {

Uh oh!

Conversation

comet-flare commented Feb 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comet-flare Feb 6, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bjorn3 commented Feb 6, 2021

Uh oh!

cart commented Feb 6, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comet-flare commented Feb 9, 2021

Uh oh!

comet-flare commented Feb 10, 2021

Uh oh!

DJMcNab commented Feb 10, 2021

Uh oh!

comet-flare commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

comet-flare commented Feb 10, 2021

Uh oh!

comet-flare commented Feb 10, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

comet-flare commented Feb 10, 2021

Uh oh!

DJMcNab commented Feb 10, 2021

Uh oh!

DJMcNab left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

comet-flare Feb 6, 2021 •

edited

Loading

comet-flare commented Feb 10, 2021 •

edited

Loading

comet-flare commented Feb 10, 2021 •

edited

Loading

DJMcNab left a comment •

edited

Loading