Merge pull request #1337 from hannobraun/merge

Clean up infrastructure for merging partial objects

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

@@ -11,7 +11,7 @@ use crate::{
     validate::{Validate, ValidationError},
 };
 
-use super::{HasPartial, Partial};
+use super::{HasPartial, MergeWith, Partial};
 
 /// Can be used everywhere either a partial or full objects are accepted
 ///
@@ -65,26 +65,6 @@ 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(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),
-            (Self::Partial(a), Self::Partial(b)) => {
-                Self::Partial(a.merge_with(b))
-            }
-        }
-    }
-
     /// Return or build a full object
     ///
     /// If this already is a full object, it is returned. If this is a partial
@@ -128,6 +108,23 @@ where
     }
 }
 
+impl<T> MergeWith for MaybePartial<T>
+where
+    T: HasPartial,
+    T::Partial: MergeWith,
+{
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        match (self, other.into()) {
+            (Self::Full(a), Self::Full(b)) => Self::Full(a.merge_with(b)),
+            (Self::Full(full), Self::Partial(_))
+            | (Self::Partial(_), Self::Full(full)) => Self::Full(full),
+            (Self::Partial(a), Self::Partial(b)) => {
+                Self::Partial(a.merge_with(b))
+            }
+        }
+    }
+}
+
 impl<T> From<Handle<T>> for MaybePartial<T>
 where
     T: HasPartial,

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

@@ -0,0 +1,106 @@
+use iter_fixed::IntoIteratorFixed;
+
+use crate::storage::Handle;
+
+/// Trait for merging partial objects
+///
+/// Implemented for all partial objects themselves, and also some related types
+/// that partial objects usually contain.
+pub trait MergeWith: Sized {
+    /// Merge this object with another
+    ///
+    /// # Panics
+    ///
+    /// Merging two objects that cannot be merged is considered a programmer
+    /// error and will result in a panic.
+    fn merge_with(self, other: impl Into<Self>) -> Self;
+}
+
+/// Wrapper struct that indicates that the contents can be merged
+///
+/// Used in connection with [`MergeWith`] to select one implementation over
+/// another.
+pub struct Mergeable<T>(pub T);
+
+impl<T, const N: usize> MergeWith for [T; N]
+where
+    T: MergeWith,
+{
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        self.into_iter_fixed()
+            .zip(other.into())
+            .collect::<[_; N]>()
+            .map(|(a, b)| a.merge_with(b))
+    }
+}
+
+impl<T> MergeWith for Option<T>
+where
+    T: PartialEq,
+{
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        let other = other.into();
+
+        if self == other {
+            return self;
+        }
+
+        // We know that `self != other`, or we wouldn't have made it here.
+        if self.is_some() && other.is_some() {
+            panic!("Can't merge two `Option`s that are both `Some`")
+        }
+
+        self.xor(other)
+    }
+}
+
+// We wouldn't need to use `Mergeable` here, if we had `specialization`:
+// https://doc.rust-lang.org/nightly/unstable-book/language-features/specialization.html
+//
+// Or maybe `min_specialization`:
+// https://doc.rust-lang.org/nightly/unstable-book/language-features/min-specialization.html
+impl<T> MergeWith for Mergeable<Option<T>>
+where
+    T: MergeWith,
+{
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        let merged = match (self.0, other.into().0) {
+            (Some(a), Some(b)) => Some(a.merge_with(b)),
+            (a, b) => a.xor(b),
+        };
+
+        Self(merged)
+    }
+}
+
+impl<T> MergeWith for Vec<T> {
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        let other = other.into();
+
+        match (self.is_empty(), other.is_empty()) {
+            (true, true) => {
+                panic!("Can't merge `PartialHalfEdge`, if both have half-edges")
+            }
+            (true, false) => other,
+            (false, true) => self,
+            (false, false) => self, // doesn't matter which we use
+        }
+    }
+}
+
+impl<T> MergeWith for Mergeable<Vec<T>> {
+    fn merge_with(mut self, other: impl Into<Self>) -> Self {
+        self.0.extend(other.into().0);
+        self
+    }
+}
+
+impl<T> MergeWith for Handle<T> {
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        if self.id() == other.into().id() {
+            return self;
+        }
+
+        panic!("Can't merge two distinct objects")
+    }
+}

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

@@ -35,12 +35,13 @@
 //! [#1147]: https://github.com/hannobraun/Fornjot/issues/1147
 
 mod maybe_partial;
+mod merge;
 mod objects;
 mod traits;
