Skip to content

Add Annulus primitive to bevy_math::primitives#12706

Merged
alice-i-cecile merged 7 commits intobevyengine:mainfrom
Chubercik:annulus-primitive
Mar 25, 2024
Merged

Add Annulus primitive to bevy_math::primitives#12706
alice-i-cecile merged 7 commits intobevyengine:mainfrom
Chubercik:annulus-primitive

Conversation

@Chubercik
Copy link
Copy Markdown
Contributor

@Chubercik Chubercik commented Mar 25, 2024 •
edited
Loading

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)

alice-i-cecile
alice-i-cecile reviewed Mar 25, 2024
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Mar 25, 2024
View reviewed changes
Copy link
Copy Markdown
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!

@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
pablo-lua
pablo-lua reviewed Mar 25, 2024
View reviewed changes
Copy link
Copy Markdown
Contributor

@pablo-lua pablo-lua 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 we can implement Meshable for this? Not sure what are the requirements.
The overall looks good!

Jondolf
Jondolf suggested changes Mar 25, 2024
View reviewed changes
Copy link
Copy Markdown
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.

Jondolf
Jondolf reviewed Mar 25, 2024
View reviewed changes
@Chubercik
Copy link
Copy Markdown
Contributor Author

Chubercik commented Mar 25, 2024

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.).

Jondolf
Jondolf approved these changes Mar 25, 2024
View reviewed changes
Copy link
Copy Markdown
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
Copy Markdown
Contributor Author

Chubercik commented Mar 25, 2024

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
Copy Markdown
Contributor

pablo-lua commented Mar 25, 2024 •
edited
Loading

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
@Jondolf Jondolf mentioned this pull request Mar 25, 2024
Open
41 tasks
@Chubercik Chubercik deleted the annulus-primitive branch March 25, 2024 23:52
@Chubercik Chubercik mentioned this pull request Mar 27, 2024
Merged
github-merge-queue bot pushed a commit that referenced this pull request Apr 2, 2024
@Chubercik
Add the annulus shape to the 2d_shapes example (#12742)
017ffc5 
# 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
@Chubercik @chompaa
Add the annulus shape to the 2d_shapes example (bevyengine#12742)
4824a3f 
# 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..)
@13ros27 13ros27 mentioned this pull request May 10, 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

+2 more reviewers

@pablo-lua pablo-lua pablo-lua left review comments

@Jondolf Jondolf Jondolf approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

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

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@Chubercik @pablo-lua @alice-i-cecile @Jondolf