From 0eb9a9efc6a69706bf0c38ed97ef29054afd055f Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 11:32:04 +0200 Subject: [PATCH 1/5] Add `RangeOnCurve::new` --- crates/fj-kernel/src/algorithms/approx/curve.rs | 5 +++++ crates/fj-kernel/src/algorithms/approx/edge.rs | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/crates/fj-kernel/src/algorithms/approx/curve.rs b/crates/fj-kernel/src/algorithms/approx/curve.rs index c0e7dafa2..fecbd7a04 100644 --- a/crates/fj-kernel/src/algorithms/approx/curve.rs +++ b/crates/fj-kernel/src/algorithms/approx/curve.rs @@ -112,6 +112,11 @@ pub struct RangeOnCurve { } impl RangeOnCurve { + /// Construct an instance of `RangeOnCurve` + pub fn new(boundary: [Vertex; 2]) -> Self { + Self { boundary } + } + /// Access the start of the range pub fn start(&self) -> Vertex { self.boundary[0] diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index e5de04683..e0001bd81 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -72,7 +72,7 @@ impl Approx for &Edge { } }; - let range = RangeOnCurve { boundary }; + let range = RangeOnCurve::new(boundary); let first = ApproxPoint::new( range.start().surface_form().position(), From d8745d4abaef0775d20ffcc3d71af02a23fdfe82 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 11:32:24 +0200 Subject: [PATCH 2/5] Remove unnecessary `pub` --- crates/fj-kernel/src/algorithms/approx/curve.rs | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/curve.rs b/crates/fj-kernel/src/algorithms/approx/curve.rs index fecbd7a04..207210cb3 100644 --- a/crates/fj-kernel/src/algorithms/approx/curve.rs +++ b/crates/fj-kernel/src/algorithms/approx/curve.rs @@ -104,11 +104,7 @@ fn number_of_vertices_for_circle( /// The range on which a curve should be approximated #[derive(Clone, Copy, Debug)] pub struct RangeOnCurve { - /// The boundary of the range - /// - /// The vertices that make up the boundary are themselves not included in - /// the approximation. - pub boundary: [Vertex; 2], + boundary: [Vertex; 2], } impl RangeOnCurve { From 8829029345f2c4df8bdf504b20943ec45a16949c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 11:40:49 +0200 Subject: [PATCH 3/5] Simplify function signature --- crates/fj-kernel/src/algorithms/approx/curve.rs | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/curve.rs b/crates/fj-kernel/src/algorithms/approx/curve.rs index 207210cb3..a77cf749a 100644 --- a/crates/fj-kernel/src/algorithms/approx/curve.rs +++ b/crates/fj-kernel/src/algorithms/approx/curve.rs @@ -44,16 +44,10 @@ impl Approx for (&GlobalCurve, RangeOnCurve) { fn approx(self, tolerance: Tolerance) -> Self::Approximation { let (curve, range) = self; - let mut points = Vec::new(); - match curve.kind() { - CurveKind::Circle(curve) => { - approx_circle(curve, range, tolerance, &mut points); - } - CurveKind::Line(_) => {} + CurveKind::Circle(curve) => approx_circle(curve, range, tolerance), + CurveKind::Line(_) => vec![], } - - points } } @@ -65,8 +59,9 @@ fn approx_circle( circle: &Circle<3>, range: impl Into, tolerance: Tolerance, - points: &mut Vec>, -) { +) -> Vec> { + let mut points = Vec::new(); + let radius = circle.a().magnitude(); let range = range.into(); @@ -87,6 +82,8 @@ fn approx_circle( points.push(ApproxPoint::new(point_curve, point_global)); } + + points } fn number_of_vertices_for_circle( From 5992e3ae49b67a93980399d2d42852a31697edab Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 11:45:00 +0200 Subject: [PATCH 4/5] Simplify code This version is simpler, because it doesn't rely on the behavior of `Range::new`. In fact, this behavior is going to change, which is the reason for this commit. --- crates/fj-kernel/src/algorithms/approx/edge.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index e0001bd81..34dc13c28 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -19,7 +19,7 @@ impl Approx for &Edge { fn approx(self, tolerance: super::Tolerance) -> Self::Approximation { // The range is only used for circles right now. - let boundary = match self.vertices().get() { + let [a, b] = match self.vertices().get() { Some(vertices) => vertices.map(|&vertex| vertex), None => { // Creating vertices from nothing, just for the sake of @@ -72,11 +72,11 @@ impl Approx for &Edge { } }; - let range = RangeOnCurve::new(boundary); + let range = RangeOnCurve::new([a, b]); let first = ApproxPoint::new( - range.start().surface_form().position(), - range.start().global_form().position(), + a.surface_form().position(), + a.global_form().position(), ); let curve_approx = (self.curve(), range).approx(tolerance); From f462a3e2f7f2e12a73781defec90ad7be4bab181 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 11:45:40 +0200 Subject: [PATCH 5/5] Fix inconsistency in curve approximation Curve approximation could result in slightly different points, depending on the direction of the range. --- .../fj-kernel/src/algorithms/approx/curve.rs | 36 +++++++++++++++++-- 1 file changed, 34 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/curve.rs b/crates/fj-kernel/src/algorithms/approx/curve.rs index a77cf749a..ba4169ffa 100644 --- a/crates/fj-kernel/src/algorithms/approx/curve.rs +++ b/crates/fj-kernel/src/algorithms/approx/curve.rs @@ -83,6 +83,10 @@ fn approx_circle( points.push(ApproxPoint::new(point_curve, point_global)); } + if range.is_reversed() { + points.reverse(); + } + points } @@ -102,12 +106,40 @@ fn number_of_vertices_for_circle( #[derive(Clone, Copy, Debug)] pub struct RangeOnCurve { boundary: [Vertex; 2], + is_reversed: bool, } impl RangeOnCurve { /// Construct an instance of `RangeOnCurve` - pub fn new(boundary: [Vertex; 2]) -> Self { - Self { boundary } + /// + /// Ranges are normalized on construction, meaning that the order of + /// vertices passed to this constructor does not influence the range that is + /// constructed. + /// + /// This is done to prevent bugs during mesh construction: The curve + /// approximation code is regularly faced with ranges that are reversed + /// versions of each other. This can lead to slightly different + /// approximations, which in turn leads to the aforementioned invalid + /// meshes. + /// + /// The caller can use `is_reversed` to determine, if the range was reversed + /// during normalization, to adjust the approximation accordingly. + pub fn new([a, b]: [Vertex; 2]) -> Self { + let (boundary, is_reversed) = if a < b { + ([a, b], false) + } else { + ([b, a], true) + }; + + Self { + boundary, + is_reversed, + } + } + + /// Indicate whether the range was reversed during normalization + pub fn is_reversed(&self) -> bool { + self.is_reversed } /// Access the start of the range