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

Extrusion #13270

Merged
merged 9 commits into from
May 7, 2024
Merged

Extrusion #13270

merged 9 commits into from
May 7, 2024

Conversation

lynn-lumen
Copy link
Contributor

@lynn-lumen lynn-lumen commented May 7, 2024 •
edited
Loading

Objective

Solution

  • Adds Measured2d and Measured3d traits for getting the perimeter/area or area/volume of shapes. This allows implementing .volume() and .area() for all extrusions Extrusion<T: Primitive2d + Measured2d> within bevy_math
  • All existing perimeter, area and volume implementations for primitves have been moved into implementations of Measured2d and Measured3d
  • Shapes should be extruded along the Z-axis since an extrusion of depth 0. should be equivalent in everything but name to the base shape

Caviats

  • I am not sure about the naming. Extrusion<T> could also be Prism<T> and the MeasuredNd could also be something like MeasuredPrimitiveNd. If you have any other suggestions, please fell free to share them :)

Future work

This PR adds a basic Extrusion shape and does not implement a lot of things you might want it to. Some of the future possibilities include:

  • bounding for extrusions
  • making extrusions work with gizmos
  • meshing

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations X-Uncontroversial This work is generally agreed upon D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Contentious There are nontrivial implications that should be thought through and removed X-Uncontroversial This work is generally agreed upon labels May 7, 2024
alice-i-cecile
alice-i-cecile approved these changes May 7, 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.

Some small doc improvements, but I'm fundamentally on board with this. I really like how straightforward and clean the extrusion type is, and the new traits are a very natural abstraction.

@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 7, 2024
@alice-i-cecile alice-i-cecile mentioned this pull request May 7, 2024
Closed
@lynn-lumen @alice-i-cecile
Improve docs
c541aff 
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
mweatherley
mweatherley approved these changes May 7, 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.

Looks good! My only misgiving with this is that the way that traits work makes some of the docs worse, but there's not really anything you can do about that.

crates/bevy_math/src/primitives/mod.rs Show resolved Hide resolved
@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 7, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 7, 2024
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks May 7, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue May 7, 2024
Merged via the queue into bevyengine:main with commit 03f4cc5 May 7, 2024
28 checks passed
@Jondolf Jondolf mentioned this pull request May 25, 2024
Open
41 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#1312 if you'd like to help out.

@lynn-lumen
Copy link
Contributor Author

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#1312 if you'd like to help out.

I think it would make a lot of sense to merge either #13478 or #13346 first, because as it stands you really cannot do anything with extrusions

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

Assignees
No one assigned
Labels
A-Math Fundamental domain-agnostic mathematical operations C-Feature A new feature, making something new possible D-Modest A "normal" level of difficulty; suitable for simple features or challenging fixes 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 X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lynn-lumen @alice-i-cecile @mweatherley