From bb12f7417188187da0625c1a8f7a5759f0354d3e Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 30 Sep 2022 10:11:04 +0200 Subject: [PATCH 01/11] Fix word in documentation --- crates/fj-kernel/src/objects/face.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-kernel/src/objects/face.rs b/crates/fj-kernel/src/objects/face.rs index c0e283933..169aa73c5 100644 --- a/crates/fj-kernel/src/objects/face.rs +++ b/crates/fj-kernel/src/objects/face.rs @@ -108,7 +108,7 @@ impl Face { /// /// Faces *do* have an orientation, meaning they have definite front and /// back sides. The front side is the side, where the face's exterior cycle - /// is wound clockwise. + /// is wound counter-clockwise. pub fn coord_handedness(&self) -> Handedness { match self.exterior().winding() { Winding::Ccw => Handedness::RightHanded, From 7793b94b02ce54d7ec5eb0730e0b069157ff3887 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 30 Sep 2022 10:37:23 +0200 Subject: [PATCH 02/11] Make variable name more specific --- crates/fj-kernel/src/objects/face.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/objects/face.rs b/crates/fj-kernel/src/objects/face.rs index 169aa73c5..dd6eabc16 100644 --- a/crates/fj-kernel/src/objects/face.rs +++ b/crates/fj-kernel/src/objects/face.rs @@ -51,14 +51,14 @@ impl Face { mut self, interiors: impl IntoIterator, ) -> Self { - for cycle in interiors.into_iter() { + for interior in interiors.into_iter() { assert_eq!( self.surface(), - cycle.surface(), + interior.surface(), "Cycles that bound a face must be in face's surface" ); - self.interiors.push(cycle); + self.interiors.push(interior); } self From 3026ed3d47a05e3b8ab608b27e8961b9c29a0db5 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 30 Sep 2022 11:01:45 +0200 Subject: [PATCH 03/11] Improve robustness of tests --- .../src/algorithms/triangulate/mod.rs | 22 +++++++++---------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/triangulate/mod.rs b/crates/fj-kernel/src/algorithms/triangulate/mod.rs index 5912fc12d..cada8bc54 100644 --- a/crates/fj-kernel/src/algorithms/triangulate/mod.rs +++ b/crates/fj-kernel/src/algorithms/triangulate/mod.rs @@ -141,12 +141,12 @@ mod tests { let triangles = triangulate(face)?; - let a = Point::from(a).to_xyz(); - let d = Point::from(d).to_xyz(); - let e = Point::from(e).to_xyz(); - let f = Point::from(f).to_xyz(); - let g = Point::from(g).to_xyz(); - let h = Point::from(h).to_xyz(); + let a = surface.point_from_surface_coords(a); + let d = surface.point_from_surface_coords(d); + let e = surface.point_from_surface_coords(e); + let f = surface.point_from_surface_coords(f); + let g = surface.point_from_surface_coords(g); + let h = surface.point_from_surface_coords(h); // Should contain some triangles from the polygon. Don't need to test // them all. @@ -193,11 +193,11 @@ mod tests { let triangles = triangulate(face)?; - let a3 = a.to_xyz(); - let b3 = b.to_xyz(); - let c3 = c.to_xyz(); - let d3 = d.to_xyz(); - let e3 = e.to_xyz(); + let a3 = surface.point_from_surface_coords(a); + let b3 = surface.point_from_surface_coords(b); + let c3 = surface.point_from_surface_coords(c); + let d3 = surface.point_from_surface_coords(d); + let e3 = surface.point_from_surface_coords(e); assert!(triangles.contains_triangle([a3, b3, d3])); assert!(triangles.contains_triangle([b3, c3, d3])); From 07490771e40ad3a46a22fbf6ddf9d062879ae265 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 30 Sep 2022 11:02:24 +0200 Subject: [PATCH 04/11] Simplify code --- crates/fj-kernel/src/algorithms/triangulate/mod.rs | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/triangulate/mod.rs b/crates/fj-kernel/src/algorithms/triangulate/mod.rs index cada8bc54..3349379a4 100644 --- a/crates/fj-kernel/src/algorithms/triangulate/mod.rs +++ b/crates/fj-kernel/src/algorithms/triangulate/mod.rs @@ -179,12 +179,12 @@ mod tests { // a ---------- b // - let a = Point::from([0., 0.]); - let b = Point::from([0.4, 0.]); - //let b = Point::from([0.5, 0.]); // test passes with this change - let c = Point::from([0.4, 1.0]); - let d = Point::from([0.1, 0.1]); - let e = Point::from([0., 0.8]); + let a = [0., 0.]; + let b = [0.4, 0.]; + //let b = [0.5, 0.]; // test passes with this change + let c = [0.4, 1.0]; + let d = [0.1, 0.1]; + let e = [0., 0.8]; let surface = Surface::xy_plane(); let face = Face::builder(&stores, surface) From 29f717e7b51f3169b94090b5b8ddd8225cc96ba4 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 30 Sep 2022 11:03:58 +0200 Subject: [PATCH 05/11] Derive `Debug` for `Mesh` --- crates/fj-interop/src/mesh.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/crates/fj-interop/src/mesh.rs b/crates/fj-interop/src/mesh.rs index 3427fd5b1..afa39519e 100644 --- a/crates/fj-interop/src/mesh.rs +++ b/crates/fj-interop/src/mesh.rs @@ -5,6 +5,7 @@ use std::{collections::HashMap, hash::Hash}; use fj_math::Point; /// A triangle mesh +#[derive(Debug)] pub struct Mesh { vertices: Vec, indices: Vec, From bc4c25b48b09d014f36670caa8bc71b4444aa24c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 30 Sep 2022 11:04:37 +0200 Subject: [PATCH 06/11] Improve comment --- crates/fj-kernel/src/algorithms/triangulate/mod.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/triangulate/mod.rs b/crates/fj-kernel/src/algorithms/triangulate/mod.rs index 3349379a4..d38acba17 100644 --- a/crates/fj-kernel/src/algorithms/triangulate/mod.rs +++ b/crates/fj-kernel/src/algorithms/triangulate/mod.rs @@ -148,8 +148,15 @@ mod tests { let g = surface.point_from_surface_coords(g); let h = surface.point_from_surface_coords(h); - // Should contain some triangles from the polygon. Don't need to test - // them all. + // Let's test that some correct triangles are present. We don't need to + // test them all. + // + // Please note that there are multiple valid triangulations of any given + // polygon. So if you change the input data above, or the algorithm, the + // following assertions might break. + // + // This limits the usefulness of this test. It would be better to have a + // smarter way of verifying the triangulation. assert!(triangles.contains_triangle([a, e, h])); assert!(triangles.contains_triangle([a, d, h])); From 2cea756dd315fda4b8ed9d0198e769959bedfb19 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 30 Sep 2022 11:05:57 +0200 Subject: [PATCH 07/11] Fix interior cycle winding in test I'm about to add validation code for that. --- crates/fj-kernel/src/algorithms/intersect/curve_face.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/intersect/curve_face.rs b/crates/fj-kernel/src/algorithms/intersect/curve_face.rs index 1cb6391b1..43bc1b667 100644 --- a/crates/fj-kernel/src/algorithms/intersect/curve_face.rs +++ b/crates/fj-kernel/src/algorithms/intersect/curve_face.rs @@ -184,9 +184,9 @@ mod tests { #[rustfmt::skip] let interior = [ [-1., -1.], - [ 1., -1.], - [ 1., 1.], [-1., 1.], + [ 1., 1.], + [ 1., -1.], ]; let face = Face::builder(&stores, surface) From a638c460206eeb8e1033d81ebb24d42c164e2ba6 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 30 Sep 2022 11:08:20 +0200 Subject: [PATCH 08/11] Fix interior cycle winding in test --- crates/fj-kernel/src/algorithms/triangulate/mod.rs | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/triangulate/mod.rs b/crates/fj-kernel/src/algorithms/triangulate/mod.rs index d38acba17..8ab4d2128 100644 --- a/crates/fj-kernel/src/algorithms/triangulate/mod.rs +++ b/crates/fj-kernel/src/algorithms/triangulate/mod.rs @@ -129,9 +129,9 @@ mod tests { let d = [0., 4.]; let e = [1., 1.]; - let f = [3., 1.]; + let f = [1., 2.]; let g = [3., 3.]; - let h = [1., 2.]; + let h = [3., 1.]; let surface = Surface::xy_plane(); let face = Face::builder(&stores, surface) @@ -142,7 +142,7 @@ mod tests { let triangles = triangulate(face)?; let a = surface.point_from_surface_coords(a); - let d = surface.point_from_surface_coords(d); + let b = surface.point_from_surface_coords(b); let e = surface.point_from_surface_coords(e); let f = surface.point_from_surface_coords(f); let g = surface.point_from_surface_coords(g); @@ -157,8 +157,8 @@ mod tests { // // This limits the usefulness of this test. It would be better to have a // smarter way of verifying the triangulation. - assert!(triangles.contains_triangle([a, e, h])); - assert!(triangles.contains_triangle([a, d, h])); + assert!(triangles.contains_triangle([a, b, e])); + assert!(triangles.contains_triangle([b, e, h])); // Shouldn't contain any possible triangle from the hole. assert!(!triangles.contains_triangle([e, f, g])); From 67e74aa6f86f229ec60bb9787cfc921db23c61b1 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 30 Sep 2022 11:08:47 +0200 Subject: [PATCH 09/11] Validate interior cycle winding --- crates/fj-kernel/src/objects/face.rs | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/crates/fj-kernel/src/objects/face.rs b/crates/fj-kernel/src/objects/face.rs index dd6eabc16..9bf7a058e 100644 --- a/crates/fj-kernel/src/objects/face.rs +++ b/crates/fj-kernel/src/objects/face.rs @@ -57,6 +57,11 @@ impl Face { interior.surface(), "Cycles that bound a face must be in face's surface" ); + assert_ne!( + self.exterior().winding(), + interior.winding(), + "Interior cycles must have opposite winding of exterior cycle" + ); self.interiors.push(interior); } From 8f4bf096227c19912797dc9383e6541d038c8985 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 30 Sep 2022 11:10:00 +0200 Subject: [PATCH 10/11] Update documentation of `Face` --- crates/fj-kernel/src/objects/face.rs | 28 +++++++++++++++++++++++++++- 1 file changed, 27 insertions(+), 1 deletion(-) diff --git a/crates/fj-kernel/src/objects/face.rs b/crates/fj-kernel/src/objects/face.rs index 9bf7a058e..90ab3d615 100644 --- a/crates/fj-kernel/src/objects/face.rs +++ b/crates/fj-kernel/src/objects/face.rs @@ -8,6 +8,29 @@ use crate::{builder::FaceBuilder, stores::Stores}; use super::{Cycle, Surface}; /// A face of a shape +/// +/// A `Face` is a bounded area of a [`Surface`], the [`Surface`] itself being an +/// infinite 2-dimensional object in 3D space. `Face`s are bound by one exterior +/// cycle, which defines the outer boundary, and an arbitrary number of interior +/// cycles (i.e. holes). +/// +/// `Face` has a defined orientation, a front and a back side. When faces are +/// combined into [`Shell`]s, the face orientation defines what is inside and +/// outside of the shell. This stands in contrast to [`Surface`], which has no +/// defined orientation. +/// +/// You can look at a `Face` from two directions: front and back. The winding of +/// the exterior cycle will be clockwise or counter-clockwise, depending on your +/// perspective. The front side of the face, is the side where from which the +/// exterior cycle appear counter-clockwise. +/// +/// Interior cycles must have the opposite winding of the exterior cycle, +/// meaning on the front side of the face, they must appear clockwise. This +/// means that all [`HalfEdge`]s that bound a `Face` have the interior of the +/// face on their left side (on the face's front side). +/// +/// [`HalfEdge`]: super::HalfEdge +/// [`Shell`]: super::Shell #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub struct Face { surface: Surface, @@ -47,6 +70,9 @@ impl Face { /// # Panics /// /// Panics, if the added cycles are not defined in the face's surface. + /// + /// Panics, if the winding of the interior cycles is not opposite that of + /// the exterior cycle. pub fn with_interiors( mut self, interiors: impl IntoIterator, @@ -82,7 +108,7 @@ impl Face { &self.surface } - /// Access the cycles that bound the face on the outside + /// Access the cycle that bounds the face on the outside pub fn exterior(&self) -> &Cycle { &self.exterior } From 52d9141640279e16a0bfc5c0640991fa01073602 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 30 Sep 2022 11:11:22 +0200 Subject: [PATCH 11/11] Update doc comment --- crates/fj-kernel/src/objects/cycle.rs | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/crates/fj-kernel/src/objects/cycle.rs b/crates/fj-kernel/src/objects/cycle.rs index 81e5a24cf..d3e09f95a 100644 --- a/crates/fj-kernel/src/objects/cycle.rs +++ b/crates/fj-kernel/src/objects/cycle.rs @@ -95,6 +95,10 @@ impl Cycle { } /// Indicate the cycle's winding, assuming a right-handed coordinate system + /// + /// Please note that this is not *the* winding of the cycle, only one of the + /// two possible windings, depending on the direction you look at the + /// surface that the cycle is defined on from. pub fn winding(&self) -> Winding { // The cycle could be made up of one or two circles. If that is the // case, the winding of the cycle is determined by the winding of the