Skip to content

Extrusion#13270

Merged
alice-i-cecile merged 9 commits intobevyengine:mainfrom
lynn-lumen:extrusion
May 7, 2024
Merged

Extrusion#13270
alice-i-cecile merged 9 commits intobevyengine:mainfrom
lynn-lumen:extrusion

Conversation

@lynn-lumen
Copy link
Copy Markdown
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 reviewed May 7, 2024
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed May 7, 2024
View reviewed changes
alice-i-cecile
alice-i-cecile approved these changes May 7, 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 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-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
Copy Markdown
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.

@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
@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
Copy Markdown
Member

alice-i-cecile commented Jun 3, 2024

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

lynn-lumen commented Jun 3, 2024

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

+1 more reviewer

@mweatherley mweatherley mweatherley approved these changes

Reviewers whose approvals may not affect merge requirements

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