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

Introduce AspectRatio struct #10368

Merged
merged 4 commits into from
Dec 17, 2023
Merged

Introduce AspectRatio struct #10368

merged 4 commits into from
Dec 17, 2023

Conversation

Olle-Lukowski
Copy link
Contributor

@Olle-Lukowski Olle-Lukowski commented Nov 4, 2023 •
edited
Loading

Objective

Solution

  • Created an intermediate AspectRatio struct, as suggested in the issue. This is currently just used in any places where aspect ratio calculations happen, to prevent doing it wrong. In my and @mamekoro 's opinion, it would be better if this was used instead of a normal f32 in various places, but I didn't want to make too many changes to begin with.

Migration Guide

  • Anywhere where you are currently expecting a f32 when getting aspect ratios, you will now receive a AspectRatio struct. this still holds the same value.

@alice-i-cecile alice-i-cecile added A-Rendering Drawing game state to the screen C-Usability A simple quality-of-life change that makes Bevy easier to use A-Math Fundamental domain-agnostic mathematical operations C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change labels Nov 4, 2023
@Olle-Lukowski @alice-i-cecile
Last fix?
c45d382 
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@rparrett rparrett added the C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 6, 2023
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Nov 6, 2023

It looks like your PR is a breaking change, but you didn't provide a migration guide.

Could you add some context on what users should update when this change get released in a new version of Bevy?
It will be used to help writing the migration guide for the version. Putting it after a ## Migration Guide will help it get automatically picked up by our tooling.

@JMS55 JMS55 added this to the 0.13 milestone Nov 23, 2023
@Olle-Lukowski
Copy link
Contributor Author

Anything I need to do before this can be merged?

@alice-i-cecile
Copy link
Member

Other than resolving merge conflicts, probably not. We need a second reviewer; let me ask around for one.

@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 Dec 17, 2023
@alice-i-cecile
Copy link
Member

Merging! FYI, next time you probably want to create a branch with a unique name on your repo for each PR: it really helps you keep things straight.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Dec 17, 2023
Merged via the queue into bevyengine:main with commit 6b9cd57 Dec 17, 2023
22 checks passed
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

@100-TomatoJuice 100-TomatoJuice 100-TomatoJuice approved these changes

Assignees
No one assigned
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Rendering Drawing game state to the screen C-Breaking-Change A breaking change to Bevy's public API that needs to be noted in a migration guide C-Bug An unexpected or incorrect behavior C-Code-Quality A section of code that is hard to understand or change C-Usability A simple quality-of-life change that makes Bevy easier to use 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
0.13
Development

Successfully merging this pull request may close these issues.

Inconsistent implementations for calculating aspect ratios
5 participants
@Olle-Lukowski @alice-i-cecile @100-TomatoJuice @rparrett @JMS55