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 AnnularSector primitive shape #17928

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

RJ
Copy link
Contributor

@RJ RJ commented Feb 18, 2025

Adds an AnnularSector primitive 2d shape.

(a pie-slice of an annulus, which is the 2d-donut)

Solution

  • Based on the existing Annulus, to which i added a half_angle defined and oriented the same way as CircularSector.

Testing

  • Visually inspected the 2d shape and the extruded mesh
  • Added it to the 2d_shapes and 3d_shapes examples to inspect it.
  • Modified the 2d_shapes example to display shapes in two rows, since it was getting crowded.
  • TODO: I've not verified if AnnularSector::closest_point is correct, is there an example that visualizes this?

Showcase

2d_shapes example
Screenshot 2025-02-18 at 21 59 42

3d_shapes example
annular sector extrusion 3d_shapes

gratuitous screenshot
Screenshot 2025-02-18 at 21 05 52

@Jondolf
Copy link
Contributor

Jondolf commented Feb 18, 2025

My gut reaction is that this should probably not be its own primitive shape, but rather the AnnulusMeshBuilder should have a helper for limiting the sector angle for the generated mesh. A sector of an annulus feels a bit too niche to be considered a first-party geometric primitive, and I don't want to add more shapes for the sake of adding them; are there common practical use cases that would motivate this to be its own primitive?

(Personally, I kinda feel like the circular segment, circular sector, and rhombus types are already a bit too niche. The circular sector is also kind of annoying for collision detection and other geometric queries since it can be concave, like this annular sector.)

@Jondolf Jondolf added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Feb 18, 2025
@alice-i-cecile
Copy link
Member

Can you explain what you're hoping to use this for?

@rdrpenguin04
Copy link
Contributor

This might just be me, but this reminds me of the ARINC 661 widget "crown". It's essentially used as a wide arc in different parts of the heads-up display that show angles, particularly roll.

I'd personally vouch for this being generally useful, and specifically useful in that it can't be constructed with the other primitive shapes. Additionally, this could effectively be the primitive for circles, arcs, and annuluses/annuli, if such consolidation is needed.

@alice-i-cecile
Copy link
Member

Additionally, this could effectively be the primitive for circles, arcs, and annuluses/annuli, if such consolidation is needed.

We should avoid this consolidation in general for our primitives: these are often performance critical, and wasted memory will have measurable impacts.

@RJ
Copy link
Contributor Author

RJ commented Feb 19, 2025

I'm using an annular sector mesh for markers for things in stable circular orbits. I don't need the primitive shape for that, i just assemble the mesh. I added the primitive shape because I was copying the approach used by CircularSector.

I could change this to just be a feature of AnnulusMeshBuilder like @Jondolf suggested, and it would still be fine for my needs.

Seems a bit arbitrary to have a circular sector primitive but not an annular sector, given circle and annulus exist. Concave shapes being a pain for physics is a good reason though, too bad CircularSector already exists.

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 C-Feature A new feature, making something new possible S-Needs-Review Needs reviewer attention (from anyone!) to move forward
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants