From 125c21c7e2c91ddcf32a7b4d371170e9c183f193 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 14 Aug 2024 19:46:42 +0200 Subject: [PATCH 1/3] Prepare for simplifying `SweepSketch` `SweepSketch` has two internal code paths. All models have so far been using the more complicated one. This commit updates them to instead use the more simple one, in preparation of removing the more complicated one. --- models/cuboid/src/lib.rs | 2 +- models/holes/src/lib.rs | 11 +++++++---- models/spacer/src/lib.rs | 2 +- models/split/src/lib.rs | 2 +- models/star/src/lib.rs | 2 +- 5 files changed, 11 insertions(+), 8 deletions(-) diff --git a/models/cuboid/src/lib.rs b/models/cuboid/src/lib.rs index 8d98cb2d2..b4d0dd1c7 100644 --- a/models/cuboid/src/lib.rs +++ b/models/cuboid/src/lib.rs @@ -14,7 +14,7 @@ pub fn model(size: impl Into>, core: &mut fj::core::Core) -> Solid { let [x, y, z] = size.into().components; let bottom_surface = core.layers.topology.surfaces.xy_plane(); - let sweep_path = Vector::from([Scalar::ZERO, Scalar::ZERO, z]); + let sweep_path = Vector::from([Scalar::ZERO, Scalar::ZERO, -z]); Sketch::empty(&core.layers.topology) .add_regions( diff --git a/models/holes/src/lib.rs b/models/holes/src/lib.rs index 240eb24fc..013cbdef8 100644 --- a/models/holes/src/lib.rs +++ b/models/holes/src/lib.rs @@ -18,7 +18,10 @@ pub fn model(radius: impl Into, core: &mut fj::core::Core) -> Solid { cuboid.update_shell( cuboid.shells().only(), |shell, core| { - let bottom_face = shell.faces().first(); + let bottom_face = shell + .faces() + .nth(5) + .expect("Expected shell to have bottom face"); let offset = size / 2.; let depth = size / 2.; @@ -32,11 +35,11 @@ pub fn model(radius: impl Into, core: &mut fj::core::Core) -> Solid { core, ); - let bottom_face = shell.faces().first(); - let top_face = shell + let bottom_face = shell .faces() .nth(5) - .expect("Expected shell to have top face"); + .expect("Expected shell to have bottom face"); + let top_face = shell.faces().first(); [shell.add_through_hole( [ diff --git a/models/spacer/src/lib.rs b/models/spacer/src/lib.rs index b0a685db1..7c398eb88 100644 --- a/models/spacer/src/lib.rs +++ b/models/spacer/src/lib.rs @@ -18,7 +18,7 @@ pub fn model( core: &mut fj::core::Core, ) -> Solid { let bottom_surface = core.layers.topology.surfaces.xy_plane(); - let sweep_path = Vector::from([0., 0., height]); + let sweep_path = Vector::from([0., 0., -height]); Sketch::empty(&core.layers.topology) .add_regions( diff --git a/models/split/src/lib.rs b/models/split/src/lib.rs index 4cba82789..013195a8e 100644 --- a/models/split/src/lib.rs +++ b/models/split/src/lib.rs @@ -22,7 +22,7 @@ pub fn model(size: f64, split_pos: f64, core: &mut fj::core::Core) -> Solid { let (shell, [face, _]) = shell.split_face(face, line, core); [shell - .sweep_face_of_shell(face, [0., 0., -size / 2.], core) + .sweep_face_of_shell(face, [0., 0., size / 2.], core) .shell] }, core, diff --git a/models/star/src/lib.rs b/models/star/src/lib.rs index bc41dac39..73235d745 100644 --- a/models/star/src/lib.rs +++ b/models/star/src/lib.rs @@ -41,7 +41,7 @@ pub fn model( } let bottom_surface = core.layers.topology.surfaces.xy_plane(); - let sweep_path = Vector::from([0., 0., h]); + let sweep_path = Vector::from([0., 0., -h]); Sketch::empty(&core.layers.topology) .add_regions( From a654c93728aacd2ff52f0795ccd4b46d982a7af2 Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 14 Aug 2024 19:49:16 +0200 Subject: [PATCH 2/3] Simplify `SweepSketch` Remove the more complicated code path, only leaving the simple ones. This puts more requirements on the caller, which I'm going to document in a follow-up commit. --- crates/fj-core/src/operations/sweep/sketch.rs | 37 +------------------ 1 file changed, 2 insertions(+), 35 deletions(-) diff --git a/crates/fj-core/src/operations/sweep/sketch.rs b/crates/fj-core/src/operations/sweep/sketch.rs index 497f202aa..4a9c217ab 100644 --- a/crates/fj-core/src/operations/sweep/sketch.rs +++ b/crates/fj-core/src/operations/sweep/sketch.rs @@ -1,8 +1,7 @@ -use fj_math::{Scalar, Vector}; +use fj_math::Vector; use crate::{ - geometry::{GlobalPath, SurfaceGeom}, - operations::{derive::DeriveFrom, insert::Insert, reverse::Reverse}, + operations::insert::Insert, storage::Handle, topology::{Face, Sketch, Solid, Surface}, Core, @@ -37,38 +36,6 @@ impl SweepSketch for Sketch { let mut shells = Vec::new(); for region in self.regions() { - let region = { - // The following code assumes that the sketch is wound counter- - // clockwise. Let's check that real quick. - assert!(region - .exterior() - .winding(&core.layers.geometry, self.surface()) - .is_ccw()); - - let SurfaceGeom::Basic { u, v } = - core.layers.geometry.of_surface(&surface); - - let is_negative_sweep = { - let u = match u { - GlobalPath::Circle(_) => todo!( - "Sweeping sketch from a rounded surfaces is not \ - supported" - ), - GlobalPath::Line(line) => line.direction(), - }; - - let normal = u.cross(v); - - normal.dot(&path) < Scalar::ZERO - }; - - if is_negative_sweep { - region.clone() - } else { - region.reverse(core).insert(core).derive_from(region, core) - } - }; - for cycle in region.all_cycles() { for half_edge in cycle.half_edges() { let curve_geom = core From 490a075112a24ae442f1a9d2a612a79358cd90ed Mon Sep 17 00:00:00 2001 From: Hanno Braun Date: Wed, 14 Aug 2024 19:54:54 +0200 Subject: [PATCH 3/3] Update documentation of `SweepSketch` --- crates/fj-core/src/operations/sweep/sketch.rs | 21 +++++++++++++++++++ 1 file changed, 21 insertions(+) diff --git a/crates/fj-core/src/operations/sweep/sketch.rs b/crates/fj-core/src/operations/sweep/sketch.rs index 4a9c217ab..85d57f4fd 100644 --- a/crates/fj-core/src/operations/sweep/sketch.rs +++ b/crates/fj-core/src/operations/sweep/sketch.rs @@ -16,6 +16,27 @@ use super::{face::SweepFace, SweepCache}; /// [module documentation]: super pub trait SweepSketch { /// # Sweep the [`Sketch`] + /// + /// Requires `path` to point towards the back of `surface`. If one of them + /// is fixed, make sure to adapt the other one accordingly. + /// + /// Not following this requirement will produce an invalid shape that + /// _should_ fail validation. + /// + /// ## Implementation Note + /// + /// The above requirement is a bit draconian. It would be much nicer, if + /// this operation just worked, regardless of the relation of `path` and + /// `surface`, and in fact, a previous version of it did. + /// + /// However, this previous version also made some undocumented assumption + /// that didn't hold in the general case. It was also getting in the way of + /// introducing the new geometry system. + /// + /// The decision was made that, for now, simplifying this operation and + /// putting more requirements on the caller, was the right call. Once the + /// new geometry system is in place, we'll hopefully be in a position to + /// improve the sweep operation substantially. fn sweep_sketch( &self, surface: Handle,