From 5b2dd67a07aabd4b6c7882b5efad31c633a7f561 Mon Sep 17 00:00:00 2001 From: Martin Svanberg Date: Wed, 28 Aug 2024 14:22:53 +0200 Subject: [PATCH 1/5] Support user data with USINGZ --- Cargo.lock | 3 +- Cargo.toml | 4 ++ src/clipper.rs | 144 ++++++++++++++++++++++++++++++++++++++++++++++++- src/path.rs | 1 + src/point.rs | 51 +++++++++++++++++- 5 files changed, 198 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0c89d6f..a3eacd0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -84,8 +84,7 @@ dependencies = [ [[package]] name = "clipper2c-sys" version = "0.1.4" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "e2995248c63ec82bf22c921cfd26fe407a4402b7039e045d286f7470cd83fe9e" +source = "git+https://github.com/tirithen/clipper2c-sys?rev=210878394637f94bcd3a476bdf24ddf4ba8a39fd#210878394637f94bcd3a476bdf24ddf4ba8a39fd" dependencies = [ "cc", "libc", diff --git a/Cargo.toml b/Cargo.toml index 946a566..47adfc3 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -15,6 +15,7 @@ categories = ["algorithms"] default = ["doc-images"] doc-images = [] serde = ["dep:serde", "clipper2c-sys/serde"] +usingz = ["clipper2c-sys/usingz"] [dependencies] libc = "0.2" @@ -31,3 +32,6 @@ serde_json = "1" # docs.rs uses a nightly compiler, so by instructing it to use our `doc-images` feature we # ensure that it will render any images that we may have in inner attribute documentation. features = ["doc-images"] + +[patch.crates-io] +clipper2c-sys = { rev = "210878394637f94bcd3a476bdf24ddf4ba8a39fd", git = "https://github.com/tirithen/clipper2c-sys" } diff --git a/src/clipper.rs b/src/clipper.rs index d6f317b..9fe01e2 100644 --- a/src/clipper.rs +++ b/src/clipper.rs @@ -3,10 +3,10 @@ use std::marker::PhantomData; use clipper2c_sys::{ clipper_clipper64, clipper_clipper64_add_clip, clipper_clipper64_add_open_subject, clipper_clipper64_add_subject, clipper_clipper64_execute, clipper_clipper64_size, - clipper_delete_clipper64, clipper_delete_paths64, ClipperClipper64, + clipper_delete_clipper64, clipper_delete_paths64, ClipperClipper64, ClipperPoint64, }; -use crate::{malloc, Centi, ClipType, FillRule, Paths, PointScaler}; +use crate::{malloc, Centi, ClipType, FillRule, Paths, Point, PointScaler}; /// The state of the Clipper struct. pub trait ClipperState {} @@ -33,6 +33,10 @@ pub struct Clipper { keep_ptr_on_drop: bool, _marker: PhantomData

, _state: S, + + /// We only hold on to this in order to avoid leaking memory when Clipper is dropped + #[cfg(feature = "usingz")] + raw_z_callback: Option<*mut libc::c_void>, } impl Clipper { @@ -48,6 +52,9 @@ impl Clipper { keep_ptr_on_drop: false, _marker: PhantomData, _state: NoSubjects {}, + + #[cfg(feature = "usingz")] + raw_z_callback: None, } } } @@ -72,6 +79,9 @@ impl Clipper { keep_ptr_on_drop: false, _marker: PhantomData, _state: WithSubjects {}, + + #[cfg(feature = "usingz")] + raw_z_callback: None, }; drop(self); @@ -98,12 +108,88 @@ impl Clipper { keep_ptr_on_drop: false, _marker: PhantomData, _state: WithSubjects {}, + + #[cfg(feature = "usingz")] + raw_z_callback: None, }; drop(self); clipper.add_open_subject(subject) } + + #[cfg(feature = "usingz")] + /// Allows specifying a callback that will be called each time a new vertex + /// is created by Clipper, in order to set user data on such points. New + /// points are created at the intersections between two edges, and the + /// callback will be called with the four neighboring points from those two + /// edges. The last argument is the new point itself. + /// + /// # Examples + /// + /// ```rust + /// use clipper2::*; + /// + /// let mut clipper = Clipper::::new(); + /// clipper.set_z_callback(|_: Point<_>, _: Point<_>, _: Point<_>, _: Point<_>, p: &mut Point<_>| { + /// p.set_z(1); + /// }); + /// ``` + pub fn set_z_callback( + &mut self, + callback: impl Fn(Point

, Point

, Point

, Point

, &mut Point

), + ) { + // The closure will be represented by a trait object behind a fat + // pointer. Since fat pointers are larger than thin pointers, they + // cannot be passed through a thin-pointer c_void type. We must + // therefore wrap the fat pointer in another box, leading to this double + // indirection. + let cb: Box, Point

, Point

, Point

, &mut Point

)>> = + Box::new(Box::new(callback)); + let raw_cb = Box::into_raw(cb) as *mut _; + + // It there is an old callback stored, drop it before replacing it + if let Some(old_raw_cb) = self.raw_z_callback { + drop(unsafe { Box::from_raw(old_raw_cb as *mut _) }); + } + self.raw_z_callback = Some(raw_cb); + + unsafe { + clipper2c_sys::clipper_clipper64_set_z_callback( + self.ptr, + raw_cb, + Some(handle_set_z_callback::

), + ); + } + } +} + +extern "C" fn handle_set_z_callback( + user_data: *mut ::std::os::raw::c_void, + e1bot: *const ClipperPoint64, + e1top: *const ClipperPoint64, + e2bot: *const ClipperPoint64, + e2top: *const ClipperPoint64, + pt: *mut ClipperPoint64, +) { + // SAFETY: user_data was set in set_z_callback, and is valid for as long as + // the Clipper2 instance exists. + let callback: &mut &mut dyn Fn(Point

, Point

, Point

, Point

, &mut Point

) = + unsafe { std::mem::transmute(user_data) }; + + // SAFETY: Clipper2 should produce valid pointers + let mut new_point = unsafe { Point::

