Skip to content

Add Tetrahedron primitive to bevy_math::primitives #12688

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

Merged
merged 14 commits into from
Apr 1, 2024
Merged

Add Tetrahedron primitive to bevy_math::primitives #12688

merged 14 commits into from
Apr 1, 2024

Conversation

Chubercik
Copy link
Contributor

@Chubercik Chubercik commented Mar 24, 2024

Objective

There is no 3D primitive available for the common shape of a tetrahedron (3-simplex).

Solution

This PR introduces a new type to the existing math primitives:

  • Tetrahedron: a polyhedron composed of four triangular faces, six straight edges, and four vertices

Changelog

Added

  • Tetrahedron primitive to the bevy_math crate
  • Tetrahedron tests (area, volume methods)
  • impl_reflect! declaration for Tetrahedron in the bevy_reflect crate

Copy link
Contributor

Welcome, new contributor!

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

@alice-i-cecile alice-i-cecile added C-Feature A new feature, making something new possible A-Math Fundamental domain-agnostic mathematical operations M-Needs-Release-Note Work that should be called out in the blog due to impact labels Mar 24, 2024
@mweatherley
Copy link
Contributor

FYI, I have also already done a substantial amount of work on this; I would recommend joining us in the math-dev channel on Discord to talk about some of the things that have come up!

@Chubercik
Copy link
Contributor Author

Aw shucks, my bad 😅

I threw this PR draft out there after checking that there isn't anything that implements it in https://github.com/bevyengine/bevy/pulls, so that there is no doubling of efforts, but I guess I was too late anyway.

Should I close this PR?

@Chubercik Chubercik marked this pull request as ready for review March 24, 2024 23:55
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.

This is fairly barebones, but I'd be happy to have this as a baseline!

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.

Pretty happy with this! Thanks!

Copy link
Contributor

@Jondolf Jondolf left a comment

Choose a reason for hiding this comment

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

I think this looks good for an initial implementation :) We can build on this in follow-ups

@Jondolf Jondolf 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 Mar 26, 2024
Comment on lines +841 to +844
Vec3::new(0.0, 0.5, 0.0),
Vec3::new(-0.5, -0.5, 0.0),
Vec3::new(0.5, -0.5, 0.0),
Vec3::new(0.0, 0.0, 0.5),
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm really confused as to why this is the default tetrahedron. I would expect a regular tetrahedron, centered at the origin. Or at least one that contains the origin. This one satisfies none of those properties. Can we have it be this instead?

Suggested change
Vec3::new(0.0, 0.5, 0.0),
Vec3::new(-0.5, -0.5, 0.0),
Vec3::new(0.5, -0.5, 0.0),
Vec3::new(0.0, 0.0, 0.5),
Vec3::new(0.5, 0.5, 0.5),
Vec3::new(-0.5, -0.5, 0.5),
Vec3::new(0.5, -0.5, -0.5),
Vec3::new(-0.5, 0.5, -0.5),

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this was chosen so that, in particular, its base is the Default Triangle3d. I guess it does contain the origin, but only on its boundary :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

What @mweatherley said, in practice it's just Triangle3d base + an extra vertex on the Z-axis.

I was thinking, if we go with your suggestion would it be possible to keep the tetrahedron upright? I think that's how people typically imagine this shape to look like.


impl Default for Tetrahedron {
/// Returns the default [`Tetrahedron`] with the vertices
/// `[0.0, 0.5, 0.0]`, `[-0.5, -0.5, 0.0]`, `[0.5, -0.5, 0.0]` and `[0.0, 0.0, 0.5]`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// `[0.0, 0.5, 0.0]`, `[-0.5, -0.5, 0.0]`, `[0.5, -0.5, 0.0]` and `[0.0, 0.0, 0.5]`.
/// `[0.5, 0.5, 0.5]`, `[-0.5, -0.5, 0.5]`, `[0.5, -0.5, -0.5]` and `[-0.5, 0.5, -0.5]`.

impl Primitive3d for Tetrahedron {}

impl Default for Tetrahedron {
/// Returns the default [`Tetrahedron`] with the vertices
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
/// Returns the default [`Tetrahedron`] with the vertices
/// Returns a regular [`Tetrahedron`] centered at the origin with the vertices

Copy link
Contributor

@NthTensor NthTensor left a comment

Choose a reason for hiding this comment

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

I think the default tet should be centered on the origin. It's going to rotate strangely when people mesh it and apply transforms. I believe all other default primitives have their center of mass more or less at the origin.

@mweatherley
Copy link
Contributor

mweatherley commented Apr 1, 2024

I think the default tet should be centered on the origin. It's going to rotate strangely when people mesh it and apply transforms. I believe all other default primitives have their center of mass more or less at the origin.

I agree with this; there's obviously some tension in terms of making things match up, but that's less important than the ergonomics of actually using the shape. (Then again, I kind of think a lot of the default shapes are a little silly)

@Chubercik
Copy link
Contributor Author

I think the default tet should be centered on the origin.

I guess that decision is being lifted off my shoulders, thanks for all the reviews!

For the record, I agree, though I still feel like having this primitive remain upright (three vertices on a plane that's parallel to the XY plane, one vertex above) would be desirable, but would cause icky vertex coordinates (especially if going for a regular tetrahedron), which is why I was hesitant.

Merged via the queue into bevyengine:main with commit 20ee56e Apr 1, 2024
@Chubercik Chubercik deleted the tetrahedron-primitive branch April 1, 2024 22:24
chompaa pushed a commit to chompaa/bevy that referenced this pull request Apr 5, 2024
)

# Objective

- bevyengine#10572

There is no 3D primitive available for the common shape of a tetrahedron
(3-simplex).

## Solution

This PR introduces a new type to the existing math primitives:

- `Tetrahedron`: a polyhedron composed of four triangular faces, six
straight edges, and four vertices

---

## Changelog

### Added

- `Tetrahedron` primitive to the `bevy_math` crate
- `Tetrahedron` tests (`area`, `volume` methods)
- `impl_reflect!` declaration for `Tetrahedron` in the `bevy_reflect`
crate
@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#1295 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
Labels
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
Development

Successfully merging this pull request may close these issues.

6 participants