New circular primitives: `Arc2d`, `CircularSector`, `CircularSegment` by spectria-limina · Pull Request #11748 · bevyengine/bevy

spectria-limina · 2024-02-06T22:18:20Z

Objective

There are no 2d primitives available for the common shapes of an arc or a circular sector (pie slice).

Solution

This PR introduces three new types to the existing math primitives:

Arc2d: a portion of a (hollow) circle
CircularSector: the area between an arc and the centre of the circle
CircularSegment: the convex hull of an arc

Run cargo run --example 2d_shapes to see examples of the CircularSector and CircularSegment type.

Design decisions

What should the arc type be named?

Answer: Arc2d, because Arc is already taken by the standard library.
What should the origin point of these shapes be?
- The origin point is the point around which the shape rotates, among other things.
- For an arc and a circular sector, it's intuitive I think that the origin be the center of the corresponding circle.
- But for a circular segment, I think the origin being the center of the chord is more intuitive, because it is easier to reason about rotation math that way.
- On the other hand, consistency between the three shapes has value.
Answer: This PR puts the origin of all three shapes in the center, prioritizing consistency,
What should the UV-mapping of CircularSector and CircularSegment look like? There are two reasonable options:
1. Treat the shapes as masks of the corresponding circle. Good for animating the texture, or cutting a single circular texture into pieces e.g. pizza slices.
2. Map the shape's bounding box onto the texture. Good when the slice size is fixed and not going to change, to be maximally efficient with the texture space.
Answer: This PR chooses option 1 as the default, but puts UV-mapping configuration in a CircularMeshUvMode enum, as futureproofing for later adding option 2.
How should the angle of these shapes be expressed?
1. Follow mathematical conventions of starting from the right and going counterclockwise.
2. Start from the top and extend by half the angle on either side.
Answer: This PR chooses option 2, because it leads to better defaults and better ergonomics when reasoning about how to rotate the shape.

TODO

Add convenience methods on CircularSector and CircularSegment that avoid needing to explicitly access the arc field.
Add examples that make use of textures, to verify that the UV mappings are correct.
Add testing, particularly of the shape math. (I've already caught at least one bug and there are probably more.)

Future Work

For after this PR is merged:

Add support for these shapes to Gizmos as in Drawing Primitives with Gizmos #11072.
Add support for the alternative UV mode discussed above, as a configurable option on the MeshBuilder types.
Integrate these shapes into the new math/primitives example.

Changelog

Added Arc2d, CircularSector, and CircularSegment primitives to [bevy::math::primitives].

github-actions · 2024-02-06T22:18:34Z

Welcome, new contributor!

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

Jondolf

I didn't take a super in-depth look at all of the math and the bounding implementation yet, but everything seems correct at a glance. Code quality overall is great!

I mainly left some documentation and naming suggestions and typo fixes.

As for the unresolved questions:

I think the origin should probably be at the circle's center for all three shapes for the sake of consistency. I can't think of another origin that would make much sence, except maybe for the circular segment, but I think I prefer consistency here. Whatever is chosen, the important thing is that it is documented (and it already is).
Option 1 (the current one) is a good default. We can add option 2 as a configuration option in a follow-up.
I agree, a half-angle in radians extending symmetrically in both directions from the vertical (i.e. the "top" of the circle) might be more intuitive and easier to work with. Starting from the right and going counterclockwise might be more conventional mathematically (though I'm not sure if arcs specifically have a convention for this), but I feel like that wouldn't be a very useful default from a practical standpoint. I don't have a strong opinion on this though, so other peoples' thoughts on the topic would be useful.

crates/bevy_math/src/primitives/dim2.rs

examples/2d/2d_shapes.rs

crates/bevy_math/src/primitives/dim2.rs

spectria-limina · 2024-02-13T05:09:19Z

Thanks for the review! I've made the changes plus a few more things. I'll update the first comment to reflect the current state of things.

github-actions · 2024-02-14T21:40:50Z

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

This required adding a parameterizable angle for the UV mapping. It's in an enum to reduce the amount of churn if/when we add additional modes.

Change the default angle to a third of a circle, because a quarter leads to too thin of a segment. Also formatting changes in 2d_shapes.rs.

There were bugs.

crates/bevy_render/src/mesh/primitives/dim2.rs

Jondolf

I love the tests and overall code quality, and the examples looks great. Large PR though!

I left a ton of tweaks and suggestions regarding names, docs, and some other things. I tried to be pretty thorough, so apologies if some things are overly nitpicky :)

crates/bevy_math/src/bounding/bounded2d/primitive_impls.rs

crates/bevy_render/src/mesh/primitives/dim2.rs

examples/2d/2d_shapes.rs

Cargo.toml

examples/2d/mesh2d_circular.rs

Co-authored-by: Joona Aalto <jondolf.dev@gmail.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

github-actions · 2024-03-09T09:18:15Z

You added a new example but didn't add metadata for it. Please update the root Cargo.toml file.

Jondolf

Alright, once the example metadata is fixed, I think this LGTM! Thanks for the patience :)

