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

New circular primitives: Arc2d, CircularSector, CircularSegment #13482

Merged
merged 23 commits into from
May 23, 2024
Merged

New circular primitives: Arc2d, CircularSector, CircularSegment #13482

merged 23 commits into from
May 23, 2024

Conversation

aristaeus
Copy link
Contributor

@aristaeus aristaeus commented May 23, 2024 •
edited
Loading

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.

spectria-limina and others added 20 commits May 23, 2024 18:11
@spectria-limina
Add Arc and CircularSector math primitives.
8813322
@spectria-limina
Add CircularSegment primitve as well.
d766ccb
@spectria-limina
Fix doc errors.
7b9b17b
@spectria-limina
Review feedback and other minor improvements.
6d92312
@spectria-limina
Change the orientation of arc primitives to vertically symmetrical.
045eaac
@spectria-limina
Fix Circular{Sector,Segment} UV-mapping and add an example.
e628cd5 
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.
@spectria-limina
Allow the apothem to be negative to simplify much of the math.
fb5011a
@spectria-limina
Some more small cleanups.
b24a097 
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.
@spectria-limina
Update examples page.
dea8e26
@spectria-limina
Rename CircularShapeUvMode to CircularMeshUvMode.
80f55a8
@spectria-limina
Add tests to primitive math.
fad57dd
@spectria-limina
Add tests for arc bounding shapes.
98f0922 
There were bugs.
@spectria-limina
Add tests to bounding shapes for CircularSector.
794a611 
Also draw bounding boxes on the mesh example to visually confirm
correctness.
@spectria-limina
Silence clippy and CI.
6de1c72 
Every suggestion for the boolean condition is less readable.
@spectria-limina @Jondolf @alice-i-cecile
Apply suggestions from code review
46e186b 
Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@spectria-limina
Review changes
b73dd0d
@spectria-limina
More review comments
7e10ab9
@spectria-limina
Fixups for review & compile fixes
feda843
@spectria-limina
Fix failing build stuff
8750c06
final fixes after rebasing on main
a077d52
@github-actions GitHub Actions
Copy link
Contributor

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.

@mweatherley
Copy link
Contributor

Thanks for adopting this!

@aristaeus aristaeus marked this pull request as ready for review May 23, 2024 14:59
@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations A-Gizmos Visual editor and debug gizmos S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels May 23, 2024
@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label May 23, 2024
alice-i-cecile
alice-i-cecile approved these changes May 23, 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.

Excellent, thanks for tackling this :) I'm happy with the state of this now, and pleased to see this picked up.

@alice-i-cecile alice-i-cecile added this to the 0.14 milestone May 23, 2024
mweatherley
mweatherley approved these changes May 23, 2024
View reviewed changes
Copy link
Contributor

@mweatherley mweatherley left a comment

Choose a reason for hiding this comment

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

Gave this a good look before, and it looks like it works as expected! Thanks again!

@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 May 23, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 23, 2024
Merged via the queue into bevyengine:main with commit ec01c2d May 23, 2024
37 checks passed
@Jondolf Jondolf mentioned this pull request May 25, 2024
Open
46 tasks
@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#1325 if you'd like to help out.

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

@mweatherley mweatherley mweatherley approved these changes

@Jondolf Jondolf Awaiting requested review from Jondolf

Assignees
No one assigned
Labels
A-Gizmos Visual editor and debug gizmos A-Math Fundamental domain-agnostic mathematical operations C-Feature A new feature, making something new possible 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
Projects
None yet
Milestone
0.14
Development

Successfully merging this pull request may close these issues.
4 participants
@aristaeus @mweatherley @alice-i-cecile @spectria-limina