From df515b869d16fad1d706f8ccba277ba4ec9a29ef Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 10 Mar 2023 12:36:06 +0100 Subject: [PATCH 01/11] Return added half-edge from `add_half_edge` This doesn't make a whole lot of sense as-is, but it's going to in a few commits. --- crates/fj-kernel/src/builder/cycle.rs | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index 9151c671c..c195051e0 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -22,7 +22,10 @@ pub trait CycleBuilder { /// /// If this is the first half-edge being added, it is connected to itself, /// meaning its front and back vertices are the same. - fn add_half_edge(&mut self, half_edge: Handle); + fn add_half_edge( + &mut self, + half_edge: Handle, + ) -> Handle; /// Update cycle as a polygon from the provided points fn update_as_polygon_from_points( @@ -50,8 +53,12 @@ pub trait CycleBuilder { } impl CycleBuilder for PartialCycle { - fn add_half_edge(&mut self, half_edge: Handle) { - self.half_edges.push(half_edge); + fn add_half_edge( + &mut self, + half_edge: Handle, + ) -> Handle { + self.half_edges.push(half_edge.clone()); + half_edge } fn update_as_polygon_from_points( From c5a83945b3c064c1626565b13b801ff30685e167 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 10 Mar 2023 12:38:28 +0100 Subject: [PATCH 02/11] Refactor code to simplify it --- crates/fj-kernel/src/builder/cycle.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index c195051e0..7258c26fd 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -75,9 +75,7 @@ impl CycleBuilder for PartialCycle { .build(objects) .insert(objects); - self.add_half_edge(half_edge.clone()); - - half_edge + self.add_half_edge(half_edge) }) } From 6079f9d784f5b1686f5569f23e6da098d0d58bcc Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 10 Mar 2023 12:39:26 +0100 Subject: [PATCH 03/11] Refactor code to simplify it --- crates/fj-kernel/src/algorithms/sweep/edge.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/edge.rs b/crates/fj-kernel/src/algorithms/sweep/edge.rs index 656a9d6b3..e0b40e62c 100644 --- a/crates/fj-kernel/src/algorithms/sweep/edge.rs +++ b/crates/fj-kernel/src/algorithms/sweep/edge.rs @@ -104,9 +104,7 @@ impl Sweep for (Handle, &Handle, &Surface, Color) { builder.build(objects).insert(objects) }; - face.exterior.write().add_half_edge(half_edge.clone()); - - half_edge + face.exterior.write().add_half_edge(half_edge) }); // And we're done creating the face! All that's left to do is build our From edda0519b05daa28a0b9cb384914e3c8d59c95d4 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 10 Mar 2023 12:40:30 +0100 Subject: [PATCH 04/11] Refactor code to simplify it --- crates/fj-kernel/src/builder/cycle.rs | 4 +--- 1 file changed, 1 insertion(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index 7258c26fd..2aaa54563 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -93,9 +93,7 @@ impl CycleBuilder for PartialCycle { .build(objects) .insert(objects); - self.add_half_edge(half_edge.clone()); - - half_edge + self.add_half_edge(half_edge) }) } } From 0809d87c9cb51e709023746b34ff81c9c1adf250 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 10 Mar 2023 12:41:41 +0100 Subject: [PATCH 05/11] Accept raw `HalfEdge` in `add_half_edge` Re-using an existing `HalfEdge` would be wrong anyway. This also meshes better with some upcoming changes I'm envisioning. --- crates/fj-kernel/src/algorithms/sweep/edge.rs | 4 ++-- crates/fj-kernel/src/builder/cycle.rs | 17 +++++++++-------- crates/fj-operations/src/sketch.rs | 5 ++--- 3 files changed, 13 insertions(+), 13 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/edge.rs b/crates/fj-kernel/src/algorithms/sweep/edge.rs index e0b40e62c..5c6883c5e 100644 --- a/crates/fj-kernel/src/algorithms/sweep/edge.rs +++ b/crates/fj-kernel/src/algorithms/sweep/edge.rs @@ -101,10 +101,10 @@ impl Sweep for (Handle, &Handle, &Surface, Color) { builder }; - builder.build(objects).insert(objects) + builder.build(objects) }; - face.exterior.write().add_half_edge(half_edge) + face.exterior.write().add_half_edge(half_edge, objects) }); // And we're done creating the face! All that's left to do is build our diff --git a/crates/fj-kernel/src/builder/cycle.rs b/crates/fj-kernel/src/builder/cycle.rs index 2aaa54563..296b74fa5 100644 --- a/crates/fj-kernel/src/builder/cycle.rs +++ b/crates/fj-kernel/src/builder/cycle.rs @@ -24,7 +24,8 @@ pub trait CycleBuilder { /// meaning its front and back vertices are the same. fn add_half_edge( &mut self, - half_edge: Handle, + half_edge: HalfEdge, + objects: &mut Service, ) -> Handle; /// Update cycle as a polygon from the provided points @@ -55,8 +56,10 @@ pub trait CycleBuilder { impl CycleBuilder for PartialCycle { fn add_half_edge( &mut self, - half_edge: Handle, + half_edge: HalfEdge, + objects: &mut Service, ) -> Handle { + let half_edge = half_edge.insert(objects); self.half_edges.push(half_edge.clone()); half_edge } @@ -72,10 +75,9 @@ impl CycleBuilder for PartialCycle { { points.map_with_next(|start, end| { let half_edge = HalfEdgeBuilder::line_segment([start, end], None) - .build(objects) - .insert(objects); + .build(objects); - self.add_half_edge(half_edge) + self.add_half_edge(half_edge, objects) }) } @@ -90,10 +92,9 @@ impl CycleBuilder for PartialCycle { edges.map_with_prev(|(_, curve, boundary), (prev, _, _)| { let half_edge = HalfEdgeBuilder::new(curve, boundary) .with_start_vertex(prev.start_vertex().clone()) - .build(objects) - .insert(objects); + .build(objects); - self.add_half_edge(half_edge) + self.add_half_edge(half_edge, objects) }) } } diff --git a/crates/fj-operations/src/sketch.rs b/crates/fj-operations/src/sketch.rs index aefee9fa8..7ef1f2f39 100644 --- a/crates/fj-operations/src/sketch.rs +++ b/crates/fj-operations/src/sketch.rs @@ -75,9 +75,8 @@ impl Shape for fj::Sketch { } }; - let half_edge = - half_edge.build(objects).insert(objects); - cycle.add_half_edge(half_edge); + let half_edge = half_edge.build(objects); + cycle.add_half_edge(half_edge, objects); } Partial::from_partial(cycle) From 1be65385fb4cf6207222fc6a38c736f003fe5648 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 10 Mar 2023 10:23:31 +0100 Subject: [PATCH 06/11] Remove approximation sources It's getting in the way of what I'm currently working on. Removing it is just easiest. It's not a lot of code, so it can be added back (in an improved form that then won't conflict with anything else, hopefully) when needed. --- .../fj-kernel/src/algorithms/approx/edge.rs | 4 +--- .../fj-kernel/src/algorithms/approx/face.rs | 6 ++---- crates/fj-kernel/src/algorithms/approx/mod.rs | 21 ------------------- 3 files changed, 3 insertions(+), 28 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index b647b7da5..6f9186636 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -43,8 +43,7 @@ impl Approx for (&Handle, &Surface) { } }; - let first = ApproxPoint::new(position_surface, position_global) - .with_source((half_edge.clone(), half_edge.boundary()[0])); + let first = ApproxPoint::new(position_surface, position_global); let points = { let approx = @@ -74,7 +73,6 @@ impl Approx for (&Handle, &Surface) { .point_from_path_coords(point.local_form); ApproxPoint::new(point_surface, point.global_form) - .with_source((half_edge.clone(), point.local_form)) }) .collect() }; diff --git a/crates/fj-kernel/src/algorithms/approx/face.rs b/crates/fj-kernel/src/algorithms/approx/face.rs index d9baff01c..1d8eae595 100644 --- a/crates/fj-kernel/src/algorithms/approx/face.rs +++ b/crates/fj-kernel/src/algorithms/approx/face.rs @@ -47,10 +47,8 @@ impl Approx for &FaceSet { panic!( "Invalid approximation: \ Distinct points are too close \ - (a: {:?}, b: {:?}, distance: {distance})\n\ - source of `a`: {:#?}\n\ - source of `b`: {:#?}\n", - a.global_form, b.global_form, a.source, b.source + (a: {:?}, b: {:?}, distance: {distance})", + a.global_form, b.global_form, ); } } diff --git a/crates/fj-kernel/src/algorithms/approx/mod.rs b/crates/fj-kernel/src/algorithms/approx/mod.rs index 52018efd0..ff0dddd16 100644 --- a/crates/fj-kernel/src/algorithms/approx/mod.rs +++ b/crates/fj-kernel/src/algorithms/approx/mod.rs @@ -10,17 +10,13 @@ pub mod solid; pub mod tolerance; use std::{ - any::Any, cmp::Ordering, fmt::Debug, hash::{Hash, Hasher}, - rc::Rc, }; use fj_math::Point; -use crate::{objects::HalfEdge, storage::Handle}; - pub use self::tolerance::{InvalidTolerance, Tolerance}; /// Approximate an object @@ -59,9 +55,6 @@ pub struct ApproxPoint { /// The global form of the points pub global_form: Point<3>, - - /// The optional source of the point - pub source: Option>, } impl ApproxPoint { @@ -70,15 +63,6 @@ impl ApproxPoint { Self { local_form, global_form, - source: None, - } - } - - /// Attach a source to the point - pub fn with_source(self, source: impl Source) -> Self { - Self { - source: Some(Rc::new(source)), - ..self } } } @@ -114,8 +98,3 @@ impl PartialOrd for ApproxPoint { Some(self.cmp(other)) } } - -/// The source of an [`ApproxPoint`] -pub trait Source: Any + Debug {} - -impl Source for (Handle, Point<1>) {} From 575b2e9fbe0e492d67106f1ebc82c31e222dfec9 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 10 Mar 2023 10:27:05 +0100 Subject: [PATCH 07/11] Relax requirements of `HalfEdge`'s `Approx` impl This will make it possible to simplify the tests, which in turn will enable more refactorings in the builder API. --- crates/fj-kernel/src/algorithms/approx/cycle.rs | 4 +++- crates/fj-kernel/src/algorithms/approx/edge.rs | 10 +++++----- 2 files changed, 8 insertions(+), 6 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/cycle.rs b/crates/fj-kernel/src/algorithms/approx/cycle.rs index 5b8b54da0..5297f0225 100644 --- a/crates/fj-kernel/src/algorithms/approx/cycle.rs +++ b/crates/fj-kernel/src/algorithms/approx/cycle.rs @@ -2,6 +2,8 @@ //! //! See [`CycleApprox`]. +use std::ops::Deref; + use fj_math::Segment; use crate::objects::{Cycle, Surface}; @@ -26,7 +28,7 @@ impl Approx for (&Cycle, &Surface) { let half_edges = cycle .half_edges() .map(|half_edge| { - (half_edge, surface).approx_with_cache(tolerance, cache) + (half_edge.deref(), surface).approx_with_cache(tolerance, cache) }) .collect(); diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index 6f9186636..82b0752f7 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -17,7 +17,7 @@ use crate::{ use super::{path::RangeOnPath, Approx, ApproxPoint, Tolerance}; -impl Approx for (&Handle, &Surface) { +impl Approx for (&HalfEdge, &Surface) { type Approximation = HalfEdgeApprox; type Cache = EdgeCache; @@ -287,7 +287,7 @@ mod tests { }; let tolerance = 1.; - let approx = (&half_edge, surface.deref()).approx(tolerance); + let approx = (half_edge.deref(), surface.deref()).approx(tolerance); assert_eq!(approx.points, Vec::new()); } @@ -313,7 +313,7 @@ mod tests { }; let tolerance = 1.; - let approx = (&half_edge, surface.deref()).approx(tolerance); + let approx = (half_edge.deref(), surface.deref()).approx(tolerance); assert_eq!(approx.points, Vec::new()); } @@ -338,7 +338,7 @@ mod tests { .insert(&mut services.objects); let tolerance = 1.; - let approx = (&half_edge, surface.deref()).approx(tolerance); + let approx = (half_edge.deref(), surface.deref()).approx(tolerance); let expected_approx = (path, range) .approx(tolerance) @@ -364,7 +364,7 @@ mod tests { .insert(&mut services.objects); let tolerance = 1.; - let approx = (&half_edge, surface.deref()).approx(tolerance); + let approx = (half_edge.deref(), surface.deref()).approx(tolerance); let expected_approx = (&half_edge.curve(), RangeOnPath::from([[0.], [TAU]])) From 17ed11f9c3ce258cb983e387644a8c7639e93d18 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 10 Mar 2023 12:46:00 +0100 Subject: [PATCH 08/11] Simplify test --- crates/fj-kernel/src/algorithms/approx/edge.rs | 15 ++++----------- 1 file changed, 4 insertions(+), 11 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index 82b0752f7..3daa90b64 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -275,19 +275,12 @@ mod tests { let mut services = Services::new(); let surface = services.objects.surfaces.xz_plane(); - let half_edge = { - let mut cycle = PartialCycle::new(&mut services.objects); - - let [half_edge, _, _] = cycle.update_as_polygon_from_points( - [[1., 1.], [2., 1.], [1., 2.]], - &mut services.objects, - ); - - half_edge - }; + let half_edge = + HalfEdgeBuilder::line_segment([[1., 1.], [2., 1.]], None) + .build(&mut services.objects); let tolerance = 1.; - let approx = (half_edge.deref(), surface.deref()).approx(tolerance); + let approx = (&half_edge, surface.deref()).approx(tolerance); assert_eq!(approx.points, Vec::new()); } From 8d3bfcc5c005f8fcbbc7ed9fe8d04ba9b91838ab Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 10 Mar 2023 12:47:08 +0100 Subject: [PATCH 09/11] Simplify test --- crates/fj-kernel/src/algorithms/approx/edge.rs | 18 +++++------------- 1 file changed, 5 insertions(+), 13 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index 3daa90b64..3e4672143 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -262,11 +262,10 @@ mod tests { use crate::{ algorithms::approx::{path::RangeOnPath, Approx, ApproxPoint}, - builder::{CycleBuilder, HalfEdgeBuilder}, + builder::HalfEdgeBuilder, geometry::{curve::GlobalPath, surface::SurfaceGeometry}, insert::Insert, objects::Surface, - partial::{PartialCycle, PartialObject}, services::Services, }; @@ -294,19 +293,12 @@ mod tests { v: [0., 0., 1.].into(), }) .insert(&mut services.objects); - let half_edge = { - let mut cycle = PartialCycle::new(&mut services.objects); - - let [half_edge, _, _] = cycle.update_as_polygon_from_points( - [[1., 1.], [2., 1.], [1., 2.]], - &mut services.objects, - ); - - half_edge - }; + let half_edge = + HalfEdgeBuilder::line_segment([[1., 1.], [2., 1.]], None) + .build(&mut services.objects); let tolerance = 1.; - let approx = (half_edge.deref(), surface.deref()).approx(tolerance); + let approx = (&half_edge, surface.deref()).approx(tolerance); assert_eq!(approx.points, Vec::new()); } From e5d747a13cbabc86502f47a320f8ae669bc42687 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 10 Mar 2023 12:47:48 +0100 Subject: [PATCH 10/11] Simplify tests --- crates/fj-kernel/src/algorithms/approx/edge.rs | 12 +++++------- 1 file changed, 5 insertions(+), 7 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index 3e4672143..2077cc34d 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -319,11 +319,10 @@ mod tests { [[0., 1.], [TAU, 1.]], Some(range.boundary), ) - .build(&mut services.objects) - .insert(&mut services.objects); + .build(&mut services.objects); let tolerance = 1.; - let approx = (half_edge.deref(), surface.deref()).approx(tolerance); + let approx = (&half_edge, surface.deref()).approx(tolerance); let expected_approx = (path, range) .approx(tolerance) @@ -344,12 +343,11 @@ mod tests { let mut services = Services::new(); let surface = services.objects.surfaces.xz_plane(); - let half_edge = HalfEdgeBuilder::circle(1.) - .build(&mut services.objects) - .insert(&mut services.objects); + let half_edge = + HalfEdgeBuilder::circle(1.).build(&mut services.objects); let tolerance = 1.; - let approx = (half_edge.deref(), surface.deref()).approx(tolerance); + let approx = (&half_edge, surface.deref()).approx(tolerance); let expected_approx = (&half_edge.curve(), RangeOnPath::from([[0.], [TAU]])) From 4cd210795686b3f2b5e60b438533a85cabf53a36 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Fri, 10 Mar 2023 12:59:41 +0100 Subject: [PATCH 11/11] Relax requirements of `HalfEdge`'s `Sweep` impl --- crates/fj-kernel/src/algorithms/sweep/edge.rs | 2 +- crates/fj-kernel/src/algorithms/sweep/face.rs | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/edge.rs b/crates/fj-kernel/src/algorithms/sweep/edge.rs index 5c6883c5e..c6cfbbe55 100644 --- a/crates/fj-kernel/src/algorithms/sweep/edge.rs +++ b/crates/fj-kernel/src/algorithms/sweep/edge.rs @@ -12,7 +12,7 @@ use crate::{ use super::{Sweep, SweepCache}; -impl Sweep for (Handle, &Handle, &Surface, Color) { +impl Sweep for (&HalfEdge, &Handle, &Surface, Color) { type Swept = (Handle, Handle); fn sweep_with_cache( diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index df81567dc..0e06e6a14 100644 --- a/crates/fj-kernel/src/algorithms/sweep/face.rs +++ b/crates/fj-kernel/src/algorithms/sweep/face.rs @@ -68,7 +68,7 @@ impl Sweep for Handle { cycle.half_edges().cloned().circular_tuple_windows() { let (face, top_edge) = ( - half_edge.clone(), + half_edge.deref(), next.start_vertex(), self.surface().deref(), self.color(),