diff --git a/crates/fj-core/src/operations/sweep/sketch.rs b/crates/fj-core/src/operations/sweep/sketch.rs index 497f202aa..85d57f4fd 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, @@ -17,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, @@ -37,38 +57,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 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(