examples/README.md

Cargo.toml

spectria-limina · 2024-03-26T08:52:37Z

I've now updated the PR and got it as close to working as I can, but #12729 is preventing all tests from passing and I can't figure out a fix locally.

github-actions · 2024-03-26T09:01:19Z

The generated examples/README.md is out of sync with the example metadata in Cargo.toml or the example readme template. Please run cargo run -p build-templated-pages -- update examples to update it, and commit the file change.

spectria-limina · 2024-03-26T09:02:17Z

There also seems to be a failing wasm build that isn't related to my PR.

bugsweeper · 2024-03-26T15:08:19Z

There also seems to be a failing wasm build that isn't related to my PR.

That has been fixed #12730

mweatherley

I have a couple of minor nits, but I would like for this to get merged. I wish I had realized this needed another review earlier; I'm sorry that this PR has been neglected somewhat.

Overall, I am very happy with the documentation, testing, and examples.

mweatherley · 2024-04-27T11:06:02Z

examples/2d/mesh2d_arcs.rs

+
+        let segment = CircularSegment::from_turns(40.0, fraction);
+        // For the circular segment, we will draw Bevy charging forward, which requires rotating the
+        // shape and texture by 90 degrees.


I don't really get what this means, since to me it just looks like Bevy is upside-down in the CircularSegment part of the example.

mweatherley · 2024-04-27T11:16:31Z

crates/bevy_math/src/primitives/dim2.rs

+        }
+    }
+
+    /// Create a new [`Arc2d`] from a `radius` and a number of `turns` of a circle.


Suggested change

/// Create a new [`Arc2d`] from a `radius` and a number of `turns` of a circle.

/// Create a new [`Arc2d`] from a `radius` and a `fraction` of a single turn.

This is a great convenience API; it looks like the documentation is just slightly out of date.

alice-i-cecile · 2024-04-30T00:46:02Z

@spectria-limina I'm happy to merge this as is: let me know if you're able to resolve merge conflicts?

…#13482) # Objective Adopted #11748 ## Solution I've rebased on main to fix the merge conflicts. ~~Not quite ready to merge yet~~ * Clippy is happy and the tests are passing, but... * ~~The new shapes in `examples/2d/2d_shapes.rs` don't look right at all~~ Never mind, looks like radians and degrees just got mixed up at some point? * I have updated one doc comment based on a review in the original PR. --------- Co-authored-by: Alexis "spectria" Horizon <spectria.limina@gmail.com> Co-authored-by: Alexis "spectria" Horizon <118812919+spectria-limina@users.noreply.github.com> Co-authored-by: Joona Aalto <jondolf.dev@gmail.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com> Co-authored-by: Ben Harper <ben@tukom.org>

alice-i-cecile added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations A-Rendering Drawing game state to the screen labels Feb 6, 2024

spectria-limina force-pushed the arc branch 3 times, most recently from 2179bba to 271f299 Compare February 7, 2024 17:45

Jondolf reviewed Feb 12, 2024

View reviewed changes

crates/bevy_math/src/primitives/dim2.rs Outdated Show resolved Hide resolved

spectria-limina changed the title ~~New circular primitives: Arc, CircularSector, CircularSegment~~ New circular primitives: Arc2d, CircularSector, CircularSegment Feb 13, 2024

spectria-limina force-pushed the arc branch from 4c771c7 to c128ef0 Compare February 13, 2024 07:23

spectria-limina added 10 commits February 14, 2024 21:00

Add Arc and CircularSector math primitives.

f637de3

Add CircularSegment primitve as well.

