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

Added CompassQuadrant and CompassOctant as per #13647 #13653

Merged
merged 9 commits into from
Jun 3, 2024
Merged

Added CompassQuadrant and CompassOctant as per #13647 #13653

merged 9 commits into from
Jun 3, 2024

Conversation

BobG1983
Copy link
Contributor

@BobG1983 BobG1983 commented Jun 3, 2024

Objective

Implements #13647

Solution

Created two enums, CompassQuadrant and CompassOctant inside compass.rs with impls To and From Dir2. Used dir.to_angle().to_degrees() and matched against the resulting value. I could have skipped to_degrees() and matched against the radian value, but I thought this was more readable. I'm probably wrong lol.

Testing

Tested various dirs to compass variations.

@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Jun 3, 2024

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@BobG1983 BobG1983 changed the title Added Compass and CompassExtent as per #13647 Added CompassQuadrant and CompassOctant as per #13647 Jun 3, 2024
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Input Player input via keyboard, mouse, gamepad, and more A-Math Fundamental domain-agnostic mathematical operations M-Needs-Release-Note Work that should be called out in the blog due to impact S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jun 3, 2024
alice-i-cecile
alice-i-cecile approved these changes Jun 3, 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.

Perfect, this is exactly what I had in mind :) Very clean and useful. I like the degrees here: I do think they make the code easier to read.

@alice-i-cecile alice-i-cecile added D-Trivial Nice and easy! A great choice to get started with Bevy X-Contentious There are nontrivial implications that should be thought through labels Jun 3, 2024
BobG1983 and others added 2 commits June 3, 2024 10:50
@BobG1983 @alice-i-cecile
Update crates/bevy_math/src/compass.rs
6f1de27 
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@BobG1983
Added some comments as per suggestion
949aa95
jnhyatt
jnhyatt approved these changes Jun 3, 2024
View reviewed changes
Copy link
Contributor

@jnhyatt jnhyatt left a comment

Choose a reason for hiding this comment

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

Nothing blocking, but consider some quick fixes (the comments I added about docs are the most important to me)

crates/bevy_math/src/compass.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/compass.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/compass.rs Outdated Show resolved Hide resolved
@alice-i-cecile alice-i-cecile 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 Jun 3, 2024
@BD103 BD103 added S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged and removed S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it labels Jun 3, 2024
BD103
BD103 approved these changes Jun 3, 2024
View reviewed changes
Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

Thank you!

@alice-i-cecile alice-i-cecile 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-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Jun 3, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jun 3, 2024
Merged via the queue into bevyengine:main with commit 0db9fc9 Jun 3, 2024
28 checks passed
@alice-i-cecile alice-i-cecile mentioned this pull request Jun 3, 2024
Closed
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1332 if you'd like to help out.

@bugsweeper bugsweeper mentioned this pull request Jun 4, 2024
Closed
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

@jnhyatt jnhyatt jnhyatt approved these changes

@BD103 BD103 BD103 approved these changes

Assignees
No one assigned
Labels
A-Input Player input via keyboard, mouse, gamepad, and more A-Math Fundamental domain-agnostic mathematical operations C-Feature A new feature, making something new possible D-Trivial Nice and easy! A great choice to get started with Bevy M-Needs-Release-Note Work that should be called out in the blog due to impact S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@BobG1983 @alice-i-cecile @jnhyatt @BD103