Merge pull request #1406 from hannobraun/transform

Simplify transform code by using cache instead of partial objects

crates/fj-kernel/Cargo.toml

@@ -20,6 +20,7 @@ pretty_assertions = "1.3.0"
 robust-predicates = "0.1.4"
 spade = "2.0.0"
 thiserror = "1.0.35"
+type-map = "0.5.0"
 
 [dev-dependencies]
 anyhow = "1.0.66"

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

@@ -1,40 +1,47 @@
 use fj_math::Transform;
 
 use crate::{
-    objects::Objects,
-    partial::{PartialCurve, PartialGlobalCurve},
+    objects::{Curve, GlobalCurve, Objects},
     services::Service,
 };
 
-use super::TransformObject;
+use super::{TransformCache, TransformObject};
 
-impl TransformObject for PartialGlobalCurve {
-    fn transform(self, _: &Transform, _: &mut Service<Objects>) -> Self {
-        // `GlobalCurve` doesn't contain any internal geometry. If it did, that
-        // would just be redundant with the geometry of other objects, and this
-        // other geometry is already being transformed by other implementations
-        // of this trait.
-        self
-    }
-}
-
-impl TransformObject for PartialCurve {
-    fn transform(
+impl TransformObject for Curve {
+    fn transform_with_cache(
         self,
         transform: &Transform,
         objects: &mut Service<Objects>,
+        cache: &mut TransformCache,
     ) -> Self {
+        // Don't need to transform path, as that's defined in surface
+        // coordinates, and thus transforming `surface` takes care of it.
+        let path = self.path();
+
         let surface = self
-            .surface
-            .map(|surface| surface.transform(transform, objects));
-        let global_form = self.global_form.transform(transform, objects);
+            .surface()
+            .clone()
+            .transform_with_cache(transform, objects, cache);
+        let global_form = self
+            .global_form()
+            .clone()
+            .transform_with_cache(transform, objects, cache);
 
-        // Don't need to transform `self.path`, as that's defined in surface
-        // coordinates, and thus transforming `surface` takes care of it.
-        PartialCurve {
-            path: self.path,
-            surface,
-            global_form,
-        }
+        Self::new(surface, path, global_form)
+    }
+}
+
+impl TransformObject for GlobalCurve {
+    fn transform_with_cache(
+        self,
+        _: &Transform,
+        _: &mut Service<Objects>,
+        _: &mut TransformCache,
+    ) -> Self {
+        // `GlobalCurve` doesn't contain any internal geometry. If it did, that
+        // would just be redundant with the geometry of other objects, and this
+        // other geometry is already being transformed by other implementations
+        // of this trait.
+        self
     }
 }

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

@@ -1,19 +1,25 @@
 use fj_math::Transform;
 
-use crate::{objects::Objects, partial::PartialCycle, services::Service};
+use crate::{
+    objects::{Cycle, Objects},
+    services::Service,
+};
 
-use super::TransformObject;
+use super::{TransformCache, TransformObject};
 
-impl TransformObject for PartialCycle {
-    fn transform(
+impl TransformObject for Cycle {
+    fn transform_with_cache(
         self,
         transform: &Transform,
         objects: &mut Service<Objects>,
+        cache: &mut TransformCache,
     ) -> Self {
-        let half_edges = self
-            .half_edges()
-            .map(|edge| edge.into_partial().transform(transform, objects));
+        let half_edges = self.half_edges().map(|half_edge| {
+            half_edge
+                .clone()
+                .transform_with_cache(transform, objects, cache)
+        });
 
-        Self::default().with_half_edges(half_edges)
+        Self::new(half_edges)
     }
 }

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

@@ -1,44 +1,47 @@
 use fj_math::Transform;
 
 use crate::{
-    objects::Objects,
-    partial::{PartialGlobalEdge, PartialHalfEdge},
+    objects::{GlobalEdge, HalfEdge, Objects},
     services::Service,
 };
 
-use super::TransformObject;
+use super::{TransformCache, TransformObject};
 
-impl TransformObject for PartialHalfEdge {
-    fn transform(
+impl TransformObject for HalfEdge {
+    fn transform_with_cache(
         self,
         transform: &Transform,
         objects: &mut Service<Objects>,
+        cache: &mut TransformCache,
     ) -> Self {
-        let curve = self.curve.transform(transform, objects);
-        let vertices = self
-            .vertices
-            .map(|vertex| vertex.transform(transform, objects));
-        let global_form = self.global_form.transform(transform, objects);
+        let vertices = self.vertices().clone().map(|vertex| {
+            vertex.transform_with_cache(transform, objects, cache)
+        });
+        let global_form = self
+            .global_form()
+            .clone()
+            .transform_with_cache(transform, objects, cache);
 
-        Self {
-            curve,
-            vertices,
-            global_form,
-        }
+        Self::new(vertices, global_form)
     }
 }
 
-impl TransformObject for PartialGlobalEdge {
-    fn transform(
+impl TransformObject for GlobalEdge {
+    fn transform_with_cache(
         self,
         transform: &Transform,
         objects: &mut Service<Objects>,
+        cache: &mut TransformCache,
     ) -> Self {
-        let curve = self.curve.transform(transform, objects);
-        let vertices = self
-            .vertices
-            .map(|vertex| vertex.transform(transform, objects));
+        let curve = self
+            .curve()
+            .clone()
+            .transform_with_cache(transform, objects, cache);
+        let vertices =
+            self.vertices().access_in_normalized_order().map(|vertex| {
+                vertex.transform_with_cache(transform, objects, cache)
+            });
 
-        Self { curve, vertices }
+        Self::new(curve, vertices)
     }
 }

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

@@ -1,63 +1,46 @@
 use fj_math::Transform;
 
 use crate::{
-    insert::Insert,
     objects::{Face, FaceSet, Objects},
-    partial::{HasPartial, PartialFace},
     services::Service,
 };
 
-use super::TransformObject;
+use super::{TransformCache, TransformObject};
 
-impl TransformObject for PartialFace {
-    fn transform(
+impl TransformObject for Face {
+    fn transform_with_cache(
         self,
         transform: &Transform,
         objects: &mut Service<Objects>,
+        cache: &mut TransformCache,
     ) -> Self {
-        let surface = self
-            .surface()
-            .map(|surface| surface.transform(transform, objects));
+        // Color does not need to be transformed.
+        let color = self.color();
+
         let exterior = self
             .exterior()
-            .into_partial()
-            .transform(transform, objects)
-            .with_surface(surface.clone());
-        let interiors = self.interiors().map(|cycle| {
-            cycle
-                .into_partial()
-                .transform(transform, objects)
-                .with_surface(surface.clone())
-                .build(objects)
-                .insert(objects)
+            .clone()
+            .transform_with_cache(transform, objects, cache);
+        let interiors = self.interiors().cloned().map(|interior| {
+            interior.transform_with_cache(transform, objects, cache)
         });
 
-        let color = self.color();
-
-        let mut face = Face::partial()
-            .with_exterior(exterior)
-            .with_interiors(interiors);
-        if let Some(surface) = surface {
-            face = face.with_surface(surface);
-        }
-        if let Some(color) = color {
-            face = face.with_color(color);
-        }
-
-        face
+        Self::new(exterior, interiors, color)
     }
 }
 
 impl TransformObject for FaceSet {
-    fn transform(
+    fn transform_with_cache(
         self,
         transform: &Transform,
         objects: &mut Service<Objects>,
+        cache: &mut TransformCache,
     ) -> Self {
         let mut faces = FaceSet::new();
         faces.extend(
-            self.into_iter()
-                .map(|face| face.transform(transform, objects)),
+            self.into_iter().map(|face| {
+                face.transform_with_cache(transform, objects, cache)
+            }),
         );
         faces
     }

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

@@ -10,15 +10,16 @@ mod solid;
 mod surface;
 mod vertex;
 
+use std::collections::BTreeMap;
+
 use fj_math::{Transform, Vector};
+use type_map::TypeMap;
 
 use crate::{
     insert::Insert,
     objects::Objects,
-    partial::{HasPartial, MaybePartial, Partial},
     services::Service,
-    storage::Handle,
-    validate::{Validate, ValidationError},
+    storage::{Handle, ObjectId},
 };
 
 /// Transform an object
@@ -36,6 +37,17 @@ pub trait TransformObject: Sized {
         self,
         transform: &Transform,
         objects: &mut Service<Objects>,
+    ) -> Self {
+        let mut cache = TransformCache::default();
+        self.transform_with_cache(transform, objects, &mut cache)
+    }
+
+    /// Transform the object using the provided cache
+    fn transform_with_cache(
+        self,
+        transform: &Transform,
+        objects: &mut Service<Objects>,
+        cache: &mut TransformCache,
     ) -> Self;
 
     /// Translate the object
@@ -63,42 +75,51 @@ pub trait TransformObject: Sized {
 
 impl<T> TransformObject for Handle<T>
 where
-    T: HasPartial + Insert,
-    T::Partial: TransformObject,
-    ValidationError: From<<T as Validate>::Error>,
+    T: Clone + Insert + TransformObject + 'static,
 {
-    fn transform(
+    fn transform_with_cache(
         self,
         transform: &Transform,
         objects: &mut Service<Objects>,
+        cache: &mut TransformCache,
     ) -> Self {
-        self.to_partial()
-            .transform(transform, objects)
-            .build(objects)
-            .insert(objects)
+        if let Some(object) = cache.get(&self) {
+            return object.clone();
+        }
+
+        let transformed = self
+            .clone_object()
+            .transform_with_cache(transform, objects, cache)
+            .insert(objects);
+
+        cache.insert(self.clone(), transformed.clone());
+
+        transformed
     }
 }
 
-impl<T> TransformObject for MaybePartial<T>
-where
-    T: HasPartial,
-    Handle<T>: TransformObject,
-    T::Partial: TransformObject,
-{
-    fn transform(
-        self,
-        transform: &Transform,
-        objects: &mut Service<Objects>,
-    ) -> Self {
-        let transformed = match self {
-            Self::Full(full) => full.to_partial().transform(transform, objects),
-            Self::Partial(partial) => partial.transform(transform, objects),
-        };
-
-        // Transforming a `MaybePartial` *always* results in a partial object.
-        // This provides the most flexibility to the caller, who might want to
-        // use the transformed partial object for merging or whatever else,
-        // before building it themselves.
-        Self::Partial(transformed)
+/// A cache for transformed objects
+///
+/// See [`TransformObject`].
+#[derive(Default)]
+pub struct TransformCache(TypeMap);
+
+impl TransformCache {
+    fn get<T: 'static>(&mut self, key: &Handle<T>) -> Option<&Handle<T>> {
+        let map = self
+            .0
+            .entry::<BTreeMap<ObjectId, Handle<T>>>()
+            .or_insert_with(BTreeMap::new);
+
+        map.get(&key.id())
+    }
+
+    fn insert<T: 'static>(&mut self, key: Handle<T>, value: Handle<T>) {
+        let map = self
+            .0
+            .entry::<BTreeMap<ObjectId, Handle<T>>>()
+            .or_insert_with(BTreeMap::new);
+
+        map.insert(key.id(), value);
     }
 }