9b88d45

Fix doc errors.

c36edb5

Review feedback and other minor improvements.

1947ac0

Change the orientation of arc primitives to vertically symmetrical.

33e2dd2

Fix Circular{Sector,Segment} UV-mapping and add an example.

971f9b0

This required adding a parameterizable angle for the UV mapping. It's in an enum to reduce the amount of churn if/when we add additional modes.

Allow the apothem to be negative to simplify much of the math.

609d238

Some more small cleanups.

faabb28

Change the default angle to a third of a circle, because a quarter leads to too thin of a segment. Also formatting changes in 2d_shapes.rs.

Update examples page.

41806b7

Rename CircularShapeUvMode to CircularMeshUvMode.

9240e7e

spectria-limina force-pushed the arc branch 4 times, most recently from 668137d to 5847618 Compare February 16, 2024 00:27

Add tests to primitive math.

ac33ec6

spectria-limina force-pushed the arc branch from 5847618 to ac33ec6 Compare February 16, 2024 03:43

Add tests for arc bounding shapes.

a4fc07f

There were bugs.

alice-i-cecile reviewed Feb 29, 2024

View reviewed changes

crates/bevy_render/src/mesh/primitives/dim2.rs Show resolved Hide resolved

Jondolf suggested changes Feb 29, 2024

View reviewed changes

Jondolf mentioned this pull request Mar 2, 2024

Tracking Issue: Primitive Shapes #10572

Open

41 tasks

spectria-limina and others added 3 commits March 9, 2024 03:35

Apply suggestions from code review

552aea5

Co-authored-by: Joona Aalto <jondolf.dev@gmail.com> Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>

Merge branch 'main' into arc

31b2be3

Review changes

9353ad0

spectria-limina force-pushed the arc branch from 4a1d0dd to 9353ad0 Compare March 9, 2024 09:06

Jondolf approved these changes Mar 15, 2024

View reviewed changes

examples/README.md Outdated Show resolved Hide resolved

Cargo.toml Outdated Show resolved Hide resolved

spectria-limina added 4 commits March 24, 2024 17:54

More review comments

1c31b1a

Merge remote-tracking branch 'origin/main' into arc

4cd5957

Merge remote-tracking branch 'origin/main' into arc

2e0740c

Fixups for review & compile fixes

fb29d40

Fix failing build stuff

1eaf343

Jondolf approved these changes Mar 26, 2024

View reviewed changes

mweatherley approved these changes Apr 27, 2024

View reviewed changes

alice-i-cecile 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 Apr 29, 2024

alice-i-cecile added the S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged label May 2, 2024

alice-i-cecile added S-Adopt-Me The original PR author has no intent to complete this work. Pick me up! and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels May 13, 2024

alice-i-cecile removed this from the 0.14 milestone May 13, 2024

aristaeus mentioned this pull request May 23, 2024

New circular primitives: Arc2d, CircularSector, CircularSegment #13482

Merged

spectria-limina closed this May 25, 2024

spectria-limina deleted the arc branch May 25, 2024 07:05

	/// Create a new [`Arc2d`] from a `radius` and a number of `turns` of a circle.
	/// Create a new [`Arc2d`] from a `radius` and a `fraction` of a single turn.

Uh oh!

Conversation

spectria-limina commented Feb 6, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Objective

Solution

Design decisions

TODO

Future Work

Changelog

Uh oh!

github-actions bot commented Feb 6, 2024

Uh oh!

Jondolf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

spectria-limina commented Feb 13, 2024

Uh oh!

github-actions bot commented Feb 14, 2024

Uh oh!

Uh oh!

Jondolf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

github-actions bot commented Mar 9, 2024

Uh oh!

Jondolf left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

spectria-limina commented Mar 26, 2024

Uh oh!

github-actions bot commented Mar 26, 2024

Uh oh!

spectria-limina commented Mar 26, 2024

Uh oh!

bugsweeper commented Mar 26, 2024

Uh oh!

mweatherley left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

mweatherley Apr 27, 2024

Choose a reason for hiding this comment

Uh oh!

mweatherley Apr 27, 2024

Choose a reason for hiding this comment

Uh oh!

alice-i-cecile commented Apr 30, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

spectria-limina commented Feb 6, 2024 •

edited

Loading

mweatherley left a comment •

edited

Loading