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.

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

Add Annulus primitive to bevy_math::primitives #12706

Merged
merged 7 commits into from
Mar 25, 2024
Merged

Add Annulus primitive to bevy_math::primitives #12706

merged 7 commits into from
Mar 25, 2024

Conversation

Chubercik
Copy link
Contributor

@Chubercik Chubercik commented Mar 25, 2024

Objective

There is no 2D primitive available for the common shape of an annulus (ring).

Solution

This PR introduces a new type to the existing math primitives:

  • Annulus: the region between two concentric circles

Changelog

Added

  • Annulus primitive to the bevy_math crate
  • Annulus tests (diameter, thickness, area, perimeter and closest_point methods)

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.

Some feedback on documentation :) Useful shape though!

crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved
@pablo-lua pablo-lua added C-Feature A new feature, making something new possible A-Rendering Drawing game state to the screen A-Math Fundamental domain-agnostic mathematical operations labels Mar 25, 2024
Copy link
Contributor

@pablo-lua pablo-lua left a comment

Choose a reason for hiding this comment

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

I think we can implement Meshable for this? Not sure what are the requirements.
The overall looks good!

Copy link
Contributor

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

I like this! Left some suggestions though :)

I think we can leave meshing, gizmos, and bounding volumes for follow-ups. I don't see a reason to block on them, and adding everything at once can add friction to reviews, as we've seen with some other primitives.

crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved
@Chubercik
Copy link
Contributor Author

Alright, every comment should've been accounted for :)

As of right now, I don't really see the reason for using Circles (only the diameter() method got something out of it), but I assume this will come in handy when we start adding more stuff onto the Annulus struct (e.g. meshing, gizmos, bounding, etc.).

Copy link
Contributor

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

I'm somewhat deliberating if we should call this Annulus or Ring; the latter would be more approachable and perhaps more obvious, but could have some ambiguities (2D or 3D?), while Annulus is more mathematically accurate and unambiguously 2D.

I'm fine with either name, and Ring is already a doc alias here so I think Annulus should be fine.

As of right now, I don't really see the reason for using Circles (only the diameter() method got something out of it)

Yeah it didn't end up being very useful here specifically, but even just for users it could be nice to have methods for getting the diameter/area/perimeter of the inner and outer circle. We could add these methods to Annulus directly, but I generally prefer reusing the existing primitives for things like this (where reasonable).

@Jondolf Jondolf 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 Mar 25, 2024
@Chubercik
Copy link
Contributor Author

I went with Annulus instead of Ring by default after googling "ring", "math ring", "geometry ring" - almost all resulting images are those of jewellery, so I thought there was a preconceived notion that a ring is understood commonly as a 3D object (a torus, perhaps).

(searching for "ring math" and "ring geometry" is an improvement, but there is still finger bling to be found here and there 😁)

@pablo-lua
Copy link
Contributor

pablo-lua commented Mar 25, 2024

TBH, After I learned the math explanation, I found Annulus more fit. At the same time, I didn't understand it at first. Maybe we can have a link to a wikipedia explanation or something.
(But I don't know if that's needed, after all, we say that its basically a ring but 2d)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 25, 2024
Merged via the queue into bevyengine:main with commit 31d9146 Mar 25, 2024
26 of 28 checks passed
@Jondolf Jondolf mentioned this pull request Mar 25, 2024
46 tasks
@Chubercik Chubercik deleted the annulus-primitive branch March 25, 2024 23:52
github-merge-queue bot pushed a commit that referenced this pull request Apr 2, 2024
# Objective

- Depends on #12734.

Since adding the `Annulus` primitive shape (#12706, #12734), the
`2d_shapes` example has become outdated.

## Solution

This PR adds the annulus shape to the `2d_shapes` example:


![image](https://github.com/bevyengine/bevy/assets/37378746/e620362d-bec6-4660-bf6e-d70babff8179)

---

## Changelog

### Added

- `Annulus` shape to the `2d_shapes` example

(~~as an added bonus, the example now features Newton's
[ROYGBIV](https://en.wikipedia.org/wiki/ROYGBIV) rainbow palette ^^~~ no
it doesn't, but one can shoehorn..)
chompaa pushed a commit to chompaa/bevy that referenced this pull request Apr 5, 2024
# Objective

- Depends on bevyengine#12734.

Since adding the `Annulus` primitive shape (bevyengine#12706, bevyengine#12734), the
`2d_shapes` example has become outdated.

## Solution

This PR adds the annulus shape to the `2d_shapes` example:


![image](https://github.com/bevyengine/bevy/assets/37378746/e620362d-bec6-4660-bf6e-d70babff8179)

---

## Changelog

### Added

- `Annulus` shape to the `2d_shapes` example

(~~as an added bonus, the example now features Newton's
[ROYGBIV](https://en.wikipedia.org/wiki/ROYGBIV) rainbow palette ^^~~ no
it doesn't, but one can shoehorn..)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Math Fundamental domain-agnostic mathematical operations A-Rendering Drawing game state to the screen C-Feature A new feature, making something new possible 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
Development

Successfully merging this pull request may close these issues.

4 participants