::from(*pt) }; + let e1bot = unsafe { Point::

::from(*e1bot) }; + let e1top = unsafe { Point::

::from(*e1top) }; + let e2bot = unsafe { Point::

::from(*e2bot) }; + let e2top = unsafe { Point::

::from(*e2top) }; + + callback(e1bot, e1top, e2bot, e2top, &mut new_point); + + // SAFETY: pt is a valid pointer and new_point is not null + unsafe { + *pt = *new_point.as_clipperpoint64(); + } } impl Clipper { @@ -171,6 +257,9 @@ impl Clipper { keep_ptr_on_drop: false, _marker: PhantomData, _state: WithClips {}, + + #[cfg(feature = "usingz")] + raw_z_callback: None, }; drop(self); @@ -321,6 +410,13 @@ impl Drop for Clipper { fn drop(&mut self) { if !self.keep_ptr_on_drop { unsafe { clipper_delete_clipper64(self.ptr) } + + #[cfg(feature = "usingz")] + { + if let Some(raw_cb) = self.raw_z_callback { + drop(unsafe { Box::from_raw(raw_cb as *mut _) }); + } + } } } } @@ -332,3 +428,47 @@ pub enum ClipperError { #[error("Failed boolean operation")] FailedBooleanOperation, } + +#[cfg(test)] +mod test { + use std::{cell::Cell, rc::Rc}; + + use super::*; + + #[cfg(feature = "usingz")] + #[test] + fn test_set_z_callback() { + let mut clipper = Clipper::::new(); + let success = Rc::new(Cell::new(false)); + { + let success = success.clone(); + clipper.set_z_callback( + move |_e1bot: Point<_>, + _e1top: Point<_>, + _e2bot: Point<_>, + _e2top: Point<_>, + p: &mut Point<_>| { + p.set_z(1); + success.set(true); + }, + ); + } + let e1: Paths = vec![(0.0, 0.0), (2.0, 2.0), (0.0, 2.0)].into(); + let e2: Paths = vec![(1.0, 0.0), (2.0, 0.0), (1.0, 2.0)].into(); + let paths = clipper + .add_subject(e1) + .add_clip(e2) + .union(FillRule::default()) + .unwrap(); + + assert_eq!(success.get(), true); + + let n_intersecting = paths + .iter() + .flatten() + .into_iter() + .filter(|v| v.z() == 1) + .count(); + assert_eq!(n_intersecting, 3); + } +} diff --git a/src/path.rs b/src/path.rs index c659d0e..13aa847 100644 --- a/src/path.rs +++ b/src/path.rs @@ -322,6 +322,7 @@ impl Path

