From eb780ee5c1f1587f89ff42ccea5dc5e339562727 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 14:28:09 +0200 Subject: [PATCH 1/8] Reverse edge in face sweep instead of edge sweep From the perspective of the edge sweep operation, there is no such thing as a negative direction. This is only relevant to the face sweep, so it makes more sense to do it there. And in fact, the edge sweep operation can't even decide whether it *should* reverse. The way the decision is made right now is incorrect: https://github.com/hannobraun/Fornjot/issues/990 --- crates/fj-kernel/src/algorithms/sweep/edge.rs | 6 ------ crates/fj-kernel/src/algorithms/sweep/face.rs | 5 +++++ 2 files changed, 5 insertions(+), 6 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/edge.rs b/crates/fj-kernel/src/algorithms/sweep/edge.rs index 47f38ec661..80f37b9072 100644 --- a/crates/fj-kernel/src/algorithms/sweep/edge.rs +++ b/crates/fj-kernel/src/algorithms/sweep/edge.rs @@ -17,12 +17,6 @@ impl Sweep for (Edge, Color) { let (edge, color) = self; let path = path.into(); - let edge = if path.is_negative_direction() { - edge.reverse_including_curve() - } else { - edge - }; - let surface = edge.curve().sweep(path); // We can't use the edge we're sweeping from as the bottom edge, as that diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index 59bbe43da4..8cb799cb1c 100644 --- a/crates/fj-kernel/src/algorithms/sweep/face.rs +++ b/crates/fj-kernel/src/algorithms/sweep/face.rs @@ -22,6 +22,11 @@ impl Sweep for Face { for cycle in self.all_cycles() { for &edge in cycle.edges() { + let edge = if path.is_negative_direction() { + edge.reverse_including_curve() + } else { + edge + }; let face = (edge, self.color()).sweep(path); faces.push(face); } From b5a5ffb34eece166952d3ae724fa934635d63c2a Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 14:30:56 +0200 Subject: [PATCH 2/8] Consolidate redundant method calls --- crates/fj-kernel/src/algorithms/sweep/face.rs | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index 8cb799cb1c..2bdc4aa993 100644 --- a/crates/fj-kernel/src/algorithms/sweep/face.rs +++ b/crates/fj-kernel/src/algorithms/sweep/face.rs @@ -13,8 +13,9 @@ impl Sweep for Face { let mut faces = Vec::new(); - let bottom_face = - create_bottom_face(&self, path.is_negative_direction()); + let is_negative_sweep = path.is_negative_direction(); + + let bottom_face = create_bottom_face(&self, is_negative_sweep); faces.push(bottom_face); let top_face = create_top_face(self.clone(), path); @@ -22,7 +23,7 @@ impl Sweep for Face { for cycle in self.all_cycles() { for &edge in cycle.edges() { - let edge = if path.is_negative_direction() { + let edge = if is_negative_sweep { edge.reverse_including_curve() } else { edge From 8a003df0e493710b823ad71ad8a46a82aa2e1879 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 14:31:43 +0200 Subject: [PATCH 3/8] Simplify argument name --- crates/fj-kernel/src/algorithms/sweep/face.rs | 7 ++----- 1 file changed, 2 insertions(+), 5 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index 2bdc4aa993..075935d57d 100644 --- a/crates/fj-kernel/src/algorithms/sweep/face.rs +++ b/crates/fj-kernel/src/algorithms/sweep/face.rs @@ -37,11 +37,8 @@ impl Sweep for Face { } } -fn create_bottom_face( - face: &Face, - is_sweep_along_negative_direction: bool, -) -> Face { - if is_sweep_along_negative_direction { +fn create_bottom_face(face: &Face, is_negative_sweep: bool) -> Face { + if is_negative_sweep { face.clone() } else { face.clone().reverse() From 73d835bbbce2abdbd1d12a3e591e0e105a5ffca7 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 14:32:15 +0200 Subject: [PATCH 4/8] Consolidate redundant method calls --- crates/fj-kernel/src/algorithms/sweep/face.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index 075935d57d..612ebdda77 100644 --- a/crates/fj-kernel/src/algorithms/sweep/face.rs +++ b/crates/fj-kernel/src/algorithms/sweep/face.rs @@ -18,7 +18,7 @@ impl Sweep for Face { let bottom_face = create_bottom_face(&self, is_negative_sweep); faces.push(bottom_face); - let top_face = create_top_face(self.clone(), path); + let top_face = create_top_face(self.clone(), path, is_negative_sweep); faces.push(top_face); for cycle in self.all_cycles() { @@ -45,10 +45,10 @@ fn create_bottom_face(face: &Face, is_negative_sweep: bool) -> Face { } } -fn create_top_face(face: Face, path: Path) -> Face { +fn create_top_face(face: Face, path: Path, is_negative_sweep: bool) -> Face { let mut face = face.translate(path.inner()); - if path.is_negative_direction() { + if is_negative_sweep { face = face.reverse(); }; From edcd5a32e57276ae996fd17a1d05c7608cc6adcb Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 14:40:21 +0200 Subject: [PATCH 5/8] Fix sweep operation only supporting xy-plane --- crates/fj-kernel/src/algorithms/sweep/face.rs | 21 +++++++++++++++++-- 1 file changed, 19 insertions(+), 2 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index 612ebdda77..24e4fe1638 100644 --- a/crates/fj-kernel/src/algorithms/sweep/face.rs +++ b/crates/fj-kernel/src/algorithms/sweep/face.rs @@ -1,6 +1,8 @@ +use fj_math::Scalar; + use crate::{ algorithms::{reverse::Reverse, transform::TransformObject}, - objects::{Face, Shell}, + objects::{CurveKind, Face, Shell, Surface}, }; use super::{Path, Sweep}; @@ -13,7 +15,22 @@ impl Sweep for Face { let mut faces = Vec::new(); - let is_negative_sweep = path.is_negative_direction(); + let is_negative_sweep = { + let Surface::SweptCurve(surface) = self.surface(); + + let a = match surface.curve { + CurveKind::Circle(_) => todo!( + "Sweeping from faces defined in round surfaces is not \ + supported" + ), + CurveKind::Line(line) => line.direction(), + }; + let b = surface.path; + + let normal = a.cross(&b); + + normal.dot(&path.inner()) < Scalar::ZERO + }; let bottom_face = create_bottom_face(&self, is_negative_sweep); faces.push(bottom_face); From 860c17dff4d7e10ec5d1342f5a80c04e55a23625 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 14:42:28 +0200 Subject: [PATCH 6/8] Simplify function argument --- crates/fj-kernel/src/algorithms/sweep/face.rs | 13 +++++++++---- 1 file changed, 9 insertions(+), 4 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index 24e4fe1638..925aecc5ec 100644 --- a/crates/fj-kernel/src/algorithms/sweep/face.rs +++ b/crates/fj-kernel/src/algorithms/sweep/face.rs @@ -1,4 +1,4 @@ -use fj_math::Scalar; +use fj_math::{Scalar, Vector}; use crate::{ algorithms::{reverse::Reverse, transform::TransformObject}, @@ -35,7 +35,8 @@ impl Sweep for Face { let bottom_face = create_bottom_face(&self, is_negative_sweep); faces.push(bottom_face); - let top_face = create_top_face(self.clone(), path, is_negative_sweep); + let top_face = + create_top_face(self.clone(), path.inner(), is_negative_sweep); faces.push(top_face); for cycle in self.all_cycles() { @@ -62,8 +63,12 @@ fn create_bottom_face(face: &Face, is_negative_sweep: bool) -> Face { } } -fn create_top_face(face: Face, path: Path, is_negative_sweep: bool) -> Face { - let mut face = face.translate(path.inner()); +fn create_top_face( + face: Face, + path: Vector<3>, + is_negative_sweep: bool, +) -> Face { + let mut face = face.translate(path); if is_negative_sweep { face = face.reverse(); From 80cd5f41057e6e652ebc82732eab7fc98e9da0f5 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 14:43:42 +0200 Subject: [PATCH 7/8] Simplify trait method argument --- crates/fj-kernel/src/algorithms/sweep/curve.rs | 8 +++++--- crates/fj-kernel/src/algorithms/sweep/edge.rs | 9 ++++----- crates/fj-kernel/src/algorithms/sweep/face.rs | 9 ++++----- crates/fj-kernel/src/algorithms/sweep/mod.rs | 2 +- crates/fj-kernel/src/algorithms/sweep/sketch.rs | 6 ++++-- crates/fj-kernel/src/algorithms/sweep/vertex.rs | 13 ++++++------- 6 files changed, 24 insertions(+), 23 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/curve.rs b/crates/fj-kernel/src/algorithms/sweep/curve.rs index 1fe0b2f87a..3c233a9aa0 100644 --- a/crates/fj-kernel/src/algorithms/sweep/curve.rs +++ b/crates/fj-kernel/src/algorithms/sweep/curve.rs @@ -1,3 +1,5 @@ +use fj_math::Vector; + use crate::objects::{Curve, GlobalCurve, Surface, SweptCurve}; use super::Sweep; @@ -5,7 +7,7 @@ use super::Sweep; impl Sweep for Curve { type Swept = Surface; - fn sweep(self, path: impl Into) -> Self::Swept { + fn sweep(self, path: impl Into>) -> Self::Swept { self.global_form().sweep(path) } } @@ -13,10 +15,10 @@ impl Sweep for Curve { impl Sweep for GlobalCurve { type Swept = Surface; - fn sweep(self, path: impl Into) -> Self::Swept { + fn sweep(self, path: impl Into>) -> Self::Swept { Surface::SweptCurve(SweptCurve { curve: *self.kind(), - path: path.into().inner(), + path: path.into(), }) } } diff --git a/crates/fj-kernel/src/algorithms/sweep/edge.rs b/crates/fj-kernel/src/algorithms/sweep/edge.rs index 80f37b9072..96d37c234d 100644 --- a/crates/fj-kernel/src/algorithms/sweep/edge.rs +++ b/crates/fj-kernel/src/algorithms/sweep/edge.rs @@ -1,5 +1,5 @@ use fj_interop::mesh::Color; -use fj_math::{Line, Scalar}; +use fj_math::{Line, Scalar, Vector}; use crate::{ algorithms::{reverse::Reverse, transform::TransformObject}, @@ -8,12 +8,12 @@ use crate::{ }, }; -use super::{Path, Sweep}; +use super::Sweep; impl Sweep for (Edge, Color) { type Swept = Face; - fn sweep(self, path: impl Into) -> Self::Swept { + fn sweep(self, path: impl Into>) -> Self::Swept { let (edge, color) = self; let path = path.into(); @@ -87,8 +87,7 @@ impl Sweep for (Edge, Color) { }); let curve = { - let global = - bottom_edge.curve().global_form().translate(path.inner()); + let global = bottom_edge.curve().global_form().translate(path); // Please note that creating a line here is correct, even if the // global curve is a circle. Projected into the side surface, it diff --git a/crates/fj-kernel/src/algorithms/sweep/face.rs b/crates/fj-kernel/src/algorithms/sweep/face.rs index 925aecc5ec..54a507c0d5 100644 --- a/crates/fj-kernel/src/algorithms/sweep/face.rs +++ b/crates/fj-kernel/src/algorithms/sweep/face.rs @@ -5,12 +5,12 @@ use crate::{ objects::{CurveKind, Face, Shell, Surface}, }; -use super::{Path, Sweep}; +use super::Sweep; impl Sweep for Face { type Swept = Shell; - fn sweep(self, path: impl Into) -> Self::Swept { + fn sweep(self, path: impl Into>) -> Self::Swept { let path = path.into(); let mut faces = Vec::new(); @@ -29,14 +29,13 @@ impl Sweep for Face { let normal = a.cross(&b); - normal.dot(&path.inner()) < Scalar::ZERO + normal.dot(&path) < Scalar::ZERO }; let bottom_face = create_bottom_face(&self, is_negative_sweep); faces.push(bottom_face); - let top_face = - create_top_face(self.clone(), path.inner(), is_negative_sweep); + let top_face = create_top_face(self.clone(), path, is_negative_sweep); faces.push(top_face); for cycle in self.all_cycles() { diff --git a/crates/fj-kernel/src/algorithms/sweep/mod.rs b/crates/fj-kernel/src/algorithms/sweep/mod.rs index 58d324204c..d6ad6b2b18 100644 --- a/crates/fj-kernel/src/algorithms/sweep/mod.rs +++ b/crates/fj-kernel/src/algorithms/sweep/mod.rs @@ -14,7 +14,7 @@ pub trait Sweep { type Swept; /// Sweep the object along the given path - fn sweep(self, path: impl Into) -> Self::Swept; + fn sweep(self, path: impl Into>) -> Self::Swept; } /// A path to be used with [`Sweep`] diff --git a/crates/fj-kernel/src/algorithms/sweep/sketch.rs b/crates/fj-kernel/src/algorithms/sweep/sketch.rs index 72701fabc6..73f50a20b8 100644 --- a/crates/fj-kernel/src/algorithms/sweep/sketch.rs +++ b/crates/fj-kernel/src/algorithms/sweep/sketch.rs @@ -1,11 +1,13 @@ +use fj_math::Vector; + use crate::objects::{Sketch, Solid}; -use super::{Path, Sweep}; +use super::Sweep; impl Sweep for Sketch { type Swept = Solid; - fn sweep(self, path: impl Into) -> Self::Swept { + fn sweep(self, path: impl Into>) -> Self::Swept { let path = path.into(); let mut shells = Vec::new(); diff --git a/crates/fj-kernel/src/algorithms/sweep/vertex.rs b/crates/fj-kernel/src/algorithms/sweep/vertex.rs index 1f64c1645e..822b357443 100644 --- a/crates/fj-kernel/src/algorithms/sweep/vertex.rs +++ b/crates/fj-kernel/src/algorithms/sweep/vertex.rs @@ -1,16 +1,16 @@ -use fj_math::{Line, Point, Scalar}; +use fj_math::{Line, Point, Scalar, Vector}; use crate::objects::{ Curve, CurveKind, Edge, GlobalCurve, GlobalEdge, GlobalVertex, Surface, SurfaceVertex, SweptCurve, Vertex, }; -use super::{Path, Sweep}; +use super::Sweep; impl Sweep for (Vertex, Surface) { type Swept = Edge; - fn sweep(self, path: impl Into) -> Self::Swept { + fn sweep(self, path: impl Into>) -> Self::Swept { let (vertex, surface) = self; let path = path.into(); @@ -53,7 +53,7 @@ impl Sweep for (Vertex, Surface) { }) = surface; assert_eq!(vertex.curve().global_form().kind(), &surface_curve); - assert_eq!(path.inner(), surface_path); + assert_eq!(path, surface_path); } // With that out of the way, let's start by creating the `GlobalEdge`, @@ -132,10 +132,9 @@ impl Sweep for (Vertex, Surface) { impl Sweep for GlobalVertex { type Swept = GlobalEdge; - fn sweep(self, path: impl Into) -> Self::Swept { + fn sweep(self, path: impl Into>) -> Self::Swept { let a = self; - let b = - GlobalVertex::from_position(self.position() + path.into().inner()); + let b = GlobalVertex::from_position(self.position() + path.into()); let curve = GlobalCurve::build().line_from_points([a.position(), b.position()]); From 62885d59ac4dcd24d0ed1f1dc25a435d8c0fe42f Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Thu, 8 Sep 2022 14:45:18 +0200 Subject: [PATCH 8/8] Remove unused code --- crates/fj-kernel/src/algorithms/sweep/mod.rs | 27 +------------------- 1 file changed, 1 insertion(+), 26 deletions(-) diff --git a/crates/fj-kernel/src/algorithms/sweep/mod.rs b/crates/fj-kernel/src/algorithms/sweep/mod.rs index d6ad6b2b18..fccdef75db 100644 --- a/crates/fj-kernel/src/algorithms/sweep/mod.rs +++ b/crates/fj-kernel/src/algorithms/sweep/mod.rs @@ -6,7 +6,7 @@ mod face; mod sketch; mod vertex; -use fj_math::{Scalar, Vector}; +use fj_math::Vector; /// Sweep an object along a path to create another object pub trait Sweep { @@ -16,28 +16,3 @@ pub trait Sweep { /// Sweep the object along the given path fn sweep(self, path: impl Into>) -> Self::Swept; } - -/// A path to be used with [`Sweep`] -#[derive(Clone, Copy, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)] -pub struct Path(Vector<3>); - -impl Path { - /// Return the vector that defines this path - pub fn inner(&self) -> Vector<3> { - self.0 - } - - /// Indicate whether the path is in the negative direction - pub fn is_negative_direction(&self) -> bool { - self.0.dot(&Vector::from([0., 0., 1.])) < Scalar::ZERO - } -} - -impl From for Path -where - T: Into>, -{ - fn from(value: T) -> Self { - Self(value.into()) - } -}