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

Jump to bottom

[Merged by Bors] - Add AutoMax next to ScalingMode::AutoMin #6496

Closed
wants to merge 5 commits into from
Closed

[Merged by Bors] - Add AutoMax next to ScalingMode::AutoMin #6496

wants to merge 5 commits into from

Conversation

DasLixou
Copy link
Contributor

@DasLixou DasLixou commented Nov 6, 2022

Objective

ScalingMode::Auto for cameras only targets min_height and min_width, or as the docs say it Use minimal possible viewport size while keeping the aspect ratio.

But there is no ScalingMode that targets max_height and Max_width or Use maximal possible viewport size while keeping the aspect ratio.

Solution

Added ScalingMode::AutoMax that does the exact opposite of ScalingMode::Auto

Changelog

Renamed ScalingMode::Auto to ScalingMode::AutoMin.

Migration Guide

just rename ScalingMode::Auto to ScalingMode::AutoMin if you are using it.

@alice-i-cecile alice-i-cecile added C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide A-Rendering Drawing game state to the screen labels Nov 6, 2022
@alice-i-cecile
Copy link
Member

Great first PR, thanks! I'm not going to merge this for 0.9, to avoid creating more work for the migration guide authors, but I expect this will be on main very shortly after.

@DasLixou
Copy link
Contributor Author

DasLixou commented Nov 6, 2022

Ok thanks for the great and fast feedback :) - Then ill just last the PR here and wait for more comments and problems etc

@DasLixou
Copy link
Contributor Author

I’ve been thinking about also adding an Custom / Closure ScalingMode for implementing own logic. Opinion?

@alice-i-cecile
Copy link
Member

Might be a good choice, but belongs in its own PR. I'm unsure how useful that would be: feels more complicated and less flexible than being able to dynamically change this in a system with a Manual option.

@DasLixou
Copy link
Contributor Author

That would probably be the better solution… ok well than I’ll screw that idea 😅

@DasLixou
Copy link
Contributor Author

So is there something that I still have to do? @alice-i-cecile

@alice-i-cecile
Copy link
Member

No, we just need another reviewer :) @bzm3r, can you review?

bzm3r
bzm3r reviewed Nov 13, 2022
View reviewed changes
Copy link

@bzm3r bzm3r left a comment •
edited
Loading

Choose a reason for hiding this comment

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

I think this makes sense to me, but I am also a bit unsure about something.

crates/bevy_render/src/camera/projection.rs Show resolved Hide resolved
@DasLixou
Copy link
Contributor Author

But if you want to know if it’s working as expected - I’m using it in my game and it works as it should. :)

alice-i-cecile
alice-i-cecile reviewed Nov 13, 2022
View reviewed changes
crates/bevy_render/src/camera/projection.rs Outdated
@@ -244,6 +246,8 @@ impl CameraProjection for OrthographicProjection {
max_width,
max_height,
} => {
// Compare Pixels of current width and maximal height and Pixels of maximal width with current height.
// Then use smaller (max_heigth when true) as what it referres to (height when true) and calculate rest so it can't get over maximum.
Copy link
Member

@alice-i-cecile alice-i-cecile Nov 13, 2022 •
edited
Loading

Choose a reason for hiding this comment

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

Suggested change
// Then use smaller (max_heigth when true) as what it referres to (height when true) and calculate rest so it can't get over maximum.
// Then use smaller (max_height when true) as what it refers to (height when true) and calculate rest so it can't get over maximum.

@alice-i-cecile
Copy link
Member

There was a mirror typo in the other comment that needs to be fixed too :)

@DasLixou
Copy link
Contributor Author

There was a mirror typo in the other comment that needs to be fixed too :)

Whoops - I’ll fix it :) - isn’t that simple with a German autocorrection and a smartphone 😅

@DasLixou
Copy link
Contributor Author

Now everything should be fine - hopefully 😅

@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 Nov 13, 2022
@alice-i-cecile
Copy link
Member

Awesome! This is now ready to be merged, but because it's a breaking change I'm going to hold off until after 0.9.1 drops to reduce the amount of cherrypicking needed.

@cart
Copy link
Member

