Merge pull request #1326 from hannobraun/validate

Move cycle validation to new infrastructure

crates/fj-kernel/Cargo.toml

@@ -13,6 +13,7 @@ categories.workspace = true
 [dependencies]
 fj-interop.workspace = true
 fj-math.workspace = true
+itertools = "0.10.5"
 parking_lot = "0.12.0"
 pretty_assertions = "1.3.0"
 robust-predicates = "0.1.4"

crates/fj-kernel/src/builder/cycle.rs

@@ -55,34 +55,32 @@ impl CycleBuilder for PartialCycle {
                     .surface()
                     .expect("Need surface to extend cycle with poly-chain");
 
-                let position_prev = vertex_prev
-                    .position()
-                    .expect("Need surface position to extend cycle");
-                let position_next = vertex_next
-                    .position()
-                    .expect("Need surface position to extend cycle");
-
-                let from = vertex_prev;
-                let to = vertex_next;
+                let [position_prev, position_next] =
+                    [&vertex_prev, &vertex_next].map(|vertex| {
+                        vertex
+                            .position()
+                            .expect("Need surface position to extend cycle")
+                    });
 
-                previous = Some(to.clone());
+                previous = Some(vertex_next.clone());
 
                 let curve = Curve::partial()
                     .with_surface(Some(surface.clone()))
                     .update_as_line_from_points([position_prev, position_next]);
 
-                let [from, to] =
-                    [(0., from), (1., to)].map(|(position, surface_form)| {
+                let vertices = [(0., vertex_prev), (1., vertex_next)].map(
+                    |(position, surface_form)| {
                         Vertex::partial()
                             .with_curve(curve.clone())
                             .with_position(Some([position]))
                             .with_surface_form(surface_form)
-                    });
+                    },
+                );
 
                 half_edges.push(
                     HalfEdge::partial()
                         .with_curve(curve)
-                        .with_vertices([from, to]),
+                        .with_vertices(vertices),
                 );
 
                 continue;

crates/fj-kernel/src/builder/edge.rs

@@ -6,7 +6,7 @@ use crate::{
         Curve, GlobalVertex, Objects, Surface, SurfaceVertex, Vertex,
         VerticesInNormalizedOrder,
     },
-    partial::{HasPartial, PartialGlobalEdge, PartialHalfEdge},
+    partial::{HasPartial, MaybePartial, PartialGlobalEdge, PartialHalfEdge},
     storage::Handle,
     validate::ValidationError,
 };
