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.

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

Add Triangle3d primitive to bevy_math::primitives #12508

Merged
merged 32 commits into from
Mar 22, 2024

Conversation

vitorfhc
Copy link
Contributor

@vitorfhc vitorfhc commented Mar 16, 2024

Context

GitHub Discussion Link

Objective

  • Clarity: More explicit representation of a common geometric primitive.
  • Convenience: Provide methods tailored to 3D triangles (area, perimeters, etc.).

Solution

  • Adding the Triangle3d primitive into the bevy_math crate.

Changelog

Added

  • Triangle3d primitive to the bevy_math crate

Changed

  • Triangle2d::reverse: the first and last vertices are swapped instead of the second and third.

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 ✨

@vitorfhc
Copy link
Contributor Author

@alice-i-cecile I opened the Triangle3d PR using my personal Github account.

I opened it as a draft so we can discuss a few things:

  1. What methods would be useful to implement?
  2. Should I check for invalid triangles (e.g. point A is in the BC segment)?

@NthTensor
Copy link
Contributor

I like this, nice work. Can we get a function for the normal as well?

@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 16, 2024
@Jondolf
Copy link
Contributor

Jondolf commented Mar 16, 2024

For feature-parity with Triangle2d, we'd need:

  • reverse to reverse the winding order (same as Triangle2d::reverse)
  • circumsphere (same as Triangle2d::circumcircle, but return a Sphere)
  • Bounded3d implementation (mostly a copy of the 2D version I think)
  • Meshing support
  • Primitive gizmo support

Other nice to have things:

  • center/centroid (doesn't exist on Triangle2d for some reason though)
  • normal

I think normal would be this:

/// Get the normal of the triangle in the direction of `AB x AC`, where `x` denotes the cross product.
///
/// If the triangle vertices are non-finite or lie on the same line, `Err(InvalidDirectionError)` is returned.
#[inline(always)]
pub fn normal(&self) -> Result<Dir3, InvalidDirectionError> {
    let ab = self.vertices[1] - self.vertices[0];
    let ac = self.vertices[2] - self.vertices[0];
    Dir3::new(ab.cross(ac))
}

@Jondolf
Copy link
Contributor

Jondolf commented Mar 16, 2024

Side note: It'd kinda be nice if we used named properties a, b, and c instead of an array of vertices. It'd make it nicer to refer to specific vertices in docs and make methods like ab, bc, and ac sensible. This is probably out of scope for this PR though since it also applies to Triangle2d.

@vitorfhc
Copy link
Contributor Author

I like this, nice work. Can we get a function for the normal as well?

@NthTensor, Awesome! I've just finished that task. Plus, I'm aiming to address those triangle-specific concerns @Jondolf raised within the week.


@Jondolf, I'm excited to work on an MVP for us to integrate swiftly. While I'm impatient to dive into all the ideas you've shared, my recent joining and ongoing learning in CG, Bevy, and Rust mean I'd like to approach it with a bit of caution.

Here's what I plan to tackle first:

  • reverse
  • circumsphere
  • circumcircle, interestingly a 3D circle here, which seems like a valuable addition alongside circumsphere.
  • center

How do you feel about deferring the remaining features to a subsequent pull request? This approach aligns with the contributors' guide to keep changes manageable and more trackable. Additionally, I need to conduct some research on mesh and gizmo support. My goal is to ensure we can quickly integrate these foundational features without unnecessary delay.


Let's talk about the pub fn normal(&self) -> Result<Dir3, InvalidDirectionError>, @Jondolf.

What if we handle these errors directly in the new() function, we can catch them sooner, leading to quicker failure points. This approach means the Triangle3d methods can trust that the data within Self is valid, as it's been verified at the point of struct instantiation. What are your thoughts on this?

@vitorfhc
Copy link
Contributor Author

Side note: It'd kinda be nice if we used named properties a, b, and c instead of an array of vertices. It'd make it nicer to refer to specific vertices in docs and make methods like ab, bc, and ac sensible. This is probably out of scope for this PR though since it also applies to Triangle2d.

Absolutely, I'm on board! And I agree that it merits a separate PR due to its nature as a breaking change.

@NthTensor
Copy link
Contributor

NthTensor commented Mar 17, 2024

Gizmos and Meshing can (imo should) be left to future work. reverse, circumsphere, circumcircle and center seems like good targets for this, Bounded3d too if possible.

I think we probably want to left people represent degenerate triangles. We generally allow the other primitives (including Triangle2d) to be degenerate. So returning a result from normal is my preference, but maybe we want an is_degenerate method. By the way, it would be good to include a note about orientation and vertex ordering when you write docs for this.

Moving to named vertices rather than arrays is a good idea, also future work.

@alice-i-cecile
Copy link
Member

Awesome. Let me know when you're comfortable with an MVP, then make an issue or five for follow-up work :) I don't have strong feelings about doing it all at once versus split up, so whatever is most comfortable for you.

crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved
crates/bevy_math/src/primitives/dim3.rs Show resolved Hide resolved
crates/bevy_math/src/primitives/dim3.rs Outdated Show resolved Hide resolved
@vitorfhc
Copy link
Contributor Author

vitorfhc commented Mar 18, 2024

This comment is more for me to keep track of the future work on this PR and any issues I might have to open after it is merged.

  • Make bounding_sphere calculate the minimum shape by handling obtuse triangles differently (context)
  • Open an issue for the meshing and gizmo support (context)
  • Check inconsistencies between the 3D and the 2D version

Let me know if there's anything else I can add, and I'll edit this.

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.

Nice work! All looks correct to me. There's some doc formatting inconsistencies, but the entire crate is rife with that so don't worry too much about it.

@alice-i-cecile alice-i-cecile added this to the 0.14 milestone Mar 21, 2024
@Jondolf
Copy link
Contributor

Jondolf commented Mar 21, 2024

It'd be nice to have tests for all of the triangle math and the bounding implementations to make sure they work as expected. Triangle2d already has these too, but it's good to make sure the results are consistent, especially since some things are handled differently for the 3D version. But I probably wouldn't block over this, and I'm fine leaving it to a follow-up.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Mar 21, 2024

I'll merge this in the merge train next Monday unless there are further concerns :) There's a couple of open discussions still so I'll give y'all a bit of time to discuss since I don't think this will block upcoming work.

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