From e9a2263c9a09ce203b804479e769c9a8208ceb01 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 22 Mar 2022 14:13:53 +0100 Subject: [PATCH 01/13] Improve formatting --- fj-kernel/src/algorithms/approximation.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index 161e27187..525152cae 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -70,6 +70,7 @@ impl Approximation { // doesn't need to be handled here, is a sphere. A spherical face would // would need to provide its own approximation, as the edges that bound // it have nothing to do with its curvature. + let mut points = HashSet::new(); let mut segments = HashSet::new(); From 4d7150f58a67b6a1d9889bd14b79619ec372ad9c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 22 Mar 2022 14:24:35 +0100 Subject: [PATCH 02/13] Inline method call No code outside of `approximation` actually needs the approximation of a single edge. --- fj-kernel/src/algorithms/approximation.rs | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index 525152cae..75d4fdfbd 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -2,7 +2,7 @@ use std::collections::HashSet; use fj_math::{Point, Scalar, Segment}; -use crate::topology::{Cycle, Edge, Face, Vertex}; +use crate::topology::{Cycle, Face, Vertex}; /// An approximation of an edge, multiple edges, or a face #[derive(Debug, PartialEq)] @@ -24,17 +24,6 @@ pub struct Approximation { } impl Approximation { - /// Compute an approximate for an edge - /// - /// `tolerance` defines how far the approximation is allowed to deviate from - /// the actual edge. - pub fn for_edge(edge: &Edge, tolerance: Scalar) -> Self { - let mut points = Vec::new(); - edge.curve().approx(tolerance, &mut points); - - approximate_edge(points, edge.vertices()) - } - /// Compute an approximation for a cycle /// /// `tolerance` defines how far the approximation is allowed to deviate from @@ -44,7 +33,10 @@ impl Approximation { let mut segments = HashSet::new(); for edge in cycle.edges() { - let approx = Self::for_edge(&edge, tolerance); + let mut edge_points = Vec::new(); + edge.curve().approx(tolerance, &mut edge_points); + + let approx = approximate_edge(edge_points, edge.vertices()); points.extend(approx.points); segments.extend(approx.segments); From 5a20c2f15a832656c90e2396b78872b0d91d704b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 22 Mar 2022 14:26:31 +0100 Subject: [PATCH 03/13] Prevent name collision I'm about to rename the test. --- fj-kernel/src/algorithms/approximation.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index 75d4fdfbd..53c592562 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -129,7 +129,7 @@ mod tests { topology::{Cycle, Face, Vertex}, }; - use super::{approximate_edge, Approximation}; + use super::Approximation; #[test] fn for_edge() { @@ -153,7 +153,7 @@ mod tests { // Regular edge assert_eq!( - approximate_edge( + super::approximate_edge( points.clone(), Some([v1.get().clone(), v2.get().clone()]) ), @@ -169,7 +169,7 @@ mod tests { // Continuous edge assert_eq!( - approximate_edge(points, None), + super::approximate_edge(points, None), Approximation { points: set![b, c], segments: set![Segment::from([b, c]), Segment::from([c, b])], From f621ccc47e1941b8b48747178360aea68092c57d Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 22 Mar 2022 14:26:49 +0100 Subject: [PATCH 04/13] Update test name --- fj-kernel/src/algorithms/approximation.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index 53c592562..c882aac3c 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -132,7 +132,7 @@ mod tests { use super::Approximation; #[test] - fn for_edge() { + fn approximate_edge() { // Doesn't test `Approximation::for_edge` directly, but that method only // contains a bit of additional glue code that is not critical. From 85497507dcb772d849b01b9a362a5eda4135753b Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 22 Mar 2022 14:27:29 +0100 Subject: [PATCH 05/13] Remove obsolete comment --- fj-kernel/src/algorithms/approximation.rs | 3 --- 1 file changed, 3 deletions(-) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index c882aac3c..07d9e4bf8 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -133,9 +133,6 @@ mod tests { #[test] fn approximate_edge() { - // Doesn't test `Approximation::for_edge` directly, but that method only - // contains a bit of additional glue code that is not critical. - let mut shape = Shape::new(); let a = Point::from([1., 2., 3.]); From 4f18b052230c13204c2ec44acfef90ce8089d739 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 22 Mar 2022 14:36:21 +0100 Subject: [PATCH 06/13] Remove redundant test The other tests in the suite already cover the same functionality pretty well. --- fj-kernel/src/algorithms/approximation.rs | 42 ----------------------- 1 file changed, 42 deletions(-) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index 07d9e4bf8..67b03b109 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -174,48 +174,6 @@ mod tests { ); } - #[test] - fn for_cycle() { - let tolerance = Scalar::ONE; - - let mut shape = Shape::new(); - - let a = Point::from([1., 2., 3.]); - let b = Point::from([2., 3., 5.]); - let c = Point::from([3., 5., 8.]); - - let v1 = shape.geometry().add_point(a); - let v2 = shape.geometry().add_point(b); - let v3 = shape.geometry().add_point(c); - - let v1 = shape.topology().add_vertex(Vertex { point: v1 }).unwrap(); - let v2 = shape.topology().add_vertex(Vertex { point: v2 }).unwrap(); - let v3 = shape.topology().add_vertex(Vertex { point: v3 }).unwrap(); - - let ab = shape - .topology() - .add_line_segment([v1.clone(), v2.clone()]) - .unwrap(); - let bc = shape.topology().add_line_segment([v2, v3.clone()]).unwrap(); - let ca = shape.topology().add_line_segment([v3, v1]).unwrap(); - - let cycle = Cycle { - edges: vec![ab, bc, ca], - }; - - assert_eq!( - Approximation::for_cycle(&cycle, tolerance), - Approximation { - points: set![a, b, c], - segments: set![ - Segment::from([a, b]), - Segment::from([b, c]), - Segment::from([c, a]), - ], - } - ); - } - #[test] fn for_face_closed() { // Test a closed face, i.e. one that is completely encircled by edges. From e22fec9e796fc0aa6ee620880fc914d0a129cdb3 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 22 Mar 2022 14:41:23 +0100 Subject: [PATCH 07/13] Convert method into function This makes room for simplifying the function, having it return something else than a full `Approximation`. --- fj-kernel/src/algorithms/approximation.rs | 44 +++++++++++------------ fj-kernel/src/algorithms/sweep.rs | 5 ++- 2 files changed, 24 insertions(+), 25 deletions(-) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index 67b03b109..698497a26 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -24,27 +24,6 @@ pub struct Approximation { } impl Approximation { - /// Compute an approximation for a cycle - /// - /// `tolerance` defines how far the approximation is allowed to deviate from - /// the actual cycle. - pub fn for_cycle(cycle: &Cycle, tolerance: Scalar) -> Self { - let mut points = HashSet::new(); - let mut segments = HashSet::new(); - - for edge in cycle.edges() { - let mut edge_points = Vec::new(); - edge.curve().approx(tolerance, &mut edge_points); - - let approx = approximate_edge(edge_points, edge.vertices()); - - points.extend(approx.points); - segments.extend(approx.segments); - } - - Self { points, segments } - } - /// Compute an approximation for a face /// /// `tolerance` defines how far the approximation is allowed to deviate from @@ -67,7 +46,7 @@ impl Approximation { let mut segments = HashSet::new(); for cycle in face.cycles() { - let approx = Self::for_cycle(&cycle, tolerance); + let approx = approximate_cycle(&cycle, tolerance); points.extend(approx.points); segments.extend(approx.segments); @@ -118,6 +97,27 @@ fn approximate_edge( } } +/// Compute an approximation for a cycle +/// +/// `tolerance` defines how far the approximation is allowed to deviate from the +/// actual cycle. +pub fn approximate_cycle(cycle: &Cycle, tolerance: Scalar) -> Approximation { + let mut points = HashSet::new(); + let mut segments = HashSet::new(); + + for edge in cycle.edges() { + let mut edge_points = Vec::new(); + edge.curve().approx(tolerance, &mut edge_points); + + let approx = approximate_edge(edge_points, edge.vertices()); + + points.extend(approx.points); + segments.extend(approx.segments); + } + + Approximation { points, segments } +} + #[cfg(test)] mod tests { use fj_math::{Point, Scalar, Segment}; diff --git a/fj-kernel/src/algorithms/sweep.rs b/fj-kernel/src/algorithms/sweep.rs index 1b2bd3542..36097998b 100644 --- a/fj-kernel/src/algorithms/sweep.rs +++ b/fj-kernel/src/algorithms/sweep.rs @@ -8,7 +8,7 @@ use crate::{ topology::{Cycle, Edge, Face, Vertex}, }; -use super::approximation::Approximation; +use super::approximation::approximate_cycle; /// Create a new shape by sweeping an existing one pub fn sweep_shape( @@ -144,8 +144,7 @@ pub fn sweep_shape( // This is the last piece of code that still uses the triangle // representation. - let approx = - Approximation::for_cycle(&cycle_source.get(), tolerance); + let approx = approximate_cycle(&cycle_source.get(), tolerance); let mut quads = Vec::new(); for segment in approx.segments { From 6854755be4a95493052749dd2b965d87a55b7f5f Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 22 Mar 2022 15:10:14 +0100 Subject: [PATCH 08/13] Simplify `approximate_edge` --- fj-kernel/src/algorithms/approximation.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index 698497a26..0dcb166c0 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -73,18 +73,17 @@ fn approximate_edge( points.push(b.point()); } - let mut segment_points = points.clone(); if vertices.is_none() { // The edge has no vertices, which means it connects to itself. We need // to reflect that in the approximation. if let Some(&point) = points.first() { - segment_points.push(point); + points.push(point); } } let mut segments = HashSet::new(); - for segment in segment_points.windows(2) { + for segment in points.windows(2) { let p0 = segment[0]; let p1 = segment[1]; From 6d50d25146f46d22e5fe80552d11e37e60188997 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 22 Mar 2022 15:13:21 +0100 Subject: [PATCH 09/13] Simplify `approximate_edge` --- fj-kernel/src/algorithms/approximation.rs | 46 ++++++++--------------- 1 file changed, 15 insertions(+), 31 deletions(-) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index 0dcb166c0..97f397731 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -59,7 +59,7 @@ impl Approximation { fn approximate_edge( mut points: Vec>, vertices: Option<[Vertex; 2]>, -) -> Approximation { +) -> Vec> { // Insert the exact vertices of this edge into the approximation. This means // we don't rely on the curve approximation to deliver accurate // representations of these vertices, which they might not be able to do. @@ -82,18 +82,7 @@ fn approximate_edge( } } - let mut segments = HashSet::new(); - for segment in points.windows(2) { - let p0 = segment[0]; - let p1 = segment[1]; - - segments.insert(Segment::from([p0, p1])); - } - - Approximation { - points: points.into_iter().collect(), - segments, - } + points } /// Compute an approximation for a cycle @@ -108,10 +97,18 @@ pub fn approximate_cycle(cycle: &Cycle, tolerance: Scalar) -> Approximation { let mut edge_points = Vec::new(); edge.curve().approx(tolerance, &mut edge_points); - let approx = approximate_edge(edge_points, edge.vertices()); + let edge_points = approximate_edge(edge_points, edge.vertices()); - points.extend(approx.points); - segments.extend(approx.segments); + let mut edge_segments = Vec::new(); + for segment in edge_points.windows(2) { + let p0 = segment[0]; + let p1 = segment[1]; + + edge_segments.push(Segment::from([p0, p1])); + } + + points.extend(edge_points); + segments.extend(edge_segments); } Approximation { points, segments } @@ -153,24 +150,11 @@ mod tests { points.clone(), Some([v1.get().clone(), v2.get().clone()]) ), - Approximation { - points: set![a, b, c, d], - segments: set![ - Segment::from([a, b]), - Segment::from([b, c]), - Segment::from([c, d]), - ], - } + vec![a, b, c, d], ); // Continuous edge - assert_eq!( - super::approximate_edge(points, None), - Approximation { - points: set![b, c], - segments: set![Segment::from([b, c]), Segment::from([c, b])], - } - ); + assert_eq!(super::approximate_edge(points, None), vec![b, c, b],); } #[test] From d5876b9650cf3f186ca2e1861cc0e19a28197e41 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 22 Mar 2022 15:28:25 +0100 Subject: [PATCH 10/13] Simplify `approximate_cycle` --- fj-kernel/src/algorithms/approximation.rs | 32 +++++++++++------------ fj-kernel/src/algorithms/sweep.rs | 6 +++-- 2 files changed, 20 insertions(+), 18 deletions(-) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index 97f397731..1d2a57dbc 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -46,10 +46,18 @@ impl Approximation { let mut segments = HashSet::new(); for cycle in face.cycles() { - let approx = approximate_cycle(&cycle, tolerance); + let cycle_points = approximate_cycle(&cycle, tolerance); - points.extend(approx.points); - segments.extend(approx.segments); + let mut cycle_segments = Vec::new(); + for segment in cycle_points.windows(2) { + let p0 = segment[0]; + let p1 = segment[1]; + + cycle_segments.push(Segment::from([p0, p1])); + } + + points.extend(cycle_points); + segments.extend(cycle_segments); } Self { points, segments } @@ -89,9 +97,8 @@ fn approximate_edge( /// /// `tolerance` defines how far the approximation is allowed to deviate from the /// actual cycle. -pub fn approximate_cycle(cycle: &Cycle, tolerance: Scalar) -> Approximation { - let mut points = HashSet::new(); - let mut segments = HashSet::new(); +pub fn approximate_cycle(cycle: &Cycle, tolerance: Scalar) -> Vec> { + let mut points = Vec::new(); for edge in cycle.edges() { let mut edge_points = Vec::new(); @@ -99,19 +106,12 @@ pub fn approximate_cycle(cycle: &Cycle, tolerance: Scalar) -> Approximation { let edge_points = approximate_edge(edge_points, edge.vertices()); - let mut edge_segments = Vec::new(); - for segment in edge_points.windows(2) { - let p0 = segment[0]; - let p1 = segment[1]; - - edge_segments.push(Segment::from([p0, p1])); - } - points.extend(edge_points); - segments.extend(edge_segments); } - Approximation { points, segments } + points.dedup(); + + points } #[cfg(test)] diff --git a/fj-kernel/src/algorithms/sweep.rs b/fj-kernel/src/algorithms/sweep.rs index 36097998b..425351aa9 100644 --- a/fj-kernel/src/algorithms/sweep.rs +++ b/fj-kernel/src/algorithms/sweep.rs @@ -1,6 +1,6 @@ use std::collections::HashMap; -use fj_math::{Scalar, Transform, Triangle, Vector}; +use fj_math::{Scalar, Segment, Transform, Triangle, Vector}; use crate::{ geometry::{Surface, SweptCurve}, @@ -147,7 +147,9 @@ pub fn sweep_shape( let approx = approximate_cycle(&cycle_source.get(), tolerance); let mut quads = Vec::new(); - for segment in approx.segments { + for segment in approx.windows(2) { + let segment = Segment::from_points([segment[0], segment[1]]); + let [v0, v1] = segment.points(); let [v3, v2] = { let segment = Transform::translation(path) From 3437463070570eb94797ba262c5fcc4dc6a1258c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 22 Mar 2022 15:29:29 +0100 Subject: [PATCH 11/13] Rename method It's the only constructor left, so it can have the traditional name of a constructor without loss of clarity. --- fj-kernel/src/algorithms/approximation.rs | 4 ++-- fj-kernel/src/algorithms/triangulation.rs | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index 1d2a57dbc..906b9e409 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -28,7 +28,7 @@ impl Approximation { /// /// `tolerance` defines how far the approximation is allowed to deviate from /// the actual edges. - pub fn for_face(face: &Face, tolerance: Scalar) -> Self { + pub fn new(face: &Face, tolerance: Scalar) -> Self { // Curved faces whose curvature is not fully defined by their edges // are not supported yet. For that reason, we can fully ignore `face`'s // `surface` field and just pass the edges to `Self::for_edges`. @@ -203,7 +203,7 @@ mod tests { }; assert_eq!( - Approximation::for_face(&face, tolerance), + Approximation::new(&face, tolerance), Approximation { points: set![a, b, c, d], segments: set![ diff --git a/fj-kernel/src/algorithms/triangulation.rs b/fj-kernel/src/algorithms/triangulation.rs index e2daeffa1..26db6c55d 100644 --- a/fj-kernel/src/algorithms/triangulation.rs +++ b/fj-kernel/src/algorithms/triangulation.rs @@ -25,7 +25,7 @@ pub fn triangulate( match &*face { Face::Face { surface, color, .. } => { let surface = surface.get(); - let approx = Approximation::for_face(&face, tolerance); + let approx = Approximation::new(&face, tolerance); let points: Vec<_> = approx .points From ce6f08042453c23177be9cda02a587fa8ea1b650 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 22 Mar 2022 15:31:25 +0100 Subject: [PATCH 12/13] Update documentation of `Approximation` --- fj-kernel/src/algorithms/approximation.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index 906b9e409..b631b4435 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -4,7 +4,7 @@ use fj_math::{Point, Scalar, Segment}; use crate::topology::{Cycle, Face, Vertex}; -/// An approximation of an edge, multiple edges, or a face +/// The approximation of a face #[derive(Debug, PartialEq)] pub struct Approximation { /// All points that make up the approximation @@ -24,10 +24,10 @@ pub struct Approximation { } impl Approximation { - /// Compute an approximation for a face + /// Compute the approximation of a face /// /// `tolerance` defines how far the approximation is allowed to deviate from - /// the actual edges. + /// the actual face. pub fn new(face: &Face, tolerance: Scalar) -> Self { // Curved faces whose curvature is not fully defined by their edges // are not supported yet. For that reason, we can fully ignore `face`'s From f20451bec35096738044b1dcc7a48d647083b6ed Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 22 Mar 2022 15:43:30 +0100 Subject: [PATCH 13/13] Refactor --- fj-kernel/src/algorithms/approximation.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/fj-kernel/src/algorithms/approximation.rs b/fj-kernel/src/algorithms/approximation.rs index b631b4435..90b8e9a70 100644 --- a/fj-kernel/src/algorithms/approximation.rs +++ b/fj-kernel/src/algorithms/approximation.rs @@ -104,9 +104,7 @@ pub fn approximate_cycle(cycle: &Cycle, tolerance: Scalar) -> Vec> { let mut edge_points = Vec::new(); edge.curve().approx(tolerance, &mut edge_points); - let edge_points = approximate_edge(edge_points, edge.vertices()); - - points.extend(edge_points); + points.extend(approximate_edge(edge_points, edge.vertices())); } points.dedup();