From 7df3c54d687c1da1c42f97250292c359e3088b6a Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Aug 2022 20:10:12 +0200 Subject: [PATCH 1/7] Don't include end point in curve/edge approx Since there is always going to be a next edge in the cycle, whose first point is the previous edge's last point, there is no need to include the last point in the approximation in the first place. --- crates/fj-kernel/src/algorithms/approx/cycle.rs | 6 +++--- crates/fj-kernel/src/algorithms/approx/edge.rs | 17 +++-------------- 2 files changed, 6 insertions(+), 17 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/cycle.rs b/crates/fj-kernel/src/algorithms/approx/cycle.rs index 855e4102e..ae1625848 100644 --- a/crates/fj-kernel/src/algorithms/approx/cycle.rs +++ b/crates/fj-kernel/src/algorithms/approx/cycle.rs @@ -27,9 +27,9 @@ impl Approx for Cycle { })); } - // Can't just rely on `dedup`, as the conversion from curve coordinates - // could lead to subtly different surface coordinates. - points.dedup_by(|(_, a), (_, b)| a == b); + if let Some(&point) = points.first() { + points.push(point); + } CycleApprox { points } } diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index f49132bcc..6286d5f96 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -40,18 +40,8 @@ pub fn approx_edge( // vertices. let vertices = vertices .convert(|vertex| (vertex.position(), vertex.global().position())); - if let Some([a, b]) = vertices { + if let Some([a, _]) = vertices { points.insert(0, a); - points.push(b); - } - - 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() { - points.push(point); - } } } @@ -79,16 +69,15 @@ mod test { let a = (Point::from([0.0]), a); let b = (Point::from([0.25]), b); let c = (Point::from([0.75]), c); - let d = (Point::from([1.0]), d); // Regular edge let mut points = vec![b, c]; super::approx_edge(vertices, &mut points); - assert_eq!(points, vec![a, b, c, d]); + assert_eq!(points, vec![a, b, c]); // Continuous edge let mut points = vec![b, c]; super::approx_edge(VerticesOfEdge::none(), &mut points); - assert_eq!(points, vec![b, c, b]); + assert_eq!(points, vec![b, c]); } } From a0dd0fd19629b47c7f1f5529c08999e97d63c9c9 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Aug 2022 20:24:20 +0200 Subject: [PATCH 2/7] Inline function The function existed only for the test, but now I'm not sure how to uphold its properties that made it testable going forward. I think just removing the test is an okay solution for now, since there are higher-level tests covering the same code. --- .../fj-kernel/src/algorithms/approx/edge.rs | 75 +++++-------------- 1 file changed, 17 insertions(+), 58 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index 6286d5f96..fa4939b4d 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -1,6 +1,6 @@ use fj_math::{Point, Scalar}; -use crate::objects::{Edge, Vertex, VerticesOfEdge}; +use crate::objects::Edge; use super::{curve::RangeOnCurve, Approx}; @@ -20,64 +20,23 @@ impl Approx for Edge { boundary: [[Scalar::ZERO].into(), [Scalar::TAU].into()], }, ); - approx_edge(*self.vertices(), &mut points); - points - } -} - -pub fn approx_edge( - vertices: VerticesOfEdge, - points: &mut Vec<(Point<1>, Point<3>)>, -) { - // 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. - // - // If we used inaccurate representations of those vertices here, then that - // would lead to bugs in the approximation, as points that should refer to - // the same vertex would be understood to refer to very close, but distinct - // vertices. - let vertices = vertices - .convert(|vertex| (vertex.position(), vertex.global().position())); - if let Some([a, _]) = vertices { - points.insert(0, a); - } -} - -#[cfg(test)] -mod test { - use fj_math::Point; + // 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. + // + // If we used inaccurate representations of those vertices here, then + // that would lead to bugs in the approximation, as points that should + // refer to the same vertex would be understood to refer to very close, + // but distinct vertices. + let vertices = self + .vertices() + .convert(|vertex| (vertex.position(), vertex.global().position())); + if let Some([a, _]) = vertices { + points.insert(0, a); + } - use crate::objects::{GlobalVertex, Vertex, VerticesOfEdge}; - - #[test] - fn approx_edge() { - let a = Point::from([1., 2., 3.]); - let b = Point::from([2., 3., 5.]); - let c = Point::from([3., 5., 8.]); - let d = Point::from([5., 8., 13.]); - - let v1 = GlobalVertex::from_position(a); - let v2 = GlobalVertex::from_position(d); - - let vertices = VerticesOfEdge::from_vertices([ - Vertex::new(Point::from([0.]), v1), - Vertex::new(Point::from([1.]), v2), - ]); - - let a = (Point::from([0.0]), a); - let b = (Point::from([0.25]), b); - let c = (Point::from([0.75]), c); - - // Regular edge - let mut points = vec![b, c]; - super::approx_edge(vertices, &mut points); - assert_eq!(points, vec![a, b, c]); - - // Continuous edge - let mut points = vec![b, c]; - super::approx_edge(VerticesOfEdge::none(), &mut points); - assert_eq!(points, vec![b, c]); + points } } From 53b04e531119da434f5c9f6c52a9d907c2247fab Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Aug 2022 20:28:47 +0200 Subject: [PATCH 3/7] Shuffle responsibilities between cycle/edge approx This serves no purpose, except making further changes I have in mind easier. --- crates/fj-kernel/src/algorithms/approx/curve.rs | 12 ++++++++++-- crates/fj-kernel/src/algorithms/approx/cycle.rs | 9 +-------- crates/fj-kernel/src/algorithms/approx/edge.rs | 8 +++++--- 3 files changed, 16 insertions(+), 13 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/curve.rs b/crates/fj-kernel/src/algorithms/approx/curve.rs index 33a99aa19..c83282718 100644 --- a/crates/fj-kernel/src/algorithms/approx/curve.rs +++ b/crates/fj-kernel/src/algorithms/approx/curve.rs @@ -7,7 +7,7 @@ use crate::objects::{Curve, CurveKind, GlobalCurve}; use super::{Approx, Tolerance}; impl Approx for Curve { - type Approximation = Vec<(Point<1>, Point<3>)>; + type Approximation = Vec<(Point<2>, Point<3>)>; type Params = RangeOnCurve; fn approx( @@ -15,7 +15,15 @@ impl Approx for Curve { tolerance: Tolerance, range: Self::Params, ) -> Self::Approximation { - self.global().approx(tolerance, range) + self.global() + .approx(tolerance, range) + .into_iter() + .map(|(point_curve, point_global)| { + let point_surface = + self.kind().point_from_curve_coords(point_curve); + (point_surface, point_global) + }) + .collect() } } diff --git a/crates/fj-kernel/src/algorithms/approx/cycle.rs b/crates/fj-kernel/src/algorithms/approx/cycle.rs index ae1625848..6c373609c 100644 --- a/crates/fj-kernel/src/algorithms/approx/cycle.rs +++ b/crates/fj-kernel/src/algorithms/approx/cycle.rs @@ -17,14 +17,7 @@ impl Approx for Cycle { for edge in self.edges() { let edge_points = edge.approx(tolerance, ()); - - points.extend(edge_points.into_iter().map(|point| { - let (point_curve, point_global) = point; - - let point_surface = - edge.curve().kind().point_from_curve_coords(point_curve); - (point_surface, point_global) - })); + points.extend(edge_points); } if let Some(&point) = points.first() { diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index fa4939b4d..db9ac2388 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -5,7 +5,7 @@ use crate::objects::Edge; use super::{curve::RangeOnCurve, Approx}; impl Approx for Edge { - type Approximation = Vec<(Point<1>, Point<3>)>; + type Approximation = Vec<(Point<2>, Point<3>)>; type Params = (); fn approx( @@ -33,8 +33,10 @@ impl Approx for Edge { let vertices = self .vertices() .convert(|vertex| (vertex.position(), vertex.global().position())); - if let Some([a, _]) = vertices { - points.insert(0, a); + if let Some([(point_curve, point_global), _]) = vertices { + let point_surface = + self.curve().kind().point_from_curve_coords(point_curve); + points.insert(0, (point_surface, point_global)); } points From dee0659ff70143abe81558af2743c180a4dc52c7 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Aug 2022 20:34:28 +0200 Subject: [PATCH 4/7] Refactor --- crates/fj-kernel/src/algorithms/approx/edge.rs | 13 ++++++------- 1 file changed, 6 insertions(+), 7 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index db9ac2388..33cb1e449 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -13,13 +13,12 @@ impl Approx for Edge { tolerance: super::Tolerance, (): Self::Params, ) -> Self::Approximation { - let mut points = self.curve().approx( - tolerance, - // The range is only used for circles right now. - RangeOnCurve { - boundary: [[Scalar::ZERO].into(), [Scalar::TAU].into()], - }, - ); + // The range is only used for circles right now. + let range = RangeOnCurve { + boundary: [[Scalar::ZERO].into(), [Scalar::TAU].into()], + }; + + let mut points = self.curve().approx(tolerance, range); // Insert the exact vertices of this edge into the approximation. This // means we don't rely on the curve approximation to deliver accurate From 7b109d139e0d2f8134481f2ff825f12ec0644b95 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Aug 2022 20:36:13 +0200 Subject: [PATCH 5/7] Refactor --- crates/fj-kernel/src/algorithms/approx/edge.rs | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index 33cb1e449..f8ff5556e 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -14,8 +14,13 @@ impl Approx for Edge { (): Self::Params, ) -> Self::Approximation { // The range is only used for circles right now. - let range = RangeOnCurve { - boundary: [[Scalar::ZERO].into(), [Scalar::TAU].into()], + let range = { + let start_curve = Point::from([Scalar::ZERO]); + let end_curve = Point::from([Scalar::TAU]); + + RangeOnCurve { + boundary: [start_curve, end_curve], + } }; let mut points = self.curve().approx(tolerance, range); From be49a756d7f0bb5562c1d6395643f982815e5488 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 26 Aug 2022 14:14:15 +0200 Subject: [PATCH 6/7] Don't recompute point that is already available --- crates/fj-kernel/src/algorithms/approx/curve.rs | 13 +++++++------ crates/fj-kernel/src/algorithms/approx/edge.rs | 13 ++++++++++++- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/curve.rs b/crates/fj-kernel/src/algorithms/approx/curve.rs index c83282718..9ec6a211d 100644 --- a/crates/fj-kernel/src/algorithms/approx/curve.rs +++ b/crates/fj-kernel/src/algorithms/approx/curve.rs @@ -64,9 +64,10 @@ fn approx_circle( let n = number_of_vertices_for_circle(tolerance, radius, range.length()); let mut points = Vec::new(); + points.push(range.start()); - for i in 0..n { - let angle = range.start().t + for i in 1..n { + let angle = range.start().0.t + (Scalar::TAU / n as f64 * i as f64) * range.direction(); let point_curve = Point::from([angle]); @@ -91,20 +92,20 @@ fn number_of_vertices_for_circle( } pub struct RangeOnCurve { - pub boundary: [Point<1>; 2], + pub boundary: [(Point<1>, Point<3>); 2], } impl RangeOnCurve { - fn start(&self) -> Point<1> { + fn start(&self) -> (Point<1>, Point<3>) { self.boundary[0] } - fn end(&self) -> Point<1> { + fn end(&self) -> (Point<1>, Point<3>) { self.boundary[1] } fn signed_length(&self) -> Scalar { - (self.end() - self.start()).t + (self.end().0 - self.start().0).t } fn length(&self) -> Scalar { diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index f8ff5556e..1ac553167 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -18,8 +18,19 @@ impl Approx for Edge { let start_curve = Point::from([Scalar::ZERO]); let end_curve = Point::from([Scalar::TAU]); + // We're dealing with a circle here. Start and end are identical + // points, in global coordinates. + let point_global = self + .global() + .curve() + .kind() + .point_from_curve_coords(start_curve); + RangeOnCurve { - boundary: [start_curve, end_curve], + boundary: [ + (start_curve, point_global), + (end_curve, point_global), + ], } }; From 972c6352b77f50604703f5ed3ea2523be395db57 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Mon, 29 Aug 2022 20:40:57 +0200 Subject: [PATCH 7/7] Align circle and line approximation With this commit, all curve approximations work the same now, making edge approximation much simpler. --- .../fj-kernel/src/algorithms/approx/curve.rs | 2 +- .../fj-kernel/src/algorithms/approx/edge.rs | 22 +------------------ 2 files changed, 2 insertions(+), 22 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/curve.rs b/crates/fj-kernel/src/algorithms/approx/curve.rs index 9ec6a211d..783729e23 100644 --- a/crates/fj-kernel/src/algorithms/approx/curve.rs +++ b/crates/fj-kernel/src/algorithms/approx/curve.rs @@ -38,7 +38,7 @@ impl Approx for GlobalCurve { ) -> Self::Approximation { match self.kind() { CurveKind::Circle(curve) => approx_circle(curve, range, tolerance), - CurveKind::Line(_) => Vec::new(), + CurveKind::Line(_) => vec![range.start()], } } } diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index 1ac553167..cf76001e0 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -34,26 +34,6 @@ impl Approx for Edge { } }; - let mut points = self.curve().approx(tolerance, range); - - // 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. - // - // If we used inaccurate representations of those vertices here, then - // that would lead to bugs in the approximation, as points that should - // refer to the same vertex would be understood to refer to very close, - // but distinct vertices. - let vertices = self - .vertices() - .convert(|vertex| (vertex.position(), vertex.global().position())); - if let Some([(point_curve, point_global), _]) = vertices { - let point_surface = - self.curve().kind().point_from_curve_coords(point_curve); - points.insert(0, (point_surface, point_global)); - } - - points + self.curve().approx(tolerance, range) } }