Merge pull request #1174 from hannobraun/surface

Fix known surface duplication issues

crates/fj-kernel/src/algorithms/transform/vertex.rs

@@ -10,9 +10,12 @@ use super::TransformObject;
 impl TransformObject for PartialVertex {
     fn transform(self, transform: &Transform, stores: &Stores) -> Self {
         let curve = self.curve.map(|curve| curve.transform(transform, stores));
-        let surface_form = self
-            .surface_form
-            .map(|surface_form| surface_form.transform(transform, stores));
+        let surface_form = self.surface_form.map(|surface_form| {
+            surface_form
+                .into_partial()
+                .transform(transform, stores)
+                .into()
+        });
         let global_form = self
             .global_form
             .map(|global_form| global_form.transform(transform, stores));

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

@@ -29,8 +29,8 @@ impl Cycle {
         // surface.
         for edge in &half_edges {
             assert_eq!(
-                &surface,
-                edge.surface(),
+                surface.id(),
+                edge.surface().id(),
                 "Edges in cycle not defined in same surface"
             );
         }

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

@@ -82,8 +82,8 @@ impl Face {
     ) -> Self {
         for interior in interiors.into_iter() {
             assert_eq!(
-                self.surface(),
-                interior.surface(),
+                self.surface().id(),
+                interior.surface().id(),
                 "Cycles that bound a face must be in face's surface"
             );
             assert_ne!(

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

@@ -2,7 +2,77 @@
 //!
 //! Objects, in Fornjot parlance, are the elements that make up shapes. An
 //! object can be simple and just contain data (like [`GlobalVertex`], for
-//! example), or they can be quite complex and refer to other objects.
+//! example), or they can be quite complex and refer to other objects (which is
+//! actually most of them).
+//!
+//! # Object Identity vs Object Equality
+//!
+//! Two objects are *equal*, if they contain the same data. For example, two
+//! instances of [`GlobalVertex`] are equal, if they have the same position.
+//! This doesn't mean those objects are *identical*. They might have been
+//! created by different pieces of code. Or maybe by the same piece of code, but
+//! at different times, maybe even based on different inputs.
+//!
+//! This distinction is relevant, because non-identical objects that are
+//! *supposed* to be equal can end up being equal, if they are created based on
+//! simple input data (as you might have in a unit test). But they might end up
+//! slightly different, if they are created based on complex input data (as you
+//! might have in the real world).
+//!
+//! ## An Example
+//!
+//! Let's talk about a specific example: two simple curves (straight lines that
+//! are coincident with coordinate system axes) which are intersecting at a
+//! simple point. Let's say the intersection point sits at the global origin
+//! (`[0, 0, 0]`), and its local coordinate on each line also happens to be `0`.
+//!
+//! If you compute the global coordinates from each of the line-local
+//! coordinates, you'll end up with the same result for sure. If we create two
+//! [`GlobalVertex`] instances from these global coordinates, any validation
+//! code that expects those two instances to be equal, will be happy.
+//!
+//! But what if the situation is not so simple? Let's say the curves are circles
+//! instead of lines, and instead of being all neat, they are at some arbitrary
+//! location in space, oriented at weird angles. The local coordinates of their
+//! intersection point are not `0`, but different values that are not neatly
+//! represented by floating point values.
+//!
+//! In such a situation, you have an excellent chance of ending up with slightly
+//! different global coordinates, if you compute them from each local
+//! coordinate. If you're building a [`Cycle`], and this intersection point is
+//! where the two curves connect, you could end up with a gap (or self-
+//! intersection) in the cycle. If that ends up exported to a triangle mesh,
+//! that mesh will be invalid.
+//!
+//! ## Validation Must Use Identity
+//!
+//! To prevent such situations, where everything looked fine during development,
+//! but you end up with a bug in production, any validation code that compares
+//! objects and expects them to be the same, must do that comparison based on
+//! identity, not equality. That way, this problem can never happen, because we
+//! never expect non-identical objects to be the same.
+//!
+//! For our example, this would mean we compute *one* [`GlobalVertex`] from
+//! *one* of the local coordinates.
+//!
+//! ## How Identity Works
+//!
+//! We can exactly determine the identity of an object, thanks to [centralized
+//! object storage][`Stores`]. If objects are created at different times,
+//! potentially by different code, they end up being stored at different memory
+//! locations, regardless of their (non-)equality.
+//!
+//! If you have two [`Handle`]s, you can compare the identity of the objects
+//! they point to using the `id` method.
+//!
+//! ## Implementation Note
+//!
+//! As of this writing, most objects are not managed in the centralized object
+//! storage. Changing this is an ongoing effort ([#1021]).
+//!
+//! [`Stores`]: crate::stores::Stores
+//! [`Handle`]: crate::stores::Handle
+//! [#1021]: https://github.com/hannobraun/Fornjot/issues/1021
 
 mod curve;
 mod cycle;

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

@@ -32,10 +32,15 @@ impl Vertex {
         let position = position.into();
 
         assert_eq!(
-            curve.surface(),
-            surface_form.surface(),
+            curve.surface().id(),
+            surface_form.surface().id(),
             "Surface form of vertex must be defined on same surface as curve",
         );
+        assert_eq!(
+            surface_form.global_form(),
+            &global_form,
+            "Vertex and its surface form must have same global form",
+        );
 
         Self {
             position,

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

@@ -97,15 +97,16 @@ impl PartialVertex {
 
         let surface_form = self
             .surface_form
-            .unwrap_or_else(|| {
-                PartialSurfaceVertex {
-                    position: Some(
-                        curve.path().point_from_path_coords(position),
-                    ),
-                    surface: Some(curve.surface().clone()),
-                    global_form: self.global_form,
-                }
-                .into()
+            .unwrap_or_else(|| SurfaceVertex::partial().into())
+            .update_partial(|partial| {
+                let position = partial.position.unwrap_or_else(|| {
+                    curve.path().point_from_path_coords(position)
+                });
+
+                partial
+                    .with_position(Some(position))
+                    .with_surface(Some(curve.surface().clone()))
+                    .with_global_form(self.global_form)
             })
             .into_full(stores);