{ .map(|point: Point

| ClipperPoint64 { x: point.x_scaled(), y: point.y_scaled(), + ..Default::default() }) .collect::>() .as_mut_ptr(), diff --git a/src/point.rs b/src/point.rs index 747b647..266d218 100644 --- a/src/point.rs +++ b/src/point.rs @@ -97,9 +97,15 @@ pub struct Point( ); impl Point

{ + #[cfg(not(feature = "usingz"))] /// The zero point. pub const ZERO: Self = Self(ClipperPoint64 { x: 0, y: 0 }, PhantomData); + #[cfg(feature = "usingz")] + /// The zero point. + pub const ZERO: Self = Self(ClipperPoint64 { x: 0, y: 0, z: 0 }, PhantomData); + + #[cfg(not(feature = "usingz"))] /// The minimum value for a point. pub const MIN: Self = Self( ClipperPoint64 { @@ -109,6 +115,18 @@ impl Point

{ PhantomData, ); + #[cfg(feature = "usingz")] + /// The minimum value for a point. + pub const MIN: Self = Self( + ClipperPoint64 { + x: i64::MIN, + y: i64::MIN, + z: 0, + }, + PhantomData, + ); + + #[cfg(not(feature = "usingz"))] /// The maximum value for a point. pub const MAX: Self = Self( ClipperPoint64 { @@ -118,12 +136,24 @@ impl Point

{ PhantomData, ); + #[cfg(feature = "usingz")] + /// The maximum value for a point. + pub const MAX: Self = Self( + ClipperPoint64 { + x: i64::MAX, + y: i64::MAX, + z: 0, + }, + PhantomData, + ); + /// Create a new point. pub fn new(x: f64, y: f64) -> Self { Self( ClipperPoint64 { x: P::scale(x) as i64, y: P::scale(y) as i64, + ..Default::default() }, PhantomData, ) @@ -132,7 +162,14 @@ impl Point

{ /// Create a new point from scaled values, this means that point is /// constructed as is without applying the scaling multiplier. pub fn from_scaled(x: i64, y: i64) -> Self { - Self(ClipperPoint64 { x, y }, PhantomData) + Self( + ClipperPoint64 { + x, + y, + ..Default::default() + }, + PhantomData, + ) } /// Returns the x coordinate of the point. @@ -158,6 +195,18 @@ impl Point

{ pub(crate) fn as_clipperpoint64(&self) -> *const ClipperPoint64 { &self.0 } + + #[cfg(feature = "usingz")] + /// Returns the user data of the point. + pub fn z(&self) -> i64 { + self.0.z + } + + #[cfg(feature = "usingz")] + /// Sets the user data of the point. + pub fn set_z(&mut self, z: i64) { + self.0.z = z; + } } impl Default for Point

{ From 063f3762cbdc67dae43d4db4ddfbfedd8fcaed80 Mon Sep 17 00:00:00 2001 From: Martin Svanberg Date: Wed, 28 Aug 2024 15:26:06 +0200 Subject: [PATCH 2/5] Test serde without usingz --- src/path.rs | 2 +- src/paths.rs | 2 +- src/point.rs | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/path.rs b/src/path.rs index 13aa847..a17cf4b 100644 --- a/src/path.rs +++ b/src/path.rs @@ -473,7 +473,7 @@ mod test { assert_eq!(area, -1200.0); } - #[cfg(feature = "serde")] + #[cfg(all(feature = "serde", not(feature = "usingz")))] #[test] fn test_serde() { let path = Path::::from(vec![(0.0, 0.0), (1.0, 1.0)]); diff --git a/src/paths.rs b/src/paths.rs index f5112dd..b86c8fa 100644 --- a/src/paths.rs +++ b/src/paths.rs @@ -563,7 +563,7 @@ mod test { assert_eq!(paths.len(), 2); } - #[cfg(feature = "serde")] + #[cfg(all(feature = "serde", not(feature = "usingz")))] #[test] fn test_serde() { let paths = Paths::::from(vec![(0.4, 0.0), (5.0, 1.0)]); diff --git a/src/point.rs b/src/point.rs index 266d218..9e69185 100644 --- a/src/point.rs +++ b/src/point.rs @@ -292,7 +292,7 @@ mod test { assert_eq!(point.y_scaled(), 4000); } - #[cfg(feature = "serde")] + #[cfg(all(feature = "serde", not(feature = "usingz")))] #[test] fn test_serde() { let point = Point::::new(1.0, 2.0); From 6f8e47bfe10b58a5c797c6abb71f00f9079078ce Mon Sep 17 00:00:00 2001 From: Martin Svanberg Date: Wed, 28 Aug 2024 15:33:38 +0200 Subject: [PATCH 3/5] Fix warnings --- src/clipper.rs | 38 +++++++++++++++++++++++++------------- 1 file changed, 25 insertions(+), 13 deletions(-) diff --git a/src/clipper.rs b/src/clipper.rs index 9fe01e2..9b2d8f5 100644 --- a/src/clipper.rs +++ b/src/clipper.rs @@ -3,10 +3,10 @@ use std::marker::PhantomData; use clipper2c_sys::{ clipper_clipper64, clipper_clipper64_add_clip, clipper_clipper64_add_open_subject, clipper_clipper64_add_subject, clipper_clipper64_execute, clipper_clipper64_size, - clipper_delete_clipper64, clipper_delete_paths64, ClipperClipper64, ClipperPoint64, + clipper_delete_clipper64, clipper_delete_paths64, ClipperClipper64, }; -use crate::{malloc, Centi, ClipType, FillRule, Paths, Point, PointScaler}; +use crate::{malloc, Centi, ClipType, FillRule, Paths, PointScaler}; /// The state of the Clipper struct. pub trait ClipperState {} @@ -137,8 +137,16 @@ impl Clipper { /// ``` pub fn set_z_callback( &mut self, - callback: impl Fn(Point

, Point

, Point

, Point

, &mut Point

), + callback: impl Fn( + crate::Point

, + crate::Point

, + crate::Point

, + crate::Point

, + &mut crate::Point

, + ), ) { + use crate::Point; + // The closure will be represented by a trait object behind a fat // pointer. Since fat pointers are larger than thin pointers, they // cannot be passed through a thin-pointer c_void type. We must @@ -164,17 +172,20 @@ impl Clipper { } } +#[cfg(feature = "usingz")] extern "C" fn handle_set_z_callback( user_data: *mut ::std::os::raw::c_void, - e1bot: *const ClipperPoint64, - e1top: *const ClipperPoint64, - e2bot: *const ClipperPoint64, - e2top: *const ClipperPoint64, - pt: *mut ClipperPoint64, + e1bot: *const clipper2c_sys::ClipperPoint64, + e1top: *const clipper2c_sys::ClipperPoint64, + e2bot: *const clipper2c_sys::ClipperPoint64, + e2top: *const clipper2c_sys::ClipperPoint64, + pt: *mut clipper2c_sys::ClipperPoint64, ) { + use crate::Point; + // SAFETY: user_data was set in set_z_callback, and is valid for as long as // the Clipper2 instance exists. - let callback: &mut &mut dyn Fn(Point

, Point

, Point

, Point

, &mut Point

) = + let callback: &mut &mut dyn Fn(Point

, Point

, Point

, Point

, &mut crate::Point

) = unsafe { std::mem::transmute(user_data) }; // SAFETY: Clipper2 should produce valid pointers @@ -431,13 +442,14 @@ pub enum ClipperError { #[cfg(test)] mod test { - use std::{cell::Cell, rc::Rc}; - - use super::*; - #[cfg(feature = "usingz")] #[test] fn test_set_z_callback() { + use std::{cell::Cell, rc::Rc}; + + use super::*; + use crate::Point; + let mut clipper = Clipper::::new(); let success = Rc::new(Cell::new(false)); { From a06d8905c79e963470346f575857ae65d02f2298 Mon Sep 17 00:00:00 2001 From: Martin Svanberg Date: Wed, 28 Aug 2024 15:57:11 +0200 Subject: [PATCH 4/5] Replace mutable z api with separate constructor --- Cargo.lock | 1 - Cargo.toml | 4 ++-- src/clipper.rs | 4 ++-- src/point.rs | 19 +++++++++++++------ 4 files changed, 17 insertions(+), 11 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a3eacd0..0fe1b9e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -84,7 +84,6 @@ dependencies = [ [[package]] name = "clipper2c-sys" version = "0.1.4" -source = "git+https://github.com/tirithen/clipper2c-sys?rev=210878394637f94bcd3a476bdf24ddf4ba8a39fd#210878394637f94bcd3a476bdf24ddf4ba8a39fd" dependencies = [ "cc", "libc", diff --git a/Cargo.toml b/Cargo.toml index 47adfc3..620b16e 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,7 +19,7 @@ usingz = ["clipper2c-sys/usingz"] [dependencies] libc = "0.2" -clipper2c-sys = "0.1.4" +clipper2c-sys = { path = "../clipper2c-sys" } thiserror = "1.0.59" serde = { version = "1", features = ["derive"], optional = true } @@ -34,4 +34,4 @@ serde_json = "1" features = ["doc-images"] [patch.crates-io] -clipper2c-sys = { rev = "210878394637f94bcd3a476bdf24ddf4ba8a39fd", git = "https://github.com/tirithen/clipper2c-sys" } +# clipper2c-sys = { rev = "210878394637f94bcd3a476bdf24ddf4ba8a39fd", git = "https://github.com/tirithen/clipper2c-sys" } diff --git a/src/clipper.rs b/src/clipper.rs index 9b2d8f5..9d921ed 100644 --- a/src/clipper.rs +++ b/src/clipper.rs @@ -132,7 +132,7 @@ impl Clipper { /// /// let mut clipper = Clipper::::new(); /// clipper.set_z_callback(|_: Point<_>, _: Point<_>, _: Point<_>, _: Point<_>, p: &mut Point<_>| { - /// p.set_z(1); + /// *p = Point::new_with_z(p.x(), p.y(), 1); /// }); /// ``` pub fn set_z_callback( @@ -460,7 +460,7 @@ mod test { _e2bot: Point<_>, _e2top: Point<_>, p: &mut Point<_>| { - p.set_z(1); + *p = Point::new_with_z(p.x(), p.y(), 1); success.set(true); }, ); diff --git a/src/point.rs b/src/point.rs index 9e69185..69059b2 100644 --- a/src/point.rs +++ b/src/point.rs @@ -159,6 +159,19 @@ impl Point

{ ) } + #[cfg(feature = "usingz")] + /// Create a new point with user data. + pub fn new_with_z(x: f64, y: f64, z: i64) -> Self { + Self( + ClipperPoint64 { + x: P::scale(x) as i64, + y: P::scale(y) as i64, + z, + }, + PhantomData, + ) + } + /// Create a new point from scaled values, this means that point is /// constructed as is without applying the scaling multiplier. pub fn from_scaled(x: i64, y: i64) -> Self { @@ -201,12 +214,6 @@ impl Point

{ pub fn z(&self) -> i64 { self.0.z } - - #[cfg(feature = "usingz")] - /// Sets the user data of the point. - pub fn set_z(&mut self, z: i64) { - self.0.z = z; - } } impl Default for Point

{ From 20bfb41094eff788742be017d33120ac88a818df Mon Sep 17 00:00:00 2001 From: Martin Svanberg Date: Wed, 28 Aug 2024 16:04:17 +0200 Subject: [PATCH 5/5] Depend directly on clipper2c-sys branch --- Cargo.lock | 1 + Cargo.toml | 3 +-- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 0fe1b9e..a3eacd0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -84,6 +84,7 @@ dependencies = [ [[package]] name = "clipper2c-sys" version = "0.1.4" +source = "git+https://github.com/tirithen/clipper2c-sys?rev=210878394637f94bcd3a476bdf24ddf4ba8a39fd#210878394637f94bcd3a476bdf24ddf4ba8a39fd" dependencies = [ "cc", "libc", diff --git a/Cargo.toml b/Cargo.toml index 620b16e..e5af61b 100644 --- a/Cargo.toml +++ b/Cargo.toml @@ -19,7 +19,7 @@ usingz = ["clipper2c-sys/usingz"] [dependencies] libc = "0.2" -clipper2c-sys = { path = "../clipper2c-sys" } +clipper2c-sys = { rev = "210878394637f94bcd3a476bdf24ddf4ba8a39fd", git = "https://github.com/tirithen/clipper2c-sys" } thiserror = "1.0.59" serde = { version = "1", features = ["derive"], optional = true } @@ -34,4 +34,3 @@ serde_json = "1" features = ["doc-images"] [patch.crates-io] -# clipper2c-sys = { rev = "210878394637f94bcd3a476bdf24ddf4ba8a39fd", git = "https://github.com/tirithen/clipper2c-sys" }