From d9533ee033bd40f985676821c403aaae12f2a06f Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Jul 2024 18:58:12 +0200 Subject: [PATCH 01/12] Add `Triangle::is_valid` The method doesn't make sense yet, as `Triangle::new` explicitly forbids degenerate triangles. This is about to change though, and adding this method is preparation for that. --- crates/fj-math/src/triangle.rs | 21 +++++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/crates/fj-math/src/triangle.rs b/crates/fj-math/src/triangle.rs index 0dcd417e33..55a768634d 100644 --- a/crates/fj-math/src/triangle.rs +++ b/crates/fj-math/src/triangle.rs @@ -1,3 +1,4 @@ +use approx::AbsDiffEq; use parry3d_f64::query::{Ray, RayCast as _}; use crate::Vector; @@ -41,6 +42,26 @@ impl Triangle { self.points } + /// # Determine whether the triangle is valid + /// + /// A triangle is valid, if it is not degenerate. In a degenerate triangle, + /// the three points do not form an actual triangle, but a line or even a + /// single point. + /// + /// ## Implementation Note + /// + /// Right now, this function computes the area of the triangle, and compares + /// it against [`Scalar`]'s default epsilon value. This might not be + /// flexible enough for all use cases. + /// + /// Long-term, it might become necessary to add some way to override the + /// epsilon value used within this function. + pub fn is_valid(&self) -> bool { + let [a, b, c] = self.points; + let area = (b - a).outer(&(c - a)).magnitude(); + area > Scalar::default_epsilon() + } + /// Normalize the triangle /// /// Returns a new `Triangle` instance with the same points, but the points From 3529eca44adf260fa83d9c999a7dbea5ea852b7a Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Jul 2024 19:13:43 +0200 Subject: [PATCH 02/12] Prepare for follow-on change --- crates/fj-core/src/algorithms/triangulate/delaunay.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/fj-core/src/algorithms/triangulate/delaunay.rs b/crates/fj-core/src/algorithms/triangulate/delaunay.rs index 306caefbbe..b729868f22 100644 --- a/crates/fj-core/src/algorithms/triangulate/delaunay.rs +++ b/crates/fj-core/src/algorithms/triangulate/delaunay.rs @@ -47,13 +47,13 @@ pub fn triangulate( let mut triangles = Vec::new(); for triangle in triangulation.inner_faces() { let [v0, v1, v2] = triangle.vertices().map(|vertex| *vertex.data()); - let triangle_winding = Triangle::<2>::from_points([ + let triangle = Triangle::<2>::from_points([ v0.point_surface, v1.point_surface, v2.point_surface, ]) - .expect("invalid triangle") - .winding(); + .expect("invalid triangle"); + let triangle_winding = triangle.winding(); let required_winding = match coord_handedness { Handedness::LeftHanded => Winding::Cw, From d4b880d185319cc0135252c3f6647ed8ebc05e15 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Jul 2024 19:14:06 +0200 Subject: [PATCH 03/12] Inline redundant variable --- crates/fj-core/src/algorithms/triangulate/delaunay.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/fj-core/src/algorithms/triangulate/delaunay.rs b/crates/fj-core/src/algorithms/triangulate/delaunay.rs index b729868f22..8a90a2cf62 100644 --- a/crates/fj-core/src/algorithms/triangulate/delaunay.rs +++ b/crates/fj-core/src/algorithms/triangulate/delaunay.rs @@ -53,14 +53,13 @@ pub fn triangulate( v2.point_surface, ]) .expect("invalid triangle"); - let triangle_winding = triangle.winding(); let required_winding = match coord_handedness { Handedness::LeftHanded => Winding::Cw, Handedness::RightHanded => Winding::Ccw, }; - let triangle = if triangle_winding == required_winding { + let triangle = if triangle.winding() == required_winding { [v0, v1, v2] } else { [v0, v2, v1] From 0df27dee0d5189d9986e6c93553521aa86f51c52 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Jul 2024 19:17:11 +0200 Subject: [PATCH 04/12] Make sure triangulation returns valid triangles This check is currently redundant with what the `Triangle` constructor already does. The constructor will soon get simplified though, and then the new check will be necessary. --- crates/fj-core/src/algorithms/triangulate/delaunay.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/fj-core/src/algorithms/triangulate/delaunay.rs b/crates/fj-core/src/algorithms/triangulate/delaunay.rs index 8a90a2cf62..05cf8cc81c 100644 --- a/crates/fj-core/src/algorithms/triangulate/delaunay.rs +++ b/crates/fj-core/src/algorithms/triangulate/delaunay.rs @@ -53,6 +53,10 @@ pub fn triangulate( v2.point_surface, ]) .expect("invalid triangle"); + assert!( + triangle.is_valid(), + "Expecting triangles created by triangulation to be valid.", + ); let required_winding = match coord_handedness { Handedness::LeftHanded => Winding::Cw, From 227b2adbc93791b7e773eb1a525ea4bd2403fb8d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Jul 2024 19:53:44 +0200 Subject: [PATCH 05/12] Make tests more explicit --- crates/fj-math/src/triangle.rs | 14 ++++++++------ 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/fj-math/src/triangle.rs b/crates/fj-math/src/triangle.rs index 55a768634d..8b375648b9 100644 --- a/crates/fj-math/src/triangle.rs +++ b/crates/fj-math/src/triangle.rs @@ -181,7 +181,8 @@ mod tests { let a = Point::from([0.0, 0.0]); let b = Point::from([1.0, 1.0]); let c = Point::from([1.0, 2.0]); - let _triangle = Triangle::from([a, b, c]); + + assert!(Triangle::from_points([a, b, c]).is_ok()); } #[test] @@ -189,25 +190,26 @@ mod tests { let a = Point::from([0.0, 0.0, 0.0]); let b = Point::from([1.0, 1.0, 0.0]); let c = Point::from([1.0, 2.0, 0.0]); - let _triangle = Triangle::from([a, b, c]); + + assert!(Triangle::from_points([a, b, c]).is_ok()); } #[test] - #[should_panic] fn invalid_triangle_2d() { let a = Point::from([0.0, 0.0]); let b = Point::from([1.0, 1.0]); let c = Point::from([2.0, 2.0]); - let _triangle = Triangle::from([a, b, c]); + + assert!(Triangle::from_points([a, b, c]).is_err()); } #[test] - #[should_panic] fn invalid_triangle_3d() { let a = Point::from([0.0, 0.0, 0.0]); let b = Point::from([1.0, 1.0, 1.0]); let c = Point::from([2.0, 2.0, 2.0]); - let _triangle = Triangle::from([a, b, c]); + + assert!(Triangle::from_points([a, b, c]).is_err()); } #[test] From f5668b0e25dbd177134e7bfe083fa77d09ec2e6b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 30 Jul 2024 18:33:28 +0200 Subject: [PATCH 06/12] Simplify numbers used in tests --- crates/fj-math/src/triangle.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/fj-math/src/triangle.rs b/crates/fj-math/src/triangle.rs index 8b375648b9..b8c2abadec 100644 --- a/crates/fj-math/src/triangle.rs +++ b/crates/fj-math/src/triangle.rs @@ -179,8 +179,8 @@ mod tests { #[test] fn valid_triangle_2d() { let a = Point::from([0.0, 0.0]); - let b = Point::from([1.0, 1.0]); - let c = Point::from([1.0, 2.0]); + let b = Point::from([1.0, 0.0]); + let c = Point::from([0.0, 1.0]); assert!(Triangle::from_points([a, b, c]).is_ok()); } @@ -188,8 +188,8 @@ mod tests { #[test] fn valid_triangle_3d() { let a = Point::from([0.0, 0.0, 0.0]); - let b = Point::from([1.0, 1.0, 0.0]); - let c = Point::from([1.0, 2.0, 0.0]); + let b = Point::from([0.0, 1.0, 0.0]); + let c = Point::from([1.0, 0.0, 0.0]); assert!(Triangle::from_points([a, b, c]).is_ok()); } @@ -197,8 +197,8 @@ mod tests { #[test] fn invalid_triangle_2d() { let a = Point::from([0.0, 0.0]); - let b = Point::from([1.0, 1.0]); - let c = Point::from([2.0, 2.0]); + let b = Point::from([1.0, 0.0]); + let c = Point::from([2.0, 0.0]); assert!(Triangle::from_points([a, b, c]).is_err()); } @@ -206,8 +206,8 @@ mod tests { #[test] fn invalid_triangle_3d() { let a = Point::from([0.0, 0.0, 0.0]); - let b = Point::from([1.0, 1.0, 1.0]); - let c = Point::from([2.0, 2.0, 2.0]); + let b = Point::from([1.0, 0.0, 0.0]); + let c = Point::from([2.0, 0.0, 0.0]); assert!(Triangle::from_points([a, b, c]).is_err()); } From cc15b15a57d8b3d4eacbf069a32c957914f2505b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Jul 2024 19:20:11 +0200 Subject: [PATCH 07/12] Allow degenerate triangles to be constructed Degenerate triangles are actually useful in some cases, and I'm actually working on some code that needs them. Thus, the constructor as it was, was too restrictive. --- crates/fj-math/src/line.rs | 2 +- crates/fj-math/src/triangle.rs | 21 +++++---------------- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/crates/fj-math/src/line.rs b/crates/fj-math/src/line.rs index 1e598ae49e..0e8bb098b5 100644 --- a/crates/fj-math/src/line.rs +++ b/crates/fj-math/src/line.rs @@ -99,7 +99,7 @@ impl Line { // The triangle is valid only, if the three points are not on the // same line. - Triangle::from_points([a, b, c]).is_ok() + Triangle::from_points([a, b, c]).unwrap().is_valid() }; if other_origin_is_not_on_self { diff --git a/crates/fj-math/src/triangle.rs b/crates/fj-math/src/triangle.rs index b8c2abadec..62d72b1c0f 100644 --- a/crates/fj-math/src/triangle.rs +++ b/crates/fj-math/src/triangle.rs @@ -23,18 +23,7 @@ impl Triangle { points: [impl Into>; 3], ) -> Result> { let points = points.map(Into::into); - - let area = { - let [a, b, c] = points.map(Point::to_xyz); - (b - a).cross(&(c - a)).magnitude() - }; - - // A triangle is not valid if it doesn't span any area - if area != Scalar::from(0.0) { - Ok(Self { points }) - } else { - Err(NotATriangle { points }) - } + Ok(Self { points }) } /// Access the triangle's points @@ -182,7 +171,7 @@ mod tests { let b = Point::from([1.0, 0.0]); let c = Point::from([0.0, 1.0]); - assert!(Triangle::from_points([a, b, c]).is_ok()); + assert!(Triangle::from_points([a, b, c]).unwrap().is_valid()); } #[test] @@ -191,7 +180,7 @@ mod tests { let b = Point::from([0.0, 1.0, 0.0]); let c = Point::from([1.0, 0.0, 0.0]); - assert!(Triangle::from_points([a, b, c]).is_ok()); + assert!(Triangle::from_points([a, b, c]).unwrap().is_valid()); } #[test] @@ -200,7 +189,7 @@ mod tests { let b = Point::from([1.0, 0.0]); let c = Point::from([2.0, 0.0]); - assert!(Triangle::from_points([a, b, c]).is_err()); + assert!(!Triangle::from_points([a, b, c]).unwrap().is_valid()); } #[test] @@ -209,7 +198,7 @@ mod tests { let b = Point::from([1.0, 0.0, 0.0]); let c = Point::from([2.0, 0.0, 0.0]); - assert!(Triangle::from_points([a, b, c]).is_err()); + assert!(!Triangle::from_points([a, b, c]).unwrap().is_valid()); } #[test] From 63a45460f94a3315be01cd073b7d1db73a2cc093 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Jul 2024 19:21:56 +0200 Subject: [PATCH 08/12] Simplify return value --- .../src/algorithms/triangulate/delaunay.rs | 3 +-- crates/fj-math/src/line.rs | 2 +- crates/fj-math/src/triangle.rs | 16 +++++++--------- 3 files changed, 9 insertions(+), 12 deletions(-) diff --git a/crates/fj-core/src/algorithms/triangulate/delaunay.rs b/crates/fj-core/src/algorithms/triangulate/delaunay.rs index 05cf8cc81c..d3826334cf 100644 --- a/crates/fj-core/src/algorithms/triangulate/delaunay.rs +++ b/crates/fj-core/src/algorithms/triangulate/delaunay.rs @@ -51,8 +51,7 @@ pub fn triangulate( v0.point_surface, v1.point_surface, v2.point_surface, - ]) - .expect("invalid triangle"); + ]); assert!( triangle.is_valid(), "Expecting triangles created by triangulation to be valid.", diff --git a/crates/fj-math/src/line.rs b/crates/fj-math/src/line.rs index 0e8bb098b5..df38100126 100644 --- a/crates/fj-math/src/line.rs +++ b/crates/fj-math/src/line.rs @@ -99,7 +99,7 @@ impl Line { // The triangle is valid only, if the three points are not on the // same line. - Triangle::from_points([a, b, c]).unwrap().is_valid() + Triangle::from_points([a, b, c]).is_valid() }; if other_origin_is_not_on_self { diff --git a/crates/fj-math/src/triangle.rs b/crates/fj-math/src/triangle.rs index 62d72b1c0f..3283c36942 100644 --- a/crates/fj-math/src/triangle.rs +++ b/crates/fj-math/src/triangle.rs @@ -19,11 +19,9 @@ impl Triangle { /// Construct a triangle from three points /// /// Returns an error, if the points don't form a triangle. - pub fn from_points( - points: [impl Into>; 3], - ) -> Result> { + pub fn from_points(points: [impl Into>; 3]) -> Self { let points = points.map(Into::into); - Ok(Self { points }) + Self { points } } /// Access the triangle's points @@ -127,7 +125,7 @@ where P: Into>, { fn from(points: [P; 3]) -> Self { - Self::from_points(points).expect("invalid triangle") + Self::from_points(points) } } @@ -171,7 +169,7 @@ mod tests { let b = Point::from([1.0, 0.0]); let c = Point::from([0.0, 1.0]); - assert!(Triangle::from_points([a, b, c]).unwrap().is_valid()); + assert!(Triangle::from_points([a, b, c]).is_valid()); } #[test] @@ -180,7 +178,7 @@ mod tests { let b = Point::from([0.0, 1.0, 0.0]); let c = Point::from([1.0, 0.0, 0.0]); - assert!(Triangle::from_points([a, b, c]).unwrap().is_valid()); + assert!(Triangle::from_points([a, b, c]).is_valid()); } #[test] @@ -189,7 +187,7 @@ mod tests { let b = Point::from([1.0, 0.0]); let c = Point::from([2.0, 0.0]); - assert!(!Triangle::from_points([a, b, c]).unwrap().is_valid()); + assert!(!Triangle::from_points([a, b, c]).is_valid()); } #[test] @@ -198,7 +196,7 @@ mod tests { let b = Point::from([1.0, 0.0, 0.0]); let c = Point::from([2.0, 0.0, 0.0]); - assert!(!Triangle::from_points([a, b, c]).unwrap().is_valid()); + assert!(!Triangle::from_points([a, b, c]).is_valid()); } #[test] From 1d149d2968c5cda8667e80049487dc795d16910c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Jul 2024 19:22:20 +0200 Subject: [PATCH 09/12] Update doc comment --- crates/fj-math/src/triangle.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/fj-math/src/triangle.rs b/crates/fj-math/src/triangle.rs index 3283c36942..7c24f9a058 100644 --- a/crates/fj-math/src/triangle.rs +++ b/crates/fj-math/src/triangle.rs @@ -17,8 +17,6 @@ pub struct Triangle { impl Triangle { /// Construct a triangle from three points - /// - /// Returns an error, if the points don't form a triangle. pub fn from_points(points: [impl Into>; 3]) -> Self { let points = points.map(Into::into); Self { points } From be31c2d10a0eea2f5e3e3b5abe053c93aec20cce Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Jul 2024 19:23:40 +0200 Subject: [PATCH 10/12] Make field of `Triangle` public With the constructor no longer preventing degenerate triangle, there's no reason to keep it private. --- crates/fj-math/src/triangle.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/fj-math/src/triangle.rs b/crates/fj-math/src/triangle.rs index 7c24f9a058..f34072a40b 100644 --- a/crates/fj-math/src/triangle.rs +++ b/crates/fj-math/src/triangle.rs @@ -12,7 +12,8 @@ use super::{Point, Scalar}; #[derive(Clone, Copy, Debug, Default, Eq, PartialEq, Hash, Ord, PartialOrd)] #[repr(C)] pub struct Triangle { - points: [Point; 3], + /// The points that make up the triangle + pub points: [Point; 3], } impl Triangle { From 14e1982545699fc269f673d63cf33e0538c2cd23 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Jul 2024 19:25:50 +0200 Subject: [PATCH 11/12] Remove `Triangle::points` With the `points` field public, it has become redundant. --- crates/fj-core/src/algorithms/triangulate/polygon.rs | 2 +- crates/fj-export/src/lib.rs | 4 ++-- crates/fj-interop/src/mesh.rs | 2 +- crates/fj-math/src/transform.rs | 2 +- crates/fj-math/src/triangle.rs | 7 +------ crates/fj-viewer/src/graphics/vertices.rs | 2 +- 6 files changed, 7 insertions(+), 12 deletions(-) diff --git a/crates/fj-core/src/algorithms/triangulate/polygon.rs b/crates/fj-core/src/algorithms/triangulate/polygon.rs index cb18de13fd..2795bd5b1a 100644 --- a/crates/fj-core/src/algorithms/triangulate/polygon.rs +++ b/crates/fj-core/src/algorithms/triangulate/polygon.rs @@ -42,7 +42,7 @@ impl Polygon { } pub fn contains_triangle(&self, triangle: impl Into>) -> bool { - let [a, b, c] = triangle.into().points(); + let [a, b, c] = triangle.into().points; let mut might_be_hole = true; diff --git a/crates/fj-export/src/lib.rs b/crates/fj-export/src/lib.rs index d193b1cef3..10d5b3df6b 100644 --- a/crates/fj-export/src/lib.rs +++ b/crates/fj-export/src/lib.rs @@ -89,7 +89,7 @@ pub fn export_stl( ) -> Result<(), Error> { let points = mesh .triangles() - .map(|triangle| triangle.inner.points()) + .map(|triangle| triangle.inner.points) .collect::>(); let vertices = points.iter().map(|points| { @@ -136,7 +136,7 @@ pub fn export_obj( ) -> Result<(), Error> { for (cnt, t) in mesh.triangles().enumerate() { // write each point of the triangle - for v in t.inner.points() { + for v in t.inner.points { wavefront_rs::obj::writer::Writer { auto_newline: true } .write( &mut write, diff --git a/crates/fj-interop/src/mesh.rs b/crates/fj-interop/src/mesh.rs index 032e5ef4fa..0635e56892 100644 --- a/crates/fj-interop/src/mesh.rs +++ b/crates/fj-interop/src/mesh.rs @@ -80,7 +80,7 @@ impl Mesh> { ) { let triangle = triangle.into(); - for point in triangle.points() { + for point in triangle.points { self.push_vertex(point); } diff --git a/crates/fj-math/src/transform.rs b/crates/fj-math/src/transform.rs index 61a32621a8..8b7e5d7ec1 100644 --- a/crates/fj-math/src/transform.rs +++ b/crates/fj-math/src/transform.rs @@ -78,7 +78,7 @@ impl Transform { /// Transform the given triangle pub fn transform_triangle(&self, triangle: &Triangle<3>) -> Triangle<3> { - let [a, b, c] = &triangle.points(); + let [a, b, c] = &triangle.points; Triangle::from([ self.transform_point(a), self.transform_point(b), diff --git a/crates/fj-math/src/triangle.rs b/crates/fj-math/src/triangle.rs index f34072a40b..5937e3f209 100644 --- a/crates/fj-math/src/triangle.rs +++ b/crates/fj-math/src/triangle.rs @@ -23,11 +23,6 @@ impl Triangle { Self { points } } - /// Access the triangle's points - pub fn points(&self) -> [Point; 3] { - self.points - } - /// # Determine whether the triangle is valid /// /// A triangle is valid, if it is not degenerate. In a degenerate triangle, @@ -88,7 +83,7 @@ impl Triangle<2> { impl Triangle<3> { /// Convert the triangle to a Parry triangle pub fn to_parry(self) -> parry3d_f64::shape::Triangle { - self.points().map(|vertex| vertex.to_na()).into() + self.points.map(|vertex| vertex.to_na()).into() } /// Cast a ray against the Triangle diff --git a/crates/fj-viewer/src/graphics/vertices.rs b/crates/fj-viewer/src/graphics/vertices.rs index 2f2af8a6c7..14e1a29c8a 100644 --- a/crates/fj-viewer/src/graphics/vertices.rs +++ b/crates/fj-viewer/src/graphics/vertices.rs @@ -29,7 +29,7 @@ impl From<&Mesh>> for Vertices { let mut m = Mesh::new(); for triangle in mesh.triangles() { - let [a, b, c] = triangle.inner.points(); + let [a, b, c] = triangle.inner.points; let normal = (b - a).cross(&(c - a)).normalize(); let color = triangle.color; From b553b1eaea4a341eaa8489ac5f4132d638095864 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Jul 2024 19:27:03 +0200 Subject: [PATCH 12/12] Remove unused code --- crates/fj-math/src/triangle.rs | 6 ------ 1 file changed, 6 deletions(-) diff --git a/crates/fj-math/src/triangle.rs b/crates/fj-math/src/triangle.rs index 5937e3f209..92cbbb0fa7 100644 --- a/crates/fj-math/src/triangle.rs +++ b/crates/fj-math/src/triangle.rs @@ -123,12 +123,6 @@ where } } -/// Returned by [`Triangle::from_points`], if the points don't form a triangle -#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] -pub struct NotATriangle { - pub points: [Point; 3], -} - /// Winding direction of a triangle. #[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub enum Winding {