From 2772e397d899d26b62c9dea662e64b62d2b3316c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 10 Jan 2023 16:52:00 +0100 Subject: [PATCH 1/9] Simplify doc comment --- crates/fj-kernel/src/objects/stores.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-kernel/src/objects/stores.rs b/crates/fj-kernel/src/objects/stores.rs index 3cda7ba09..123eb03f3 100644 --- a/crates/fj-kernel/src/objects/stores.rs +++ b/crates/fj-kernel/src/objects/stores.rs @@ -80,7 +80,7 @@ impl Surfaces { self.store.reserve() } - /// Insert a [`Surface`] into the store + /// Insert an object into the store pub fn insert(&mut self, handle: Handle, surface: Surface) { self.store.insert(handle, surface); } From 41e4d0db5e0e7359f7c14c2cabe9c0d370caf94c Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Feb 2023 12:49:19 +0100 Subject: [PATCH 2/9] Remove outdated implementation note --- crates/fj-kernel/src/objects/stores.rs | 7 ------- 1 file changed, 7 deletions(-) diff --git a/crates/fj-kernel/src/objects/stores.rs b/crates/fj-kernel/src/objects/stores.rs index 123eb03f3..73e3c7df8 100644 --- a/crates/fj-kernel/src/objects/stores.rs +++ b/crates/fj-kernel/src/objects/stores.rs @@ -11,13 +11,6 @@ use super::{ }; /// The available object stores -/// -/// # Implementation Note -/// -/// The intention is to eventually manage all objects in here. Making this -/// happen is simply a case of putting in the required work. See [#1021]. -/// -/// [#1021]: https://github.com/hannobraun/Fornjot/issues/1021 #[derive(Debug, Default)] pub struct Objects { /// Store for [`Curve`]s From 1fe5b4b62e296ed50881a5b36f6c8f924a22edfc Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Feb 2023 12:58:30 +0100 Subject: [PATCH 3/9] Avoid using `Curve::global_form` --- .../fj-kernel/src/algorithms/approx/curve.rs | 21 ++++++++++++------- .../fj-kernel/src/algorithms/approx/edge.rs | 7 ++++++- 2 files changed, 20 insertions(+), 8 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/curve.rs b/crates/fj-kernel/src/algorithms/approx/curve.rs index b8597a00c..ec119345c 100644 --- a/crates/fj-kernel/src/algorithms/approx/curve.rs +++ b/crates/fj-kernel/src/algorithms/approx/curve.rs @@ -19,7 +19,7 @@ use crate::{ use super::{path::RangeOnPath, Approx, ApproxPoint, Tolerance}; -impl Approx for (&Handle, &Surface, RangeOnPath) { +impl Approx for (&Handle, &Surface, Handle, RangeOnPath) { type Approximation = CurveApprox; type Cache = CurveCache; @@ -28,9 +28,8 @@ impl Approx for (&Handle, &Surface, RangeOnPath) { tolerance: impl Into, cache: &mut Self::Cache, ) -> Self::Approximation { - let (curve, surface, range) = self; + let (curve, surface, global_curve, range) = self; - let global_curve = curve.global_form().clone(); let global_curve_approx = match cache.get(global_curve.clone(), range) { Some(approx) => approx, None => { @@ -237,7 +236,9 @@ mod tests { .insert(&mut services.objects); let range = RangeOnPath::from([[0.], [1.]]); - let approx = (&curve, surface.deref(), range).approx(1.); + let approx = + (&curve, surface.deref(), curve.global_form().clone(), range) + .approx(1.); assert_eq!(approx, CurveApprox::empty()); } @@ -259,7 +260,9 @@ mod tests { .insert(&mut services.objects); let range = RangeOnPath::from([[0.], [1.]]); - let approx = (&curve, surface.deref(), range).approx(1.); + let approx = + (&curve, surface.deref(), curve.global_form().clone(), range) + .approx(1.); assert_eq!(approx, CurveApprox::empty()); } @@ -281,7 +284,9 @@ mod tests { let range = RangeOnPath::from([[0.], [TAU]]); let tolerance = 1.; - let approx = (&curve, surface.deref(), range).approx(tolerance); + let approx = + (&curve, surface.deref(), curve.global_form().clone(), range) + .approx(tolerance); let expected_approx = (path, range) .approx(tolerance) @@ -310,7 +315,9 @@ mod tests { let range = RangeOnPath::from([[0.], [TAU]]); let tolerance = 1.; - let approx = (&curve, surface.deref(), range).approx(tolerance); + let approx = + (&curve, surface.deref(), curve.global_form().clone(), range) + .approx(tolerance); let expected_approx = (curve.path(), range) .approx(tolerance) diff --git a/crates/fj-kernel/src/algorithms/approx/edge.rs b/crates/fj-kernel/src/algorithms/approx/edge.rs index cefa0cee0..9d3226b24 100644 --- a/crates/fj-kernel/src/algorithms/approx/edge.rs +++ b/crates/fj-kernel/src/algorithms/approx/edge.rs @@ -35,7 +35,12 @@ impl Approx for (&Handle, &Surface) { half_edge.start_vertex().global_form().position(), ) .with_source((half_edge.clone(), half_edge.boundary()[0])); - let curve_approx = (half_edge.curve(), surface, range) + let curve_approx = ( + half_edge.curve(), + surface, + half_edge.global_form().curve().clone(), + range, + ) .approx_with_cache(tolerance, cache); HalfEdgeApprox { From 599008b865b72798595280597d31af03b0d625c8 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Feb 2023 13:01:44 +0100 Subject: [PATCH 4/9] Avoid using `Curve::global_form` --- .../fj-kernel/src/algorithms/approx/curve.rs | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/approx/curve.rs b/crates/fj-kernel/src/algorithms/approx/curve.rs index ec119345c..21d4d05df 100644 --- a/crates/fj-kernel/src/algorithms/approx/curve.rs +++ b/crates/fj-kernel/src/algorithms/approx/curve.rs @@ -218,6 +218,7 @@ mod tests { builder::{CurveBuilder, SurfaceBuilder}, geometry::path::GlobalPath, insert::Insert, + objects::GlobalCurve, partial::{PartialCurve, PartialObject, PartialSurface}, services::Services, }; @@ -234,11 +235,10 @@ mod tests { let curve = curve .build(&mut services.objects) .insert(&mut services.objects); + let global_curve = GlobalCurve.insert(&mut services.objects); let range = RangeOnPath::from([[0.], [1.]]); - let approx = - (&curve, surface.deref(), curve.global_form().clone(), range) - .approx(1.); + let approx = (&curve, surface.deref(), global_curve, range).approx(1.); assert_eq!(approx, CurveApprox::empty()); } @@ -258,11 +258,10 @@ mod tests { let curve = curve .build(&mut services.objects) .insert(&mut services.objects); + let global_curve = GlobalCurve.insert(&mut services.objects); let range = RangeOnPath::from([[0.], [1.]]); - let approx = - (&curve, surface.deref(), curve.global_form().clone(), range) - .approx(1.); + let approx = (&curve, surface.deref(), global_curve, range).approx(1.); assert_eq!(approx, CurveApprox::empty()); } @@ -280,13 +279,13 @@ mod tests { let curve = curve .build(&mut services.objects) .insert(&mut services.objects); + let global_curve = GlobalCurve.insert(&mut services.objects); let range = RangeOnPath::from([[0.], [TAU]]); let tolerance = 1.; let approx = - (&curve, surface.deref(), curve.global_form().clone(), range) - .approx(tolerance); + (&curve, surface.deref(), global_curve, range).approx(tolerance); let expected_approx = (path, range) .approx(tolerance) @@ -312,12 +311,12 @@ mod tests { let curve = curve .build(&mut services.objects) .insert(&mut services.objects); + let global_curve = GlobalCurve.insert(&mut services.objects); let range = RangeOnPath::from([[0.], [TAU]]); let tolerance = 1.; let approx = - (&curve, surface.deref(), curve.global_form().clone(), range) - .approx(tolerance); + (&curve, surface.deref(), global_curve, range).approx(tolerance); let expected_approx = (curve.path(), range) .approx(tolerance) From a52fc9ca691d5a3c572610fa65eac6e6a5bf2818 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Feb 2023 13:03:31 +0100 Subject: [PATCH 5/9] Avoid use of `Curve::global_form` --- crates/fj-kernel/src/objects/full/edge.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-kernel/src/objects/full/edge.rs b/crates/fj-kernel/src/objects/full/edge.rs index 5288eec25..2c578bd06 100644 --- a/crates/fj-kernel/src/objects/full/edge.rs +++ b/crates/fj-kernel/src/objects/full/edge.rs @@ -64,7 +64,7 @@ impl fmt::Display for HalfEdge { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { let [a, b] = self.boundary(); write!(f, "edge from {a:?} to {b:?}")?; - write!(f, " on {:?}", self.curve().global_form())?; + write!(f, " on {:?}", self.global_form().curve())?; Ok(()) } From 5ee56a3ed8bc0b89abe8f93128653d46baf806a5 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Feb 2023 13:12:27 +0100 Subject: [PATCH 6/9] Remove redundant line of code It never seems to have an effect. --- crates/fj-kernel/src/builder/edge.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index 2807164c2..af7e4daf3 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -166,7 +166,6 @@ impl HalfEdgeBuilder for PartialHalfEdge { } fn infer_global_form(&mut self) -> Partial { - self.global_form.write().curve = self.curve.read().global_form.clone(); self.global_form.write().vertices = self .vertices .each_ref_ext() From 944f791cf1dafea1b00967c22dcde62202b7a093 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Feb 2023 13:15:18 +0100 Subject: [PATCH 7/9] Avoid use of `PartialCurve`'s `global_form` field --- crates/fj-kernel/src/builder/edge.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index af7e4daf3..5c65f36bc 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -223,7 +223,7 @@ impl HalfEdgeBuilder for PartialHalfEdge { other: &Partial, surface: &SurfaceGeometry, ) { - let global_curve = other.read().curve.read().global_form.clone(); + let global_curve = other.read().global_form.read().curve.clone(); self.curve.write().global_form = global_curve.clone(); self.global_form.write().curve = global_curve; From 4befa7c6e8c5c41c40365ecebdacc1e137249496 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Feb 2023 13:20:29 +0100 Subject: [PATCH 8/9] Remove `GlobalCurve` from `Curve`/`PartialCurve` --- .../algorithms/intersect/surface_surface.rs | 6 +- crates/fj-kernel/src/algorithms/sweep/edge.rs | 3 - .../src/algorithms/transform/curve.rs | 13 ++--- crates/fj-kernel/src/builder/edge.rs | 1 - crates/fj-kernel/src/objects/full/curve.rs | 21 +------ crates/fj-kernel/src/partial/objects/curve.rs | 13 ++--- crates/fj-kernel/src/partial/objects/edge.rs | 3 +- crates/fj-kernel/src/validate/edge.rs | 58 +------------------ 8 files changed, 15 insertions(+), 103 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/intersect/surface_surface.rs b/crates/fj-kernel/src/algorithms/intersect/surface_surface.rs index 48b37f882..9406e92d4 100644 --- a/crates/fj-kernel/src/algorithms/intersect/surface_surface.rs +++ b/crates/fj-kernel/src/algorithms/intersect/surface_surface.rs @@ -3,7 +3,7 @@ use fj_math::{Line, Plane, Point, Scalar}; use crate::{ geometry::path::{GlobalPath, SurfacePath}, insert::Insert, - objects::{Curve, GlobalCurve, Objects, Surface}, + objects::{Curve, Objects, Surface}, services::Service, storage::Handle, }; @@ -55,9 +55,7 @@ impl SurfaceSurfaceIntersection { let curves = planes.map(|plane| { let path = SurfacePath::Line(plane.project_line(&line)); - let global_form = GlobalCurve.insert(objects); - - Curve::new(path, global_form).insert(objects) + Curve::new(path).insert(objects) }); Some(Self { diff --git a/crates/fj-kernel/src/algorithms/sweep/edge.rs b/crates/fj-kernel/src/algorithms/sweep/edge.rs index 4f539b11b..2a625ab36 100644 --- a/crates/fj-kernel/src/algorithms/sweep/edge.rs +++ b/crates/fj-kernel/src/algorithms/sweep/edge.rs @@ -135,9 +135,6 @@ impl Sweep for (Handle, &Surface, Color) { .zip_ext(global_edges) .map(|(mut half_edge, global_edge)| { let global_edge = Partial::from(global_edge); - - half_edge.curve.write().global_form = - global_edge.read().curve.clone(); half_edge.global_form = global_edge; }); diff --git a/crates/fj-kernel/src/algorithms/transform/curve.rs b/crates/fj-kernel/src/algorithms/transform/curve.rs index 3b786abcd..27489c417 100644 --- a/crates/fj-kernel/src/algorithms/transform/curve.rs +++ b/crates/fj-kernel/src/algorithms/transform/curve.rs @@ -10,20 +10,15 @@ use super::{TransformCache, TransformObject}; impl TransformObject for Curve { fn transform_with_cache( self, - transform: &Transform, - objects: &mut Service, - cache: &mut TransformCache, + _: &Transform, + _: &mut Service, + _: &mut TransformCache, ) -> Self { // Don't need to transform path, as that's defined in surface // coordinates, and thus transforming `surface` takes care of it. let path = self.path(); - let global_form = self - .global_form() - .clone() - .transform_with_cache(transform, objects, cache); - - Self::new(path, global_form) + Self::new(path) } } diff --git a/crates/fj-kernel/src/builder/edge.rs b/crates/fj-kernel/src/builder/edge.rs index 5c65f36bc..eacb8d1eb 100644 --- a/crates/fj-kernel/src/builder/edge.rs +++ b/crates/fj-kernel/src/builder/edge.rs @@ -224,7 +224,6 @@ impl HalfEdgeBuilder for PartialHalfEdge { surface: &SurfaceGeometry, ) { let global_curve = other.read().global_form.read().curve.clone(); - self.curve.write().global_form = global_curve.clone(); self.global_form.write().curve = global_curve; self.curve.write().path = diff --git a/crates/fj-kernel/src/objects/full/curve.rs b/crates/fj-kernel/src/objects/full/curve.rs index 5515141ce..4bce36c06 100644 --- a/crates/fj-kernel/src/objects/full/curve.rs +++ b/crates/fj-kernel/src/objects/full/curve.rs @@ -1,36 +1,21 @@ -use crate::{ - geometry::path::SurfacePath, - storage::{Handle, HandleWrapper}, -}; +use crate::geometry::path::SurfacePath; /// A curve, defined in local surface coordinates #[derive(Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] pub struct Curve { path: SurfacePath, - global_form: HandleWrapper, } impl Curve { /// Construct a new instance of `Curve` - pub fn new( - path: SurfacePath, - global_form: impl Into>, - ) -> Self { - Self { - path, - global_form: global_form.into(), - } + pub fn new(path: SurfacePath) -> Self { + Self { path } } /// Access the path that defines the curve pub fn path(&self) -> SurfacePath { self.path } - - /// Access the global form of the curve - pub fn global_form(&self) -> &Handle { - &self.global_form - } } /// A curve, defined in global (3D) coordinates diff --git a/crates/fj-kernel/src/partial/objects/curve.rs b/crates/fj-kernel/src/partial/objects/curve.rs index 4377536ae..7f3174d40 100644 --- a/crates/fj-kernel/src/partial/objects/curve.rs +++ b/crates/fj-kernel/src/partial/objects/curve.rs @@ -3,7 +3,7 @@ use fj_math::Scalar; use crate::{ geometry::path::SurfacePath, objects::{Curve, GlobalCurve, Objects}, - partial::{FullToPartialCache, Partial, PartialObject}, + partial::{FullToPartialCache, PartialObject}, services::Service, }; @@ -12,22 +12,18 @@ use crate::{ pub struct PartialCurve { /// The path that defines the curve pub path: Option, - - /// The global form of the curve - pub global_form: Partial, } impl PartialObject for PartialCurve { type Full = Curve; - fn from_full(curve: &Self::Full, cache: &mut FullToPartialCache) -> Self { + fn from_full(curve: &Self::Full, _: &mut FullToPartialCache) -> Self { Self { path: Some(curve.path().into()), - global_form: Partial::from_full(curve.global_form().clone(), cache), } } - fn build(self, objects: &mut Service) -> Self::Full { + fn build(self, _: &mut Service) -> Self::Full { let path = match self.path.expect("Need path to build curve") { MaybeSurfacePath::Defined(path) => path, undefined => { @@ -36,9 +32,8 @@ impl PartialObject for PartialCurve { ) } }; - let global_form = self.global_form.build(objects); - Curve::new(path, global_form) + Curve::new(path) } } diff --git a/crates/fj-kernel/src/partial/objects/edge.rs b/crates/fj-kernel/src/partial/objects/edge.rs index db7ea206b..ada1c7338 100644 --- a/crates/fj-kernel/src/partial/objects/edge.rs +++ b/crates/fj-kernel/src/partial/objects/edge.rs @@ -74,7 +74,6 @@ impl Default for PartialHalfEdge { (None, surface_form) }); - let global_curve = curve.read().global_form.clone(); let global_vertices = vertices.each_ref_ext().map( |vertex: &(Option>, Partial)| { let surface_vertex = vertex.1.clone(); @@ -84,8 +83,8 @@ impl Default for PartialHalfEdge { ); let global_form = Partial::from_partial(PartialGlobalEdge { - curve: global_curve, vertices: global_vertices, + ..Default::default() }); Self { diff --git a/crates/fj-kernel/src/validate/edge.rs b/crates/fj-kernel/src/validate/edge.rs index e0f7e933c..224512fc9 100644 --- a/crates/fj-kernel/src/validate/edge.rs +++ b/crates/fj-kernel/src/validate/edge.rs @@ -13,7 +13,6 @@ impl Validate for HalfEdge { config: &ValidationConfig, errors: &mut Vec, ) { - HalfEdgeValidationError::check_global_curve_identity(self, errors); HalfEdgeValidationError::check_global_vertex_identity(self, errors); HalfEdgeValidationError::check_vertex_coincidence(self, config, errors); } @@ -107,26 +106,6 @@ pub enum HalfEdgeValidationError { } impl HalfEdgeValidationError { - fn check_global_curve_identity( - half_edge: &HalfEdge, - errors: &mut Vec, - ) { - let global_curve_from_curve = half_edge.curve().global_form(); - let global_curve_from_global_form = half_edge.global_form().curve(); - - if global_curve_from_curve.id() != global_curve_from_global_form.id() { - errors.push( - Box::new(Self::GlobalCurveMismatch { - global_curve_from_curve: global_curve_from_curve.clone(), - global_curve_from_global_form: - global_curve_from_global_form.clone(), - half_edge: half_edge.clone(), - }) - .into(), - ); - } - } - fn check_global_vertex_identity( half_edge: &HalfEdge, errors: &mut Vec, @@ -185,47 +164,12 @@ mod tests { use crate::{ builder::HalfEdgeBuilder, - insert::Insert, - objects::{GlobalCurve, HalfEdge}, + objects::HalfEdge, partial::{Partial, PartialHalfEdge, PartialObject}, services::Services, validate::Validate, }; - #[test] - fn half_edge_global_curve_mismatch() -> anyhow::Result<()> { - let mut services = Services::new(); - - let valid = { - let surface = services.objects.surfaces.xy_plane(); - - let mut half_edge = PartialHalfEdge::default(); - half_edge.update_as_line_segment_from_points([[0., 0.], [1., 0.]]); - half_edge.infer_vertex_positions_if_necessary(&surface.geometry()); - - half_edge.build(&mut services.objects) - }; - let invalid = { - let global_form = { - let mut global_edge = - Partial::from(valid.global_form().clone()); - global_edge.write().curve = - Partial::from(GlobalCurve.insert(&mut services.objects)); - global_edge.build(&mut services.objects) - }; - let vertices = valid - .boundary() - .zip_ext(valid.surface_vertices().map(Clone::clone)); - - HalfEdge::new(valid.curve().clone(), vertices, global_form) - }; - - valid.validate_and_return_first_error()?; - assert!(invalid.validate_and_return_first_error().is_err()); - - Ok(()) - } - #[test] fn half_edge_global_vertex_mismatch() -> anyhow::Result<()> { let mut services = Services::new(); From 01e75d14a1a0e57b1f9d4bbb4cb60a5d023f0fad Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Tue, 21 Feb 2023 13:21:48 +0100 Subject: [PATCH 9/9] Inline variable --- crates/fj-kernel/src/algorithms/sweep/edge.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/edge.rs b/crates/fj-kernel/src/algorithms/sweep/edge.rs index 2a625ab36..5fc50f4c5 100644 --- a/crates/fj-kernel/src/algorithms/sweep/edge.rs +++ b/crates/fj-kernel/src/algorithms/sweep/edge.rs @@ -134,8 +134,7 @@ impl Sweep for (Handle, &Surface, Color) { [edge_bottom.write(), edge_up.write(), edge_down.write()] .zip_ext(global_edges) .map(|(mut half_edge, global_edge)| { - let global_edge = Partial::from(global_edge); - half_edge.global_form = global_edge; + half_edge.global_form = Partial::from(global_edge); }); // And we're done creating the face! All that's left to do is build our