From e7c686af2d912772f0f1bf2f8f152248a7e48596 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 9 Mar 2023 14:20:03 +0100 Subject: [PATCH 01/11] Make variable name more specific --- crates/fj-kernel/src/builder/cycle.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index 2ee669fec..edbd2ec82 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -84,12 +84,12 @@ impl CycleBuilder for PartialCycle { O: ObjectArgument>, { edges.map_with_prev(|_, prev| { - let mut edge: Partial = Partial::new(objects); - edge.write().start_vertex = prev.read().start_vertex.clone(); + let mut half_edge: Partial = Partial::new(objects); + half_edge.write().start_vertex = prev.read().start_vertex.clone(); - self.add_half_edge(edge.clone()); + self.add_half_edge(half_edge.clone()); - edge + half_edge }) } } From 9fda0439fee9581282ff5dbd98598a1be3dac878 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 9 Mar 2023 14:46:02 +0100 Subject: [PATCH 02/11] Move code closer to where it's used --- crates/fj-kernel/src/algorithms/sweep/face.rs | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index 155cf5db6..7bb429fe2 100644 --- a/crates/fj-kernel/src/algorithms/sweep/face.rs +++ b/crates/fj-kernel/src/algorithms/sweep/face.rs @@ -64,12 +64,6 @@ impl Sweep for Handle { for (i, cycle) in bottom_face.all_cycles().cloned().enumerate() { let cycle = cycle.reverse(objects); - let mut top_cycle = if i == 0 { - top_face.exterior.clone() - } else { - top_face.add_interior(objects) - }; - let mut original_edges = Vec::new(); let mut top_edges = Vec::new(); for (half_edge, next) in @@ -89,6 +83,11 @@ impl Sweep for Handle { top_edges.push(Partial::from(top_edge)); } + let mut top_cycle = if i == 0 { + top_face.exterior.clone() + } else { + top_face.add_interior(objects) + }; top_cycle.write().connect_to_edges(top_edges, objects); for (bottom, top) in original_edges From 10033b961f5b41ec27cbd134bd37855ab7c2cf22 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 9 Mar 2023 14:50:16 +0100 Subject: [PATCH 03/11] Update builder method to not require mutation --- crates/fj-kernel/src/algorithms/sweep/face.rs | 24 ++++--------------- crates/fj-kernel/src/builder/cycle.rs | 16 +++++++++---- 2 files changed, 16 insertions(+), 24 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index 7bb429fe2..c3f0757ed 100644 --- a/crates/fj-kernel/src/algorithms/sweep/face.rs +++ b/crates/fj-kernel/src/algorithms/sweep/face.rs @@ -1,6 +1,5 @@ use std::ops::Deref; -use fj_interop::ext::ArrayExt; use fj_math::{Scalar, Vector}; use itertools::Itertools; @@ -64,7 +63,6 @@ impl Sweep for Handle { for (i, cycle) in bottom_face.all_cycles().cloned().enumerate() { let cycle = cycle.reverse(objects); - let mut original_edges = Vec::new(); let mut top_edges = Vec::new(); for (half_edge, next) in cycle.half_edges().cloned().circular_tuple_windows() @@ -79,8 +77,11 @@ impl Sweep for Handle { faces.push(face); - original_edges.push(half_edge); - top_edges.push(Partial::from(top_edge)); + top_edges.push(( + Partial::from(top_edge), + half_edge.curve(), + half_edge.boundary(), + )); } let mut top_cycle = if i == 0 { @@ -89,21 +90,6 @@ impl Sweep for Handle { top_face.add_interior(objects) }; top_cycle.write().connect_to_edges(top_edges, objects); - - for (bottom, top) in original_edges - .into_iter() - .zip(top_cycle.write().half_edges.iter_mut()) - { - top.write().curve = Some(bottom.curve()); - - let boundary = bottom.boundary(); - - for (top, bottom) in - top.write().boundary.each_mut_ext().zip_ext(boundary) - { - *top = Some(bottom); - } - } } let top_face = top_face.build(objects).insert(objects); diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index edbd2ec82..758065f86 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -1,6 +1,7 @@ use fj_math::Point; use crate::{ + geometry::curve::Curve, objects::{HalfEdge, Objects}, partial::{Partial, PartialCycle, PartialHalfEdge}, services::Service, @@ -43,7 +44,7 @@ pub trait CycleBuilder { objects: &mut Service, ) -> O::SameSize> where - O: ObjectArgument>; + O: ObjectArgument<(Partial, Curve, [Point<1>; 2])>; } impl CycleBuilder for PartialCycle { @@ -81,11 +82,16 @@ impl CycleBuilder for PartialCycle { objects: &mut Service, ) -> O::SameSize> where - O: ObjectArgument>, + O: ObjectArgument<(Partial, Curve, [Point<1>; 2])>, { - edges.map_with_prev(|_, prev| { - let mut half_edge: Partial = Partial::new(objects); - half_edge.write().start_vertex = prev.read().start_vertex.clone(); + edges.map_with_prev(|(_, curve, boundary), (prev, _, _)| { + let half_edge = PartialHalfEdge::make_half_edge( + curve, + boundary, + Some(prev.read().start_vertex.clone()), + None, + objects, + ); self.add_half_edge(half_edge.clone()); From 9547a963d85577f50e29a124dfb95716d92de567 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 9 Mar 2023 14:51:42 +0100 Subject: [PATCH 04/11] Remove unused code --- crates/fj-kernel/src/partial/objects/edge.rs | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) diff --git a/crates/fj-kernel/src/partial/objects/edge.rs b/crates/fj-kernel/src/partial/objects/edge.rs index ae6165fa4..e41ebe7d0 100644 --- a/crates/fj-kernel/src/partial/objects/edge.rs +++ b/crates/fj-kernel/src/partial/objects/edge.rs @@ -2,7 +2,6 @@ use fj_math::Point; use crate::{ geometry::curve::Curve, - insert::Insert, objects::{GlobalEdge, HalfEdge, Objects, Vertex}, partial::{FullToPartialCache, PartialObject}, services::Service, @@ -43,13 +42,10 @@ impl PartialHalfEdge { impl PartialObject for PartialHalfEdge { type Full = HalfEdge; - fn new(objects: &mut Service) -> Self { - Self { - curve: None, - boundary: [None; 2], - start_vertex: Vertex::new().insert(objects), - global_form: GlobalEdge::new().insert(objects), - } + fn new(_: &mut Service) -> Self { + // This method is no longer used, and since `PartialHalfEdge` will be + // replaced with `HalfEdge`, it will soon be removed. + unreachable!() } fn from_full(half_edge: &Self::Full, _: &mut FullToPartialCache) -> Self { From 8b6039a3d0f70c7cf6e1e86029c9807e42fc52bf Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 9 Mar 2023 14:53:03 +0100 Subject: [PATCH 05/11] Make `Curve` in `PartialHalfEdge` non-optional This makes it more similar to `HalfEdge`, preparing for an eventual merger. --- crates/fj-kernel/src/builder/edge.rs | 2 +- crates/fj-kernel/src/partial/objects/edge.rs | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index 7eb5625b7..e49cfcaf2 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -112,7 +112,7 @@ impl HalfEdgeBuilder for PartialHalfEdge { objects: &mut Service, ) -> Partial { Partial::from_partial(PartialHalfEdge { - curve: Some(curve), + curve, boundary: boundary.map(Some), start_vertex: start_vertex .unwrap_or_else(|| Vertex::new().insert(objects)), diff --git a/crates/fj-kernel/src/partial/objects/edge.rs b/crates/fj-kernel/src/partial/objects/edge.rs index e41ebe7d0..35f80da29 100644 --- a/crates/fj-kernel/src/partial/objects/edge.rs +++ b/crates/fj-kernel/src/partial/objects/edge.rs @@ -12,7 +12,7 @@ use crate::{ #[derive(Clone, Debug)] pub struct PartialHalfEdge { /// The curve that the half-edge is defined in - pub curve: Option, + pub curve: Curve, /// The boundary of the half-edge on the curve pub boundary: [Option>; 2], @@ -32,9 +32,9 @@ impl PartialHalfEdge { // could compute the surface position from slightly different data. let [start, _] = self.boundary; - start.and_then(|start| { - let curve = self.curve?; - Some(curve.point_from_path_coords(start)) + start.map(|start| { + let curve = self.curve; + curve.point_from_path_coords(start) }) } } @@ -50,7 +50,7 @@ impl PartialObject for PartialHalfEdge { fn from_full(half_edge: &Self::Full, _: &mut FullToPartialCache) -> Self { Self { - curve: Some(half_edge.curve()), + curve: half_edge.curve(), boundary: half_edge.boundary().map(Some), start_vertex: half_edge.start_vertex().clone(), global_form: half_edge.global_form().clone(), @@ -58,7 +58,7 @@ impl PartialObject for PartialHalfEdge { } fn build(self, _: &mut Service) -> Self::Full { - let curve = self.curve.expect("Need path to build curve"); + let curve = self.curve; let boundary = self.boundary.map(|point| { point.expect("Can't build `HalfEdge` without boundary positions") }); From 9efc962e75b18cf06f213e9515846c22b1d838b4 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 9 Mar 2023 14:55:05 +0100 Subject: [PATCH 06/11] Inline redundant variable --- crates/fj-kernel/src/partial/objects/edge.rs | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/crates/fj-kernel/src/partial/objects/edge.rs b/crates/fj-kernel/src/partial/objects/edge.rs index 35f80da29..8d0842801 100644 --- a/crates/fj-kernel/src/partial/objects/edge.rs +++ b/crates/fj-kernel/src/partial/objects/edge.rs @@ -32,10 +32,7 @@ impl PartialHalfEdge { // could compute the surface position from slightly different data. let [start, _] = self.boundary; - start.map(|start| { - let curve = self.curve; - curve.point_from_path_coords(start) - }) + start.map(|start| self.curve.point_from_path_coords(start)) } } From a97f5ee7436196df6836d255d08697f54afb38c0 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 9 Mar 2023 14:55:55 +0100 Subject: [PATCH 07/11] Inline redundant variable --- crates/fj-kernel/src/partial/objects/edge.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/partial/objects/edge.rs b/crates/fj-kernel/src/partial/objects/edge.rs index 8d0842801..54c37bafc 100644 --- a/crates/fj-kernel/src/partial/objects/edge.rs +++ b/crates/fj-kernel/src/partial/objects/edge.rs @@ -55,11 +55,10 @@ impl PartialObject for PartialHalfEdge { } fn build(self, _: &mut Service) -> Self::Full { - let curve = self.curve; let boundary = self.boundary.map(|point| { point.expect("Can't build `HalfEdge` without boundary positions") }); - HalfEdge::new(curve, boundary, self.start_vertex, self.global_form) + HalfEdge::new(self.curve, boundary, self.start_vertex, self.global_form) } } From 3a0396c6068c87e50192f1c781beba00cc137856 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 9 Mar 2023 14:58:16 +0100 Subject: [PATCH 08/11] Make `boundary` in `PartialHalfEdge` non-optional This makes it more similar to `HalfEdge`, preparing for an eventual merger. --- crates/fj-kernel/src/algorithms/approx/edge.rs | 4 ++-- crates/fj-kernel/src/builder/edge.rs | 2 +- crates/fj-kernel/src/partial/objects/edge.rs | 10 ++++------ 3 files changed, 7 insertions(+), 9 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index 76988c7c4..9f3ebe38b 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -346,8 +346,8 @@ mod tests { &mut services.objects, ); - half_edge.write().boundary[0] = Some(range.boundary[0]); - half_edge.write().boundary[1] = Some(range.boundary[1]); + half_edge.write().boundary[0] = range.boundary[0]; + half_edge.write().boundary[1] = range.boundary[1]; let half_edge = half_edge.read().clone(); half_edge diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index e49cfcaf2..be395df81 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -113,7 +113,7 @@ impl HalfEdgeBuilder for PartialHalfEdge { ) -> Partial { Partial::from_partial(PartialHalfEdge { curve, - boundary: boundary.map(Some), + boundary, start_vertex: start_vertex .unwrap_or_else(|| Vertex::new().insert(objects)), global_form: global_form diff --git a/crates/fj-kernel/src/partial/objects/edge.rs b/crates/fj-kernel/src/partial/objects/edge.rs index 54c37bafc..72310b7f1 100644 --- a/crates/fj-kernel/src/partial/objects/edge.rs +++ b/crates/fj-kernel/src/partial/objects/edge.rs @@ -15,7 +15,7 @@ pub struct PartialHalfEdge { pub curve: Curve, /// The boundary of the half-edge on the curve - pub boundary: [Option>; 2], + pub boundary: [Point<1>; 2], /// The surface vertex where the half-edge starts pub start_vertex: Handle, @@ -32,7 +32,7 @@ impl PartialHalfEdge { // could compute the surface position from slightly different data. let [start, _] = self.boundary; - start.map(|start| self.curve.point_from_path_coords(start)) + Some(self.curve.point_from_path_coords(start)) } } @@ -48,16 +48,14 @@ impl PartialObject for PartialHalfEdge { fn from_full(half_edge: &Self::Full, _: &mut FullToPartialCache) -> Self { Self { curve: half_edge.curve(), - boundary: half_edge.boundary().map(Some), + boundary: half_edge.boundary(), start_vertex: half_edge.start_vertex().clone(), global_form: half_edge.global_form().clone(), } } fn build(self, _: &mut Service) -> Self::Full { - let boundary = self.boundary.map(|point| { - point.expect("Can't build `HalfEdge` without boundary positions") - }); + let boundary = self.boundary; HalfEdge::new(self.curve, boundary, self.start_vertex, self.global_form) } From 3bbb2c0fa930943f4a0107c3501aa9bccca6fb4c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 9 Mar 2023 14:59:10 +0100 Subject: [PATCH 09/11] Inline redundant variable --- crates/fj-kernel/src/partial/objects/edge.rs | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/partial/objects/edge.rs b/crates/fj-kernel/src/partial/objects/edge.rs index 72310b7f1..19cf922cc 100644 --- a/crates/fj-kernel/src/partial/objects/edge.rs +++ b/crates/fj-kernel/src/partial/objects/edge.rs @@ -55,8 +55,11 @@ impl PartialObject for PartialHalfEdge { } fn build(self, _: &mut Service) -> Self::Full { - let boundary = self.boundary; - - HalfEdge::new(self.curve, boundary, self.start_vertex, self.global_form) + HalfEdge::new( + self.curve, + self.boundary, + self.start_vertex, + self.global_form, + ) } } From 1c160106a5e85085478936279beed7a76480d978 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 9 Mar 2023 15:00:00 +0100 Subject: [PATCH 10/11] Remove unused method --- crates/fj-kernel/src/partial/objects/edge.rs | 12 ------------ 1 file changed, 12 deletions(-) diff --git a/crates/fj-kernel/src/partial/objects/edge.rs b/crates/fj-kernel/src/partial/objects/edge.rs index 19cf922cc..7f637590e 100644 --- a/crates/fj-kernel/src/partial/objects/edge.rs +++ b/crates/fj-kernel/src/partial/objects/edge.rs @@ -24,18 +24,6 @@ pub struct PartialHalfEdge { pub global_form: Handle, } -impl PartialHalfEdge { - /// Compute the surface position where the half-edge starts - pub fn start_position(&self) -> Option> { - // Computing the surface position from the curve position is fine. - // `HalfEdge` "owns" its start position. There is no competing code that - // could compute the surface position from slightly different data. - - let [start, _] = self.boundary; - Some(self.curve.point_from_path_coords(start)) - } -} - impl PartialObject for PartialHalfEdge { type Full = HalfEdge; From 5427b55c08dd74ca38c1db5adb6aa405bfc447a2 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 9 Mar 2023 16:06:41 +0100 Subject: [PATCH 11/11] Refactor test to simplify it --- .../fj-kernel/src/algorithms/approx/edge.rs | 24 +++++++------------ 1 file changed, 8 insertions(+), 16 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index 9f3ebe38b..7d5e467b5 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -338,22 +338,14 @@ mod tests { v: [0., 0., 1.].into(), }) .insert(&mut services.objects); - let half_edge = { - let mut cycle = PartialCycle::new(&mut services.objects); - - let [mut half_edge, _, _] = cycle.update_as_polygon_from_points( - [[0., 1.], [1., 1.], [1., 2.]], - &mut services.objects, - ); - - half_edge.write().boundary[0] = range.boundary[0]; - half_edge.write().boundary[1] = range.boundary[1]; - - let half_edge = half_edge.read().clone(); - half_edge - .build(&mut services.objects) - .insert(&mut services.objects) - }; + let half_edge = PartialHalfEdge::make_line_segment( + [[0., 1.], [TAU, 1.]], + Some(range.boundary), + None, + None, + &mut services.objects, + ) + .build(&mut services.objects); let tolerance = 1.; let approx = (&half_edge, surface.deref()).approx(tolerance);