-mod util;
 
 pub use self::{
     maybe_partial::MaybePartial,
+    merge::{MergeWith, Mergeable},
     objects::{
         curve::{PartialCurve, PartialGlobalCurve},
         cycle::PartialCycle,

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

@@ -1,6 +1,6 @@
 use crate::{
     objects::{Curve, GlobalCurve, Objects, Surface},
-    partial::{util::merge_options, MaybePartial},
+    partial::{MaybePartial, MergeWith, Mergeable},
     path::SurfacePath,
     storage::Handle,
     validate::ValidationError,
@@ -59,26 +59,6 @@ impl PartialCurve {
         self
     }
 
-    /// Merge this partial object with another
-    pub fn merge_with(self, other: Self) -> Self {
-        // This is harder than it should be, as `global_form` uses the redundant
-        // `Option<MaybePartial<_>>` representation. There's some code relying
-        // on that though, so we have to live with it for now.
-        let global_form = match (self.global_form, other.global_form) {
-            (Some(a), Some(b)) => Some(a.merge_with(b)),
-            (Some(global_form), None) | (None, Some(global_form)) => {
-                Some(global_form)
-            }
-            (None, None) => None,
-        };
-
-        Self {
-            path: merge_options(self.path, other.path),
-            surface: merge_options(self.surface, other.surface),
-            global_form,
-        }
-    }
-
     /// Build a full [`Curve`] from the partial curve
     pub fn build(self, objects: &Objects) -> Result<Curve, ValidationError> {
         let path = self.path.expect("Can't build `Curve` without path");
@@ -95,6 +75,20 @@ impl PartialCurve {
     }
 }
 
+impl MergeWith for PartialCurve {
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        let other = other.into();
+
+        Self {
+            path: self.path.merge_with(other.path),
+            surface: self.surface.merge_with(other.surface),
+            global_form: Mergeable(self.global_form)
+                .merge_with(Mergeable(other.global_form))
+                .0,
+        }
+    }
+}
+
 impl From<&Curve> for PartialCurve {
     fn from(curve: &Curve) -> Self {
         Self {
@@ -117,17 +111,18 @@ impl From<&Curve> for PartialCurve {
 pub struct PartialGlobalCurve;
 
 impl PartialGlobalCurve {
-    /// Merge this partial object with another
-    pub fn merge_with(self, _: Self) -> Self {
-        Self
-    }
-
     /// Build a full [`GlobalCurve`] from the partial global curve
     pub fn build(self, _: &Objects) -> Result<GlobalCurve, ValidationError> {
         Ok(GlobalCurve)
     }
 }
 
+impl MergeWith for PartialGlobalCurve {
+    fn merge_with(self, _: impl Into<Self>) -> Self {
+        Self
+    }
+}
+
 impl From<&GlobalCurve> for PartialGlobalCurve {
     fn from(_: &GlobalCurve) -> Self {
         Self

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

@@ -1,9 +1,7 @@
 use crate::{
     builder::HalfEdgeBuilder,
     objects::{Cycle, HalfEdge, Objects, Surface},
-    partial::{
-        util::merge_options, MaybePartial, PartialHalfEdge, PartialVertex,
-    },
+    partial::{MaybePartial, MergeWith, PartialHalfEdge, PartialVertex},
     storage::Handle,
     validate::ValidationError,
 };
@@ -45,7 +43,7 @@ impl PartialCycle {
 
         let mut surface = self.surface();
         for half_edge in half_edges {
-            surface = merge_options(surface, half_edge.curve().surface());
+            surface = surface.merge_with(half_edge.curve().surface());
             self.half_edges.push(half_edge);
         }
 
@@ -66,22 +64,6 @@ impl PartialCycle {
         self
     }
 
-    /// Merge this partial object with another
-    pub fn merge_with(self, other: Self) -> Self {
-        let a_is_empty = self.half_edges.is_empty();
-        let b_is_empty = other.half_edges.is_empty();
-        let half_edges = match (a_is_empty, b_is_empty) {
-            (true, true) => {
-                panic!("Can't merge `PartialHalfEdge`, if both have half-edges")
-            }
-            (true, false) => self.half_edges,
-            (false, true) => other.half_edges,
-            (false, false) => self.half_edges, // doesn't matter which we use
-        };
-
-        Self { half_edges }
-    }
-
     /// Build a full [`Cycle`] from the partial cycle
     pub fn build(
         mut self,
@@ -146,6 +128,16 @@ impl PartialCycle {
     }
 }
 
+impl MergeWith for PartialCycle {
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        let other = other.into();
+
+        Self {
+            half_edges: self.half_edges.merge_with(other.half_edges),
+        }
+    }
+}
+
 impl From<&Cycle> for PartialCycle {
     fn from(cycle: &Cycle) -> Self {
         Self {

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

@@ -6,7 +6,7 @@ use crate::{
         Curve, GlobalCurve, GlobalEdge, GlobalVertex, HalfEdge, Objects,
         Surface, Vertex,
     },
-    partial::{util::merge_arrays, MaybePartial},
+    partial::{MaybePartial, MergeWith, Mergeable},
     storage::Handle,
     validate::ValidationError,
 };
@@ -92,15 +92,6 @@ impl PartialHalfEdge {
         self
     }
 
-    /// Merge this partial object with another
-    pub fn merge_with(self, other: Self) -> Self {
-        Self {
-            curve: self.curve.merge_with(other.curve),
-            vertices: merge_arrays(self.vertices, other.vertices),
-            global_form: self.global_form.merge_with(other.global_form),
-        }
-    }
-
     /// Build a full [`HalfEdge`] from the partial half-edge
     pub fn build(self, objects: &Objects) -> Result<HalfEdge, ValidationError> {
         let curve = self.curve.into_full(objects)?;
@@ -121,6 +112,18 @@ impl PartialHalfEdge {
     }
 }
 
+impl MergeWith for PartialHalfEdge {
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        let other = other.into();
+
+        Self {
+            curve: self.curve.merge_with(other.curve),
+            vertices: self.vertices.merge_with(other.vertices),
+            global_form: self.global_form.merge_with(other.global_form),
+        }
+    }
+}
+
 impl From<&HalfEdge> for PartialHalfEdge {
     fn from(half_edge: &HalfEdge) -> Self {
         let [back_vertex, front_vertex] =
@@ -176,23 +179,6 @@ impl PartialGlobalEdge {
         self
     }
 
-    /// Merge this partial object with another
-    pub fn merge_with(self, other: Self) -> Self {
-        // This is harder than it needs to be, because `vertices` uses the
-        // redundant combination of `Option` and `MaybePartial`. There's some
-        // code relying on that, however, so we have to live with it for now.
-        let vertices = match (self.vertices, other.vertices) {
-            (Some(a), Some(b)) => Some(merge_arrays(a, b)),
-            (Some(vertices), None) | (None, Some(vertices)) => Some(vertices),
-            (None, None) => None,
-        };
-
-        Self {
-            curve: self.curve.merge_with(other.curve),
-            vertices,
-        }
-    }
-
     /// Build a full [`GlobalEdge`] from the partial global edge
     pub fn build(
         self,
@@ -208,6 +194,19 @@ impl PartialGlobalEdge {
     }
 }
 
+impl MergeWith for PartialGlobalEdge {
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        let other = other.into();
+
+        Self {
+            curve: self.curve.merge_with(other.curve),
+            vertices: Mergeable(self.vertices)
+                .merge_with(Mergeable(other.vertices))
+                .0,
+        }
+    }
+}
+
 impl From<&GlobalEdge> for PartialGlobalEdge {
     fn from(global_edge: &GlobalEdge) -> Self {
         Self {

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

@@ -2,7 +2,7 @@ use fj_interop::mesh::Color;
 
 use crate::{
     objects::{Cycle, Face, Objects, Surface},
-    partial::{util::merge_options, MaybePartial},
+    partial::{MaybePartial, MergeWith, Mergeable},
     storage::Handle,
     validate::ValidationError,
 };
@@ -70,19 +70,6 @@ impl PartialFace {
         self
     }
 
-    /// Merge this partial object with another
-    pub fn merge_with(self, other: Self) -> Self {
-        let mut interiors = self.interiors;
-        interiors.extend(other.interiors);
-
-        Self {
-            surface: merge_options(self.surface, other.surface),
-            exterior: self.exterior.merge_with(other.exterior),
-            interiors,
-            color: merge_options(self.color, other.color),
-        }
-    }
-
     /// Construct a polygon from a list of points
     pub fn build(self, objects: &Objects) -> Result<Face, ValidationError> {
         let exterior = self.exterior.into_full(objects)?;
@@ -97,6 +84,21 @@ impl PartialFace {
     }
 }
 
+impl MergeWith for PartialFace {
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        let other = other.into();
+
+        Self {
+            surface: self.surface.merge_with(other.surface),
+            exterior: self.exterior.merge_with(other.exterior),
+            interiors: Mergeable(self.interiors)
+                .merge_with(Mergeable(other.interiors))
+                .0,
+            color: self.color.merge_with(other.color),
+        }
+    }
+}
+
 impl From<&Face> for PartialFace {
     fn from(face: &Face) -> Self {
         Self {

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

@@ -25,10 +25,6 @@ macro_rules! impl_traits {
             impl Partial for $partial {
                 type Full = $full;
 
-                fn merge_with(self, other: Self) -> Self {
-                    self.merge_with(other)
-                }
-
                 fn build(self, objects: &Objects)
                     -> Result<
                         Self::Full,

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

@@ -3,7 +3,7 @@ use fj_math::Point;
 use crate::{
     builder::GlobalVertexBuilder,
     objects::{Curve, GlobalVertex, Objects, Surface, SurfaceVertex, Vertex},
-    partial::{util::merge_options, MaybePartial},
+    partial::{MaybePartial, MergeWith},
     storage::Handle,
     validate::ValidationError,
 };
@@ -60,15 +60,6 @@ impl PartialVertex {
         self
     }
 
-    /// Merge this partial object with another
-    pub fn merge_with(self, other: Self) -> Self {
-        Self {
-            position: merge_options(self.position, other.position),
-            curve: self.curve.merge_with(other.curve),
-            surface_form: self.surface_form.merge_with(other.surface_form),
-        }
-    }
-
     /// Build a full [`Vertex`] from the partial vertex
     ///
     /// # Panics
@@ -99,6 +90,18 @@ impl PartialVertex {
     }
 }
 
+impl MergeWith for PartialVertex {
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        let other = other.into();
+
+        Self {
+            position: self.position.merge_with(other.position),
+            curve: self.curve.merge_with(other.curve),
+            surface_form: self.surface_form.merge_with(other.surface_form),
+        }
+    }
+}
+
 impl From<&Vertex> for PartialVertex {
     fn from(vertex: &Vertex) -> Self {
         Self {
@@ -165,15 +168,6 @@ impl PartialSurfaceVertex {
         self
     }
 
-    /// Merge this partial object with another
-    pub fn merge_with(self, other: Self) -> Self {
-        Self {
-            position: merge_options(self.position, other.position),
-            surface: merge_options(self.surface, other.surface),
-            global_form: self.global_form.merge_with(other.global_form),
-        }
-    }
-
     /// Build a full [`SurfaceVertex`] from the partial surface vertex
     pub fn build(
         self,
@@ -197,6 +191,18 @@ impl PartialSurfaceVertex {
     }
 }
 
+impl MergeWith for PartialSurfaceVertex {
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        let other = other.into();
+
+        Self {
+            position: self.position.merge_with(other.position),
+            surface: self.surface.merge_with(other.surface),
+            global_form: self.global_form.merge_with(other.global_form),
+        }
+    }
+}
+
 impl From<&SurfaceVertex> for PartialSurfaceVertex {
     fn from(surface_vertex: &SurfaceVertex) -> Self {
         Self {
@@ -232,13 +238,6 @@ impl PartialGlobalVertex {
         self
     }
 
-    /// Merge this partial object with another
-    pub fn merge_with(self, other: Self) -> Self {
-        Self {
-            position: merge_options(self.position, other.position),
-        }
-    }
-
     /// Build a full [`GlobalVertex`] from the partial global vertex
     pub fn build(self, _: &Objects) -> Result<GlobalVertex, ValidationError> {
         let position = self
@@ -249,6 +248,16 @@ impl PartialGlobalVertex {
     }
 }
 
+impl MergeWith for PartialGlobalVertex {
+    fn merge_with(self, other: impl Into<Self>) -> Self {
+        let other = other.into();
+
+        Self {
+            position: self.position.merge_with(other.position),
+        }
+    }
+}
+
 impl From<&GlobalVertex> for PartialGlobalVertex {
     fn from(global_vertex: &GlobalVertex) -> Self {
         Self {

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

@@ -68,9 +68,6 @@ pub trait Partial: Default + for<'a> From<&'a Self::Full> {
     /// The type representing the full variant of this object
     type Full;
 
-    /// Merge another partial object of the same type into this one
-    fn merge_with(self, other: Self) -> Self;
-
     /// Build a full object from this partial one
     ///
     /// Implementations of this method will typically try to infer any missing

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

@@ -1,29 +0,0 @@
-use iter_fixed::IntoIteratorFixed;
-
-use super::{HasPartial, MaybePartial};
-
-pub fn merge_options<T>(a: Option<T>, b: Option<T>) -> Option<T>
-where
-    T: Eq,
-{
-    if a == b {
-        return a;
-    }
-
-    // We know that `a != b`, or we wouldn't have made it here.
-    if a.is_some() && b.is_some() {
-        panic!("Can't merge `Option`s if both are defined");
-    }
-
-    a.xor(b)
-}
-
-pub fn merge_arrays<T: HasPartial>(
-    a: [MaybePartial<T>; 2],
-    b: [MaybePartial<T>; 2],
-) -> [MaybePartial<T>; 2] {
-    a.into_iter_fixed()
-        .zip(b)
-        .collect::<[_; 2]>()
-        .map(|(a, b)| a.merge_with(b))
-}