cart commented Nov 14, 2022

Awesome! This is now ready to be merged, but because it's a breaking change I'm going to hold off until after 0.9.1 drops to reduce the amount of cherrypicking needed.

I'm cool with merging things now. I'd prefer not to hold back our dev processes for patch releases when cherry picking is straightforward.

@cart
Copy link
Member

cart commented Nov 14, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 14, 2022
@DasLixou
Add AutoMax next to ScalingMode::AutoMin (#6496)
b765682 
# Objective

`ScalingMode::Auto` for cameras only targets min_height and min_width, or as the docs say it `Use minimal possible viewport size while keeping the aspect ratio.`

But there is no ScalingMode that targets max_height and Max_width or `Use maximal possible viewport size while keeping the aspect ratio.`

## Solution

Added `ScalingMode::AutoMax` that does the exact opposite of `ScalingMode::Auto`

---

## Changelog

Renamed `ScalingMode::Auto` to `ScalingMode::AutoMin`.

## Migration Guide

just rename `ScalingMode::Auto` to `ScalingMode::AutoMin` if you are using it.


Co-authored-by: Lixou <[email protected]>
@bors
Copy link
Contributor

bors bot commented Nov 14, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Add AutoMax next to ScalingMode::AutoMin [Merged by Bors] - Add AutoMax next to ScalingMode::AutoMin Nov 14, 2022
@bors bors bot closed this Nov 14, 2022
@DasLixou DasLixou deleted the feature/scalingmode-automin-and-automax branch November 15, 2022 05:54
taiyoungjang pushed a commit to taiyoungjang/bevy that referenced this pull request Dec 15, 2022
@DasLixou @taiyoungjang
Add AutoMax next to ScalingMode::AutoMin (bevyengine#6496)
3545e2e 
# Objective

`ScalingMode::Auto` for cameras only targets min_height and min_width, or as the docs say it `Use minimal possible viewport size while keeping the aspect ratio.`

But there is no ScalingMode that targets max_height and Max_width or `Use maximal possible viewport size while keeping the aspect ratio.`

## Solution

Added `ScalingMode::AutoMax` that does the exact opposite of `ScalingMode::Auto`

---

## Changelog

Renamed `ScalingMode::Auto` to `ScalingMode::AutoMin`.

## Migration Guide

just rename `ScalingMode::Auto` to `ScalingMode::AutoMin` if you are using it.


Co-authored-by: Lixou <[email protected]>
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
@DasLixou @alradish
Add AutoMax next to ScalingMode::AutoMin (bevyengine#6496)
e21119d 
# Objective

`ScalingMode::Auto` for cameras only targets min_height and min_width, or as the docs say it `Use minimal possible viewport size while keeping the aspect ratio.`

But there is no ScalingMode that targets max_height and Max_width or `Use maximal possible viewport size while keeping the aspect ratio.`

## Solution

Added `ScalingMode::AutoMax` that does the exact opposite of `ScalingMode::Auto`

---

## Changelog

Renamed `ScalingMode::Auto` to `ScalingMode::AutoMin`.

## Migration Guide

just rename `ScalingMode::Auto` to `ScalingMode::AutoMin` if you are using it.


Co-authored-by: Lixou <[email protected]>
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@DasLixou @ItsDoot
Add AutoMax next to ScalingMode::AutoMin (bevyengine#6496)
b0e4b16 
# Objective

`ScalingMode::Auto` for cameras only targets min_height and min_width, or as the docs say it `Use minimal possible viewport size while keeping the aspect ratio.`

But there is no ScalingMode that targets max_height and Max_width or `Use maximal possible viewport size while keeping the aspect ratio.`

## Solution

Added `ScalingMode::AutoMax` that does the exact opposite of `ScalingMode::Auto`

---

## Changelog

Renamed `ScalingMode::Auto` to `ScalingMode::AutoMin`.

## Migration Guide

just rename `ScalingMode::Auto` to `ScalingMode::AutoMin` if you are using it.


Co-authored-by: Lixou <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@bzm3r bzm3r bzm3r approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@DasLixou @alice-i-cecile @cart @bzm3r