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

Add common aspect ratio constants and improve documentation #15091

Merged
merged 14 commits into from
Sep 9, 2024
Merged

Add common aspect ratio constants and improve documentation #15091

merged 14 commits into from
Sep 9, 2024

Conversation

miniex
Copy link
Contributor

@miniex miniex commented Sep 8, 2024

Hello,

I'd like to contribute to this project by adding some useful constants and improving the documentation for the AspectRatio struct. Here's a summary of the changes I've made:

  1. Added new constants for common aspect ratios:

    • SIXTEEN_NINE (16:9)
    • FOUR_THREE (4:3)
    • ULTRAWIDE (21:9)

  2. Enhanced the overall documentation:

    • Improved module-level documentation with an overview and use cases
    • Expanded explanation of the AspectRatio struct with examples
    • Added detailed descriptions and examples for all methods (both existing and new)
    • Included explanations for the newly introduced constant values
    • Added clarifications for From trait implementations

These changes aim to make the AspectRatio API more user-friendly and easier to understand. The new constants provide convenient access to commonly used aspect ratios, which I believe will be helpful in many scenarios.

DasLixou
DasLixou reviewed Sep 8, 2024
View reviewed changes
crates/bevy_math/src/aspect_ratio.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/aspect_ratio.rs Outdated Show resolved Hide resolved
@miniex
Add common aspect ratio constants and improve documentation
87383ee 
- Add new constants for common aspect ratios: SIXTEEN_NINE, FOUR_THREE, ULTRAWIDE
- Enhance module-level documentation with overview and use cases
- Expand explanation of AspectRatio struct with examples
- Provide detailed descriptions and examples for all methods (existing and new)
- Add explanations for the newly introduced constant values
- Include clarifications for From trait implementations

This commit introduces convenient constants for frequently used aspect ratios
and significantly improves the overall documentation, enhancing the usability
and understanding of the AspectRatio API.
@miniex miniex requested a review from DasLixou September 8, 2024 07:40
@miniex @Bluefinger
Update crates/bevy_math/src/aspect_ratio.rs
5e7ba86 
Co-authored-by: Gonçalo Rica Pais da Silva <bluefinger@gmail.com>
@IQuick143 IQuick143 added C-Docs An addition or correction to our documentation A-Rendering Drawing game state to the screen C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 8, 2024
miniex and others added 2 commits September 8, 2024 19:40
@miniex @DasLixou
Update mod.rs
4c9bc67 
Co-authored-by: Lixou <82600264+DasLixou@users.noreply.github.com>
@miniex
EDIT: updated with better wording/grammar.
ef94bb0
alice-i-cecile
alice-i-cecile reviewed Sep 8, 2024
View reviewed changes
crates/bevy_math/src/aspect_ratio/mod.rs Outdated
/// An `AspectRatio` is the ratio of width to height.
#[derive(Copy, Clone, Debug, PartialEq, PartialOrd)]
#[cfg_attr(feature = "bevy_reflect", derive(Reflect), reflect(Debug, PartialEq))]
pub struct AspectRatio(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 think we're missing a getter for the internal field here. You can call .into, but that's pretty obscure.

Copy link
Contributor Author

@miniex miniex Sep 9, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Should the getter be named as_f32 or ratio?
I'm thinking about this, ^-^

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In my opinion, ratio is more appropriate.

alice-i-cecile
alice-i-cecile requested changes Sep 8, 2024
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

I really like the thrust of this: helpful error types are excellent, and I like the utility methods you've added. I have a few relatively minor requests.

@alice-i-cecile alice-i-cecile added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 8, 2024
@janhohenheim
Copy link
Member

There's some potential overlap with #14158, FYI

@janhohenheim janhohenheim added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Sep 9, 2024
@miniex
Copy link
Contributor Author

miniex commented Sep 9, 2024

Is there anything else we should do to avoid overlap with issue #14158 ?

@janhohenheim janhohenheim added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Sep 9, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Sep 9, 2024
@alice-i-cecile alice-i-cecile mentioned this pull request Sep 9, 2024
Closed
Merged via the queue into bevyengine:main with commit 29c632b Sep 9, 2024
27 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Bluefinger Bluefinger Bluefinger approved these changes

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

@DasLixou DasLixou DasLixou approved these changes

Assignees
No one assigned
Labels
A-Rendering Drawing game state to the screen C-Docs An addition or correction to our documentation C-Usability A targeted quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples 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.
6 participants
@miniex @janhohenheim @Bluefinger @alice-i-cecile @DasLixou @IQuick143