Merge pull request #1322 from hannobraun/fj

Clean up sketch code in `fj` crate

crates/fj/src/abi/ffi_safe.rs

@@ -127,6 +127,35 @@ impl<T> Drop for Vec<T> {
 unsafe impl<T: Send> Send for Vec<T> {}
 unsafe impl<T: Sync> Sync for Vec<T> {}
 
+#[cfg(feature = "serde")]
+impl<T> serde::ser::Serialize for Vec<T>
+where
+    T: serde::ser::Serialize,
+{
+    fn serialize<S>(
+        &self,
+        serializer: S,
+    ) -> std::result::Result<S::Ok, S::Error>
+    where
+        S: serde::ser::Serializer,
+    {
+        self.deref().serialize(serializer)
+    }
+}
+
+#[cfg(feature = "serde")]
+impl<'de, T> serde::de::Deserialize<'de> for Vec<T>
+where
+    T: serde::de::Deserialize<'de>,
+{
+    fn deserialize<D>(deserializer: D) -> std::result::Result<Self, D::Error>
+    where
+        D: serde::de::Deserializer<'de>,
+    {
+        Ok(std::vec::Vec::deserialize(deserializer)?.into())
+    }
+}
+
 /// A FFI-safe version of `Box<str>`.
 #[repr(transparent)]
 #[derive(Debug, PartialEq, Clone)]

crates/fj/src/shape_2d.rs

@@ -1,7 +1,4 @@
-use std::mem;
+use crate::{abi::ffi_safe, Shape};
-use std::sync::atomic;
-
-use crate::Shape;
 
 /// A 2-dimensional shape
 #[derive(Clone, Debug, PartialEq)]
@@ -100,8 +97,6 @@ impl From<Difference2d> for Shape2d {
 #[repr(C)]
 pub struct Sketch {
     chain: Chain,
-
-    // The color of the sketch in RGBA
     color: [u8; 4],
 }
 
@@ -139,6 +134,18 @@ impl Sketch {
     }
 }
 
+impl From<Sketch> for Shape {
+    fn from(shape: Sketch) -> Self {
+        Self::Shape2d(shape.into())
+    }
+}
+
+impl From<Sketch> for Shape2d {
+    fn from(shape: Sketch) -> Self {
+        Shape2d::Sketch(shape)
+    }
+}
+
 /// A chain of elements that is part of a [`Sketch`]
 #[derive(Clone, Debug, PartialEq)]
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
@@ -173,229 +180,22 @@ impl Circle {
 }
 
 /// A polygonal chain that is part of a [`Sketch`]
-#[derive(Debug)]
+#[derive(Clone, Debug, PartialEq)]
+#[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[repr(C)]
 pub struct PolyChain {
-    // The fields are the raw parts of a `Vec`. `Sketch` needs to be FFI-safe,
+    points: ffi_safe::Vec<[f64; 2]>,
-    // meaning it can't store a `Vec` directly. It needs to take this detour.
-    ptr: *mut [f64; 2],
-    length: usize,
-    capacity: usize,
-
-    // The `Sketch` can be cloned, so we need to track the number of live
-    // instances, so as to free the buffer behind `ptr` only when the last
-    // one is dropped.
-    rc: *mut atomic::AtomicUsize,
 }
 
 impl PolyChain {
     /// Construct an instance from a list of points
-    pub fn from_points(mut points: Vec<[f64; 2]>) -> Self {
+    pub fn from_points(points: Vec<[f64; 2]>) -> Self {
-        // This can be cleaned up, once `Vec::into_raw_parts` is stable.
+        let points = points.into();
-        let ptr = points.as_mut_ptr();
+        Self { points }
-        let length = points.len();
-        let capacity = points.capacity();
-
-        // We're taking ownership of the memory here, so we can't allow `points`
-        // to deallocate it.
-        mem::forget(points);
-
-        // Allocate the reference counter on the heap. It will be reclaimed
-        // alongside `points` when it reaches 0.
-        let rc = Box::new(atomic::AtomicUsize::new(1));
-        let rc = Box::leak(rc) as *mut _;
-
-        Self {
-            ptr,
-            length,
-            capacity,
-            rc,
-        }
-    }
-
-    /// Get a reference to the points in this [`PolyChain`].
-    fn points(&self) -> &[[f64; 2]] {
-        unsafe { std::slice::from_raw_parts(self.ptr, self.length) }
     }
 
     /// Return the points that define the polygonal chain
     pub fn to_points(&self) -> Vec<[f64; 2]> {
-        // This is sound. All invariants are automatically kept, as the raw
+        self.points.clone().into()
-        // parts come from an original `Vec` that is identical to the new one we
-        // create here, and aren't being modified anywhere.
-        let points = unsafe {
-            Vec::from_raw_parts(self.ptr, self.length, self.capacity)
-        };
-
-        // Ownership of the pointer in `self.raw_parts` transferred to `points`.
-        // We work around that, by returning a clone of `points` (hence not
-        // giving ownership to the caller).
-        let ret = points.clone();
-
-        // Now we just need to forget that `points` ever existed, and we keep
-        // ownership of the pointer.
-        mem::forget(points);
-
-        ret
-    }
-}
-
-impl Clone for PolyChain {
-    fn clone(&self) -> Self {
-        // Increment the reference counter
-        unsafe {
-            (*self.rc).fetch_add(1, atomic::Ordering::AcqRel);
-        }
-
-        Self {
-            ptr: self.ptr,
-            length: self.length,
-            capacity: self.capacity,
-            rc: self.rc,
-        }
-    }
-}
-
-impl PartialEq for PolyChain {
-    fn eq(&self, other: &Self) -> bool {
-        self.points() == other.points()
-    }
-}
-
-impl Drop for PolyChain {
-    fn drop(&mut self) {
-        // Decrement the reference counter
-        let rc_last =
-            unsafe { (*self.rc).fetch_sub(1, atomic::Ordering::AcqRel) };
-
-        // If the value of the refcount before decrementing was 1,
-        // then this must be the last Drop call. Reclaim all resources
-        // allocated on the heap.
-        if rc_last == 1 {
-            unsafe {
-                let points =
-                    Vec::from_raw_parts(self.ptr, self.length, self.capacity);
-                let rc = Box::from_raw(self.rc);
-
-                drop(points);
-                drop(rc);
-            }
-        }
-    }
-}
-
-// `PolyChain` can be `Send`, because it encapsulates the raw pointer it
-// contains, making sure memory ownership rules are observed.
-unsafe impl Send for PolyChain {}
-
-#[cfg(feature = "serde")]
-impl serde::ser::Serialize for PolyChain {
-    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    where
-        S: serde::ser::Serializer,
-    {
-        let serde_sketch = PolyChainSerde {
-            points: self.to_points(),
-        };
-
-        serde_sketch.serialize(serializer)
-    }
-}
-
-#[cfg(feature = "serde")]
-impl<'de> serde::de::Deserialize<'de> for PolyChain {
-    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
-    where
-        D: serde::de::Deserializer<'de>,
-    {
-        PolyChainSerde::deserialize(deserializer)
-            .map(|serde_sketch| PolyChain::from_points(serde_sketch.points))
-    }
-}
-
-/// An owned, non-repr-C [`PolyChain`]
-///
-/// De/serializing a non-trivial structure with raw pointers is a hassle.
-/// This structure is a simple, owned intermediate form that can use the derive
-/// macros provided by serde. The implementation of the `Serialize` and
-/// `Deserialize` traits for [`PolyChain`] use this type as a stepping stone.
-///
-/// Note that constructing this requires cloning the points behind
-/// [`PolyChain`]. If de/serialization turns out to be a bottleneck, a more
-/// complete implementation will be required.
-#[cfg(feature = "serde")]
-#[derive(serde::Serialize, serde::Deserialize)]
-#[serde(rename = "Polyline")]
-struct PolyChainSerde {
-    points: Vec<[f64; 2]>,
-}
-
-impl From<Sketch> for Shape {
-    fn from(shape: Sketch) -> Self {
-        Self::Shape2d(shape.into())
-    }
-}
-
-impl From<Sketch> for Shape2d {
-    fn from(shape: Sketch) -> Self {
-        Shape2d::Sketch(shape)
-    }
-}
-
-#[cfg(test)]
-mod tests {
-    use super::*;
-
-    fn test_points() -> Vec<[f64; 2]> {
-        vec![[1.0, 1.0], [2.0, 1.0], [2.0, 2.0], [1.0, 2.0]]
-    }
-
-    #[test]
-    fn test_poly_chain_preserve_points() {
-        let points = test_points();
-        let poly_chain = PolyChain::from_points(points.clone());
-
-        assert_eq!(poly_chain.to_points(), points);
-    }
-
-    #[test]
-    fn test_poly_chain_rc() {
-        let assert_rc = |poly_chain: &PolyChain, expected_rc: usize| {
-            let rc =
-                unsafe { (*poly_chain.rc).load(atomic::Ordering::Acquire) };
-            assert_eq!(
-                rc, expected_rc,
-                "Sketch has rc = {rc}, expected {expected_rc}"
-            );
-        };
-
-        let poly_chain = PolyChain::from_points(test_points());
-        assert_rc(&poly_chain, 1);
-
-        let (s2, s3) = (poly_chain.clone(), poly_chain.clone());
-        assert_rc(&poly_chain, 3);
-
-        drop(s2);
-        assert_rc(&poly_chain, 2);
-
-        drop(s3);
-        assert_rc(&poly_chain, 1);
-
-        // rc is deallocated after the last drop, so we can't assert that it's 0
-    }
-
-    #[cfg(feature = "serde")]
-    #[test]
-    fn test_poly_chain_serialize_loopback() {
-        use serde_json::{from_str, to_string};
-
-        let poly_chain = PolyChain::from_points(test_points());
-
-        let json = to_string(&poly_chain).expect("failed to serialize sketch");
-        let poly_chain_de: PolyChain =
-            from_str(&json).expect("failed to deserialize sketch");
-
-        // ensure same content
-        assert_eq!(poly_chain.to_points(), poly_chain_de.to_points());
     }
 }