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.

Sign up for GitHub

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
Closed

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

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
72f795e
Add Window Resize Constraints
gabriel-gheorghe Feb 6, 2021
bb4f1bb
Fix format
gabriel-gheorghe Feb 6, 2021
6140363
Fix resize_constraints docs
gabriel-gheorghe Feb 6, 2021
0599ae4
Change minimum resolution to 180x120
gabriel-gheorghe Feb 6, 2021
9785627
Make max inner size infinite by default
gabriel-gheorghe Feb 6, 2021
b3e9147
Fix format
gabriel-gheorghe Feb 6, 2021
f15e8dc
Remove Resize Constraints from Window struct
gabriel-gheorghe Feb 6, 2021
6e32651
Update Window Resize Constraints
gabriel-gheorghe Feb 9, 2021
5480a40
Add Window Set Resize Constraints Command
gabriel-gheorghe Feb 10, 2021
4e52e5f
Fix format and warnings
gabriel-gheorghe Feb 10, 2021
6d834e5
Fix format in utils
gabriel-gheorghe Feb 10, 2021
015883f
Fix clippy errors
gabriel-gheorghe Feb 10, 2021
0f9d101
Move float_cmp to check_resize_constraints
gabriel-gheorghe Feb 10, 2021
0cf3b88
Code refactoring
gabriel-gheorghe Feb 10, 2021
4cfd81d
Remove .clone() call on resize constraints data
gabriel-gheorghe Feb 10, 2021
d7d4632
Remove another .clone()
gabriel-gheorghe Feb 10, 2021
07e14e9
Remove unused imports
gabriel-gheorghe Feb 10, 2021
e84d4cb
Fix format
gabriel-gheorghe Feb 10, 2021
5c888e8
Add docs to WindowResizeConstraints
gabriel-gheorghe Feb 12, 2021
31fa5f4
remove utils method in favor of WindowResizeConstraints::check_constr…
cart Mar 3, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions crates/bevy_window/src/window.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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.

use std::fmt;

impl fmt::Display for WindowId {
Expand All @@ -32,6 +33,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

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

pub min_width: f32,
pub min_height: f32,
pub max_width: f32,
pub max_height: f32,
}

impl Default for WindowResizeConstraints {
fn default() -> Self {
Self {
min_width: 180.,
min_height: 120.,
max_width: f32::INFINITY,
max_height: f32::INFINITY,
}
}
}

/// An operating system window that can present content and receive user input.
///
/// ## Window Sizes
Expand Down Expand Up @@ -255,6 +275,7 @@ impl Window {
if self.requested_width == width && self.requested_height == height {
return;
}

self.requested_width = width;
self.requested_height = height;
self.command_queue.push(WindowCommand::SetResolution {
Expand Down Expand Up @@ -424,6 +445,7 @@ impl Window {
pub struct WindowDescriptor {
pub width: f32,
pub height: f32,
pub resize_constraints: WindowResizeConstraints,
pub scale_factor_override: Option<f64>,
pub title: String,
pub vsync: bool,
Expand All @@ -442,6 +464,7 @@ impl Default for WindowDescriptor {
title: "bevy".to_string(),
width: 1280.,
height: 720.,
resize_constraints: WindowResizeConstraints::default(),
scale_factor_override: None,
vsync: true,
resizable: true,
Expand Down
48 changes: 47 additions & 1 deletion crates/bevy_winit/src/winit_windows.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
use core::f32;

use bevy_math::IVec2;
use bevy_utils::HashMap;
use bevy_window::{Window, WindowDescriptor, WindowId, WindowMode};
use bevy_window::{Window, WindowDescriptor, WindowId, WindowMode, WindowResizeConstraints};
use winit::dpi::LogicalSize;

#[derive(Debug, Default)]
pub struct WinitWindows {
Expand All @@ -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?

pub fn create_window(
&mut self,
event_loop: &winit::event_loop::EventLoopWindowTarget<()>,
Expand All @@ -25,6 +29,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

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


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


winit_window_builder = match window_descriptor.mode {
WindowMode::BorderlessFullscreen => winit_window_builder.with_fullscreen(Some(
winit::window::Fullscreen::Borderless(event_loop.primary_monitor()),
Expand Down Expand Up @@ -59,6 +94,17 @@ impl WinitWindows {
.with_decorations(window_descriptor.decorations),
};

#[allow(unused_mut)]
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()

{
winit_window_builder
.with_min_inner_size(min_inner_size)
.with_max_inner_size(max_inner_size)
} else {
winit_window_builder.with_min_inner_size(min_inner_size)
};

#[allow(unused_mut)]
let mut winit_window_builder = winit_window_builder.with_title(&window_descriptor.title);

Expand Down
8 changes: 7 additions & 1 deletion examples/window/window_settings.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,4 @@
use bevy::prelude::*;
use bevy::{prelude::*, window::WindowResizeConstraints};

/// This example illustrates how to customize the default window settings
fn main() {
Expand All @@ -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.

resize_constraints: WindowResizeConstraints {
max_width: 1280.,
max_height: 720.,
..Default::default()
},
..Default::default()
})
.add_plugins(DefaultPlugins)
Expand Down