@@ -15,6 +15,12 @@ use super::{CurveBuilder, GlobalVertexBuilder};
 
 /// Builder API for [`PartialHalfEdge`]
 pub trait HalfEdgeBuilder: Sized {
+    /// Update the partial half-edge with the given back vertex
+    fn with_back_vertex(self, back: impl Into<MaybePartial<Vertex>>) -> Self;
+
+    /// Update the partial half-edge with the given front vertex
+    fn with_front_vertex(self, front: impl Into<MaybePartial<Vertex>>) -> Self;
+
     /// Update partial half-edge as a circle, from the given radius
     ///
     /// # Implementation Note
@@ -37,9 +43,22 @@ pub trait HalfEdgeBuilder: Sized {
 
     /// Update partial half-edge as a line segment, reusing existing vertices
     fn update_as_line_segment(self) -> Self;
+
+    /// Infer the global form of the partial half-edge
+    fn infer_global_form(self) -> Self;
 }
 
 impl HalfEdgeBuilder for PartialHalfEdge {
+    fn with_back_vertex(self, back: impl Into<MaybePartial<Vertex>>) -> Self {
+        let [_, front] = self.vertices();
+        self.with_vertices([back.into(), front])
+    }
+
+    fn with_front_vertex(self, front: impl Into<MaybePartial<Vertex>>) -> Self {
+        let [back, _] = self.vertices();
+        self.with_vertices([back, front.into()])
+    }
+
     fn update_as_circle_from_radius(
         self,
         radius: impl Into<Scalar>,
@@ -181,6 +200,10 @@ impl HalfEdgeBuilder for PartialHalfEdge {
 
         self.with_curve(curve).with_vertices([back, front])
     }
+
+    fn infer_global_form(self) -> Self {
+        self.with_global_form(PartialGlobalEdge::default())
+    }
 }
 
 /// Builder API for [`PartialGlobalEdge`]

crates/fj-kernel/src/objects/cycle.rs

@@ -1,6 +1,7 @@
+use std::slice;
+
 use fj_interop::ext::SliceExt;
 use fj_math::{Scalar, Winding};
-use pretty_assertions::assert_eq;
 
 use crate::{path::SurfacePath, storage::Handle};
 
@@ -24,48 +25,14 @@ impl Cycle {
     pub fn new(half_edges: impl IntoIterator<Item = Handle<HalfEdge>>) -> Self {
         let half_edges = half_edges.into_iter().collect::<Vec<_>>();
 
-        let surface = match half_edges.first() {
-            Some(half_edge) => half_edge.surface().clone(),
-            None => panic!("Cycle must contain at least one half-edge"),
-        };
-
-        // Verify, that the curves of all edges are defined in the correct
-        // surface.
-        for edge in &half_edges {
-            assert_eq!(
-                surface.id(),
-                edge.curve().surface().id(),
-                "Edges in cycle not defined in same surface"
-            );
-        }
-
-        if half_edges.len() != 1 {
-            // Verify that all edges connect.
-            for [a, b] in half_edges.as_slice().array_windows_ext() {
-                let [_, prev] = a.vertices();
-                let [next, _] = b.vertices();
-
-                assert_eq!(
-                    prev.surface_form().id(),
-                    next.surface_form().id(),
-                    "Edges in cycle do not connect"
-                );
-            }
-        }
-
-        // Verify that the edges form a cycle
-        if let Some(first) = half_edges.first() {
-            if let Some(last) = half_edges.last() {
-                let [first, _] = first.vertices();
-                let [_, last] = last.vertices();
-
-                assert_eq!(
-                    first.surface_form().id(),
-                    last.surface_form().id(),
-                    "Edges do not form a cycle"
-                );
-            }
-        }
+        // This is not a validation check, and thus not part of the validation
+        // infrastructure. The property being checked here is inherent to the
+        // validity of a `Cycle`, as methods of `Cycle` might assume that there
+        // is at least one edge.
+        assert!(
+            !half_edges.is_empty(),
+            "Cycle must contain at least one half-edge"
+        );
 
         Self { half_edges }
     }
@@ -82,7 +49,7 @@ impl Cycle {
     }
 
     /// Access the half-edges that make up the cycle
-    pub fn half_edges(&self) -> impl Iterator<Item = &Handle<HalfEdge>> + '_ {
+    pub fn half_edges(&self) -> HalfEdgesOfCycle {
         self.half_edges.iter()
     }
 
@@ -144,3 +111,8 @@ impl Cycle {
         unreachable!("Encountered invalid cycle: {self:#?}");
     }
 }
+
+/// An iterator over the half-edges of a [`Cycle`]
+///
+/// Returned by [`Cycle::half_edges`].
+pub type HalfEdgesOfCycle<'a> = slice::Iter<'a, Handle<HalfEdge>>;

crates/fj-kernel/src/objects/mod.rs

@@ -85,7 +85,7 @@ mod vertex;
 
 pub use self::{
     curve::{Curve, GlobalCurve},
-    cycle::Cycle,
+    cycle::{Cycle, HalfEdgesOfCycle},
     edge::{GlobalEdge, HalfEdge, VerticesInNormalizedOrder},
     face::{Face, FaceSet, Handedness},
     shell::Shell,
@@ -103,8 +103,8 @@ use crate::{
     path::GlobalPath,
     storage::{Handle, Store},
     validate::{
-        HalfEdgeValidationError, SurfaceVertexValidationError, Validate2,
-        VertexValidationError,
+        CycleValidationError, HalfEdgeValidationError,
+        SurfaceVertexValidationError, Validate2, VertexValidationError,
     },
 };
 
@@ -187,7 +187,10 @@ pub struct Cycles {
 
 impl Cycles {
     /// Insert a [`Cycle`] into the store
-    pub fn insert(&self, cycle: Cycle) -> Result<Handle<Cycle>, Infallible> {
+    pub fn insert(
+        &self,
+        cycle: Cycle,
+    ) -> Result<Handle<Cycle>, CycleValidationError> {
         cycle.validate()?;
         Ok(self.store.insert(cycle))
     }

crates/fj-kernel/src/partial/maybe_partial.rs

@@ -67,8 +67,14 @@ impl<T: HasPartial> MaybePartial<T> {
     /// Merge this `MaybePartial` with another of the same type
     pub fn merge_with(self, other: impl Into<Self>) -> Self {
         match (self, other.into()) {
-            (Self::Full(_), Self::Full(_)) => {
-                panic!("Can't merge two full objects")
+            (Self::Full(a), Self::Full(b)) => {
+                if a.id() != b.id() {
+                    panic!("Can't merge two full objects")
+                }
+
+                // If they're equal, which they are, if we reach this point,
+                // then merging them is a no-op.
+                Self::Full(a)
             }
             (Self::Full(full), Self::Partial(_))
             | (Self::Partial(_), Self::Full(full)) => Self::Full(full),
@@ -218,6 +224,14 @@ impl MaybePartial<SurfaceVertex> {
             Self::Partial(partial) => partial.surface(),
         }
     }
+
+    /// Access the global form
+    pub fn global_form(&self) -> MaybePartial<GlobalVertex> {
+        match self {
+            Self::Full(full) => full.global_form().clone().into(),
+            Self::Partial(partial) => partial.global_form(),
+        }
+    }
 }
 
 impl MaybePartial<Vertex> {

crates/fj-kernel/src/partial/objects/cycle.rs

@@ -1,4 +1,5 @@
 use crate::{
+    builder::HalfEdgeBuilder,
     objects::{Cycle, HalfEdge, Objects, Surface},
     partial::{
         util::merge_options, MaybePartial, PartialHalfEdge, PartialVertex,
@@ -86,6 +87,22 @@ impl PartialCycle {
         mut self,
         objects: &Objects,
     ) -> Result<Handle<Cycle>, ValidationError> {
+        // Check that the cycle is closed. This will lead to a panic further
+        // down anyway, but that panic would be super-confusing. This one should
+        // be a bit more explicit on what is wrong.
+        if let (Some(first), Some(last)) =
+            (self.half_edges.first(), self.half_edges.last())
+        {
+            let [first, _] = first.vertices();
+            let [_, last] = last.vertices();
+
+            assert_eq!(
+                first.surface_form().position(),
+                last.surface_form().position(),
+                "Attempting to build un-closed cycle"
+            );
+        }
+
         // To create a cycle, we need to make sure that all its half-edges
         // connect to each other. Let's start with all the connections between
         // the first and the last half-edge.
@@ -96,14 +113,11 @@ impl PartialCycle {
                 half_edge.front().surface_form().into_full(objects)?;
 
             *half_edge = half_edge.clone().merge_with(
-                PartialHalfEdge::default()
-                    .with_back_vertex(
-                        PartialVertex::default().with_surface_form(back_vertex),
-                    )
-                    .with_front_vertex(
-                        PartialVertex::default()
-                            .with_surface_form(front_vertex.clone()),
-                    ),
+                PartialHalfEdge::default().with_vertices([
+                    PartialVertex::default().with_surface_form(back_vertex),
+                    PartialVertex::default()
+                        .with_surface_form(front_vertex.clone()),
+                ]),
             );
 
             previous_vertex = Some(MaybePartial::from(front_vertex));

crates/fj-kernel/src/partial/objects/edge.rs

@@ -73,28 +73,6 @@ impl PartialHalfEdge {
         self
     }
 
-    /// Update the partial half-edge with the given back vertex
-    pub fn with_back_vertex(
-        mut self,
-        vertex: impl Into<MaybePartial<Vertex>>,
-    ) -> Self {
-        let [from, _] = &mut self.vertices;
-        *from = vertex.into();
-
-        self
-    }
-
-    /// Update the partial half-edge with the given front vertex
-    pub fn with_front_vertex(
-        mut self,
-        vertex: impl Into<MaybePartial<Vertex>>,
-    ) -> Self {
-        let [_, to] = &mut self.vertices;
-        *to = vertex.into();
-
-        self
-    }
-
     /// Update the partial half-edge with the given vertices
     pub fn with_vertices(
         mut self,