From c07769793e7e64bc7054d4b3ac6006d85a2d96ff Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Mon, 25 Nov 2024 16:24:35 -0800 Subject: [PATCH 1/3] Upgrade to i_overlay 1.8 i_overlay 1.8 introduced a new trait based approach which allows us to remove some of our own trait juggling. However, our old friend, the orphan trait rule, prevents this from happening easily. For now, I'm creating wrapper structs in geo for this algo. i_overlay 1.8 also introduced a new method signature for doing bool_ops, removing the old ones. This caused our trait juggling to infinitely recurse: See https://github.com/georust/geo/issues/1270 Now that we're using the new methods, the problem is avoided. --- geo/Cargo.toml | 2 +- .../bool_ops/i_overlay_integration.rs | 277 ++---------------- geo/src/algorithm/bool_ops/mod.rs | 62 ++-- geo/src/algorithm/bool_ops/tests.rs | 10 +- 4 files changed, 61 insertions(+), 290 deletions(-) diff --git a/geo/Cargo.toml b/geo/Cargo.toml index 005a55386..7867e94e6 100644 --- a/geo/Cargo.toml +++ b/geo/Cargo.toml @@ -31,7 +31,7 @@ proj = { version = "0.27.0", optional = true } robust = "1.1.0" rstar = "0.12.0" serde = { version = "1.0", optional = true, features = ["derive"] } -i_overlay = { version = "1.7.2, < 1.8.0", default-features = false } +i_overlay = { version = "1.8.0, < 1.9.0", default-features = false } [dev-dependencies] approx = ">= 0.4.0, < 0.6.0" diff --git a/geo/src/algorithm/bool_ops/i_overlay_integration.rs b/geo/src/algorithm/bool_ops/i_overlay_integration.rs index bb78254a7..2ad867a13 100644 --- a/geo/src/algorithm/bool_ops/i_overlay_integration.rs +++ b/geo/src/algorithm/bool_ops/i_overlay_integration.rs @@ -1,290 +1,71 @@ use crate::geometry::Coord; use crate::GeoNum; -use i_overlay::core::fill_rule::FillRule; -use i_overlay::core::overlay::ShapeType; -use i_overlay::core::overlay_rule::OverlayRule; -use i_overlay::string::clip::ClipRule; +use i_overlay::i_float::float::compatible::FloatPointCompatible; +use i_overlay::i_float::float::number::FloatNumber; -pub trait BoolOpsCoord: Copy { - fn new(x: T, y: T) -> Self; - fn x(&self) -> T; - fn y(&self) -> T; -} - -/// A geometry coordinate number suitable for performing geometric boolean operations. -pub trait BoolOpsNum: GeoNum { - type CoordType: BoolOpsCoord; - type OverlayType: BoolOpsOverlay; - type StringOverlayType: BoolOpsStringOverlay; - - fn to_bops_coord(geo_coord: Coord) -> Self::CoordType { - Self::CoordType::new(geo_coord.x, geo_coord.y) - } - - fn to_geo_coord(bops_coord: Self::CoordType) -> Coord { - Coord { - x: bops_coord.x(), - y: bops_coord.y(), - } - } -} - -pub trait BoolOpsOverlay { - type CoordType; - type OverlayGraph: BoolOpsOverlayGraph; - fn new() -> Self; - fn add_path(&mut self, path: Vec, shape_type: ShapeType); - fn into_graph(self, fill_rule: FillRule) -> Self::OverlayGraph; -} - -pub(super) trait BoolOpsOverlayGraph { - type CoordType; - fn extract_shapes(&self, overlay_rule: OverlayRule) -> Vec>>; -} - -pub trait BoolOpsStringOverlay { - type CoordType; - type StringGraph: BoolOpsStringGraph; - fn new() -> Self; - fn add_shape_path(&mut self, path: Vec); - fn add_string_line(&mut self, path: [Self::CoordType; 2]); - fn into_graph(self, fill_rule: FillRule) -> Self::StringGraph; -} - -pub(super) trait BoolOpsStringGraph { - type CoordType; - fn clip_string_lines(&self, clip_rule: ClipRule) -> Vec>; -} - -mod f64 { - use super::{ClipRule, FillRule, OverlayRule, ShapeType}; - use i_overlay::f64::{ - graph::F64OverlayGraph, - overlay::F64Overlay, - string::{F64StringGraph, F64StringOverlay}, - }; - use i_overlay::i_float::f64_point::F64Point; - - impl super::BoolOpsNum for f64 { - type CoordType = F64Point; - type OverlayType = F64Overlay; - type StringOverlayType = F64StringOverlay; - } - - impl super::BoolOpsCoord for F64Point { - #[inline] - fn new(x: f64, y: f64) -> Self { - Self::new(x, y) - } - - #[inline] - fn x(&self) -> f64 { - self.x - } - - #[inline] - fn y(&self) -> f64 { - self.y - } - } - - impl super::BoolOpsOverlay for F64Overlay { - type CoordType = F64Point; - type OverlayGraph = F64OverlayGraph; - - #[inline] - fn new() -> Self { - Self::new() - } - - #[inline] - fn add_path(&mut self, path: Vec, shape_type: ShapeType) { - self.add_path(path, shape_type) - } - - #[inline] - fn into_graph(self, fill_rule: FillRule) -> Self::OverlayGraph { - self.into_graph(fill_rule) - } - } - - impl super::BoolOpsOverlayGraph for F64OverlayGraph { - type CoordType = F64Point; - - #[inline] - fn extract_shapes(&self, overlay_rule: OverlayRule) -> Vec>> { - self.extract_shapes(overlay_rule) - } - } - - impl super::BoolOpsStringOverlay for F64StringOverlay { - type CoordType = F64Point; - type StringGraph = F64StringGraph; - - #[inline] - fn new() -> Self { - Self::new() - } +/// A geometry coordinate scalar suitable for performing geometric boolean operations. +pub trait BoolOpsNum: GeoNum + FloatNumber {} +impl BoolOpsNum for T {} - #[inline] - fn add_shape_path(&mut self, path: Vec) { - self.add_shape_path(path) - } - - #[inline] - fn add_string_line(&mut self, path: [Self::CoordType; 2]) { - self.add_string_line(path) - } +/// New type for `Coord` that implements `FloatPointCompatible` for `BoolOpsNum` to +/// circumvent orphan rule, since Coord is defined in geo_types. +#[derive(Copy, Clone, Debug)] +pub struct BoolOpsCoord(pub(crate) Coord); - #[inline] - fn into_graph(self, fill_rule: FillRule) -> Self::StringGraph { - self.into_graph(fill_rule) - } +impl FloatPointCompatible for BoolOpsCoord { + fn from_xy(x: T, y: T) -> Self { + Self(Coord { x, y }) } - impl super::BoolOpsStringGraph for F64StringGraph { - type CoordType = F64Point; - - #[inline] - fn clip_string_lines(&self, clip_rule: ClipRule) -> Vec> { - self.clip_string_lines(clip_rule) - } + fn x(&self) -> T { + self.0.x } -} - -mod f32 { - use i_overlay::core::fill_rule::FillRule; - use i_overlay::core::overlay::ShapeType; - use i_overlay::core::overlay_rule::OverlayRule; - use i_overlay::f32::graph::F32OverlayGraph; - use i_overlay::f32::overlay::F32Overlay; - use i_overlay::f32::string::{F32StringGraph, F32StringOverlay}; - use i_overlay::i_float::f32_point::F32Point; - use i_overlay::string::clip::ClipRule; - impl super::BoolOpsNum for f32 { - type CoordType = F32Point; - type OverlayType = F32Overlay; - type StringOverlayType = F32StringOverlay; - } - - impl super::BoolOpsCoord for F32Point { - #[inline] - fn new(x: f32, y: f32) -> Self { - Self::new(x, y) - } - #[inline] - fn x(&self) -> f32 { - self.x - } - #[inline] - fn y(&self) -> f32 { - self.y - } - } - - impl super::BoolOpsOverlay for F32Overlay { - type CoordType = F32Point; - type OverlayGraph = F32OverlayGraph; - - #[inline] - fn new() -> Self { - Self::new() - } - - #[inline] - fn add_path(&mut self, path: Vec, shape_type: ShapeType) { - self.add_path(path, shape_type) - } - - #[inline] - fn into_graph(self, fill_rule: FillRule) -> Self::OverlayGraph { - self.into_graph(fill_rule) - } - } - - impl super::BoolOpsOverlayGraph for F32OverlayGraph { - type CoordType = F32Point; - - #[inline] - fn extract_shapes(&self, overlay_rule: OverlayRule) -> Vec>> { - self.extract_shapes(overlay_rule) - } - } - - impl super::BoolOpsStringOverlay for F32StringOverlay { - type CoordType = F32Point; - type StringGraph = F32StringGraph; - - #[inline] - fn new() -> Self { - Self::new() - } - - #[inline] - fn add_shape_path(&mut self, path: Vec) { - self.add_shape_path(path) - } - - #[inline] - fn add_string_line(&mut self, path: [Self::CoordType; 2]) { - self.add_string_line(path) - } - - #[inline] - fn into_graph(self, fill_rule: FillRule) -> Self::StringGraph { - self.into_graph(fill_rule) - } - } - - impl super::BoolOpsStringGraph for F32StringGraph { - type CoordType = F32Point; - - #[inline] - fn clip_string_lines(&self, clip_rule: ClipRule) -> Vec> { - self.clip_string_lines(clip_rule) - } + fn y(&self) -> T { + self.0.y } } pub(super) mod convert { use super::super::OpType; - use super::{BoolOpsNum, OverlayRule}; + use super::BoolOpsNum; + use crate::bool_ops::i_overlay_integration::BoolOpsCoord; use crate::geometry::{LineString, MultiLineString, MultiPolygon, Polygon}; + use i_overlay::core::overlay_rule::OverlayRule; - pub fn line_string_from_path(path: Vec) -> LineString { - let coords = path.into_iter().map(T::to_geo_coord); - LineString(coords.collect()) + pub fn line_string_from_path(path: Vec>) -> LineString { + let coords = path.into_iter().map(|bops_coord| bops_coord.0).collect(); + LineString(coords) } pub fn multi_line_string_from_paths( - paths: Vec>, + paths: Vec>>, ) -> MultiLineString { let line_strings = paths.into_iter().map(|p| line_string_from_path(p)); MultiLineString(line_strings.collect()) } - pub fn polygon_from_shape(shape: Vec>) -> Polygon { + pub fn polygon_from_shape(shape: Vec>>) -> Polygon { let mut rings = shape.into_iter().map(|p| line_string_from_path(p)); let exterior = rings.next().unwrap_or(LineString::new(vec![])); Polygon::new(exterior, rings.collect()) } pub fn multi_polygon_from_shapes( - shapes: Vec>>, + shapes: Vec>>>, ) -> MultiPolygon { let polygons = shapes.into_iter().map(|s| polygon_from_shape(s)); MultiPolygon(polygons.collect()) } - pub fn ring_to_shape_path(line_string: &LineString) -> Vec { + pub fn ring_to_shape_path(line_string: &LineString) -> Vec> { if line_string.0.is_empty() { return vec![]; } // In geo, Polygon rings are explicitly closed LineStrings — their final coordinate is the same as their first coordinate, // however in i_overlay, shape paths are implicitly closed, so we skip the last coordinate. let coords = &line_string.0[..line_string.0.len() - 1]; - coords.iter().copied().map(T::to_bops_coord).collect() + coords.iter().copied().map(BoolOpsCoord).collect() } impl From for OverlayRule { @@ -308,7 +89,7 @@ mod tests { #[test] fn two_empty_polygons() { let p1: Polygon = wkt!(POLYGON EMPTY); - let p2 = wkt!(POLYGON EMPTY); + let p2: Polygon = wkt!(POLYGON EMPTY); assert_eq!(&p1.union(&p2), &wkt!(MULTIPOLYGON EMPTY)); assert_eq!(&p1.intersection(&p2), &wkt!(MULTIPOLYGON EMPTY)); } @@ -316,7 +97,7 @@ mod tests { #[test] fn one_empty_polygon() { let p1: Polygon = wkt!(POLYGON((0. 0., 0. 1., 1. 1., 1. 0., 0. 0.))); - let p2 = wkt!(POLYGON EMPTY); + let p2: Polygon = wkt!(POLYGON EMPTY); assert_eq!(&p1.union(&p2), &MultiPolygon(vec![p1.clone()])); assert_eq!(&p1.intersection(&p2), &wkt!(MULTIPOLYGON EMPTY)); } diff --git a/geo/src/algorithm/bool_ops/mod.rs b/geo/src/algorithm/bool_ops/mod.rs index 16287b049..e0debd80a 100644 --- a/geo/src/algorithm/bool_ops/mod.rs +++ b/geo/src/algorithm/bool_ops/mod.rs @@ -2,6 +2,14 @@ mod i_overlay_integration; #[cfg(test)] mod tests; +use crate::bool_ops::i_overlay_integration::convert::{ + multi_polygon_from_shapes, ring_to_shape_path, +}; +use crate::bool_ops::i_overlay_integration::BoolOpsCoord; +use i_overlay::core::fill_rule::FillRule; +use i_overlay::float::clip::FloatClip; +use i_overlay::float::single::SingleFloatOverlay; +use i_overlay::string::clip::ClipRule; pub use i_overlay_integration::BoolOpsNum; use crate::geometry::{LineString, MultiLineString, MultiPolygon, Polygon}; @@ -43,22 +51,10 @@ pub trait BooleanOps { other: &impl BooleanOps, op: OpType, ) -> MultiPolygon { - use i_overlay::core::fill_rule::FillRule; - use i_overlay::core::overlay::ShapeType; - use i_overlay_integration::{convert, BoolOpsOverlay, BoolOpsOverlayGraph}; - let mut overlay = ::OverlayType::new(); - - for ring in self.rings() { - overlay.add_path(convert::ring_to_shape_path(ring), ShapeType::Subject); - } - for ring in other.rings() { - overlay.add_path(convert::ring_to_shape_path(ring), ShapeType::Clip); - } - - let graph = overlay.into_graph(FillRule::EvenOdd); - let shapes = graph.extract_shapes(op.into()); - - convert::multi_polygon_from_shapes(shapes) + let subject = self.rings().map(ring_to_shape_path).collect::>(); + let clip = other.rings().map(ring_to_shape_path).collect::>(); + let shapes = subject.overlay(&clip, op.into(), FillRule::EvenOdd); + multi_polygon_from_shapes(shapes) } fn intersection( @@ -89,31 +85,19 @@ pub trait BooleanOps { multi_line_string: &MultiLineString, invert: bool, ) -> MultiLineString { - use i_overlay::core::fill_rule::FillRule; - use i_overlay::string::clip::ClipRule; - use i_overlay_integration::{convert, BoolOpsStringGraph, BoolOpsStringOverlay}; - - let mut overlay = ::StringOverlayType::new(); + let subject: Vec> = multi_line_string + .iter() + .map(|line_string| line_string.coords().map(|c| BoolOpsCoord(*c)).collect()) + .collect(); - for ring in self.rings() { - overlay.add_shape_path(convert::ring_to_shape_path(ring)); - } - for line_string in multi_line_string { - for line in line_string.lines() { - let line = [ - Self::Scalar::to_bops_coord(line.start), - Self::Scalar::to_bops_coord(line.end), - ]; - overlay.add_string_line(line) - } - } + let clip = self.rings().map(ring_to_shape_path).collect::>(); - let graph = overlay.into_graph(FillRule::EvenOdd); - let paths = graph.clip_string_lines(ClipRule { + let clip_rule = ClipRule { invert, boundary_included: true, - }); - convert::multi_line_string_from_paths(paths) + }; + let paths = subject.clip_by(&clip, FillRule::EvenOdd, clip_rule); + i_overlay_integration::convert::multi_line_string_from_paths(paths) } } @@ -129,7 +113,7 @@ impl BooleanOps for Polygon { type Scalar = T; fn rings(&self) -> impl Iterator> { - std::iter::once(self.exterior()).chain(self.interiors().iter()) + std::iter::once(self.exterior()).chain(self.interiors()) } } @@ -137,6 +121,6 @@ impl BooleanOps for MultiPolygon { type Scalar = T; fn rings(&self) -> impl Iterator> { - self.0.iter().flat_map(|p| p.rings()) + self.iter().flat_map(BooleanOps::rings) } } diff --git a/geo/src/algorithm/bool_ops/tests.rs b/geo/src/algorithm/bool_ops/tests.rs index 4fac6d0e6..ffc4c059a 100644 --- a/geo/src/algorithm/bool_ops/tests.rs +++ b/geo/src/algorithm/bool_ops/tests.rs @@ -1,5 +1,6 @@ use super::BooleanOps; use crate::{wkt, Convert, MultiPolygon, Relate}; +use wkt::ToWkt; #[test] fn jts_test_overlay_la_1() { @@ -50,7 +51,12 @@ fn jts_test_overlay_la_1() { .convert(); let im = actual.relate(&expected); - assert!(im.is_equal_topo()); + assert!( + im.is_equal_topo(), + "actual: {:#?}, expected: {:#?}", + actual.wkt_string(), + expected.wkt_string() + ); } mod gh_issues { @@ -181,7 +187,7 @@ mod gh_issues { wkt!(POLYGON((3.3493652 -55.80127,171.22443 -300.,195.83728 -300.,-46.650635 30.80127,3.3493652 -55.80127))), ]; - let mut multi = MultiPolygon::new(Vec::new()); + let mut multi: MultiPolygon = MultiPolygon::new(Vec::new()); for poly in polygons { multi = multi.union(&MultiPolygon::new(vec![poly])); } From 759fa4cf6fe17c46490612463f048f692f03a10c Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Mon, 25 Nov 2024 20:00:14 -0800 Subject: [PATCH 2/3] update to i_overlay 1.9 Note there is a serious performance regression for larger geometries in our benchmarks vs. 1.8. cargo bench --bench=boolean_ops -- --baseline=main ... small ones are fine, but big ones have huge regression Circular polygon boolean-ops/bops::union/1024 time: [4.3625 ms 4.3922 ms 4.4093 ms] change: [+1.7493% +2.4971% +3.2026%] (p = 0.00 < 0.05) Performance has regressed. Circular polygon boolean-ops/bops::intersection/2048 time: [14.057 ms 14.115 ms 14.157 ms] change: [+9.0224% +10.438% +11.658%] (p = 0.00 < 0.05) Performance has regressed. Circular polygon boolean-ops/bops::union/2048 time: [14.012 ms 14.071 ms 14.134 ms] change: [+11.212% +12.141% +13.317%] (p = 0.00 < 0.05) Performance has regressed. Found 1 outliers among 10 measurements (10.00%) 1 (10.00%) high severe Circular polygon boolean-ops/bops::intersection/4096 time: [360.66 ms 361.18 ms 361.83 ms] change: [+709.38% +713.12% +716.34%] (p = 0.00 < 0.05) Performance has regressed. Found 2 outliers among 10 measurements (20.00%) 2 (20.00%) high severe Circular polygon boolean-ops/bops::union/4096 time: [361.35 ms 361.71 ms 362.13 ms] change: [+710.58% +713.76% +716.59%] (p = 0.00 < 0.05) Performance has regressed. Likely this is because there is a new heuristic for "solver" strategies based on how big your input is - the cliff likely indicates the switch to one of the solvers used for larger inputs. See https://github.com/iShape-Rust/iOverlay/issues/13 There are significant gains with some "real world" data, e.g. urshrei's cascaded union tests: https://github.com/georust/geo/issues/1273#issuecomment-2500778924 I'm more inclined to trust that more realistic data over the synthetic geometries we use in our benchmarks. Plus, I'd prefer to leave in performance twiddling to upstream and contribute changes there rather than fine-tuning things at our own call sites. --- geo/Cargo.toml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/geo/Cargo.toml b/geo/Cargo.toml index 7867e94e6..1c40d91ff 100644 --- a/geo/Cargo.toml +++ b/geo/Cargo.toml @@ -31,7 +31,7 @@ proj = { version = "0.27.0", optional = true } robust = "1.1.0" rstar = "0.12.0" serde = { version = "1.0", optional = true, features = ["derive"] } -i_overlay = { version = "1.8.0, < 1.9.0", default-features = false } +i_overlay = { version = "1.9.0, < 1.10.0", default-features = false } [dev-dependencies] approx = ">= 0.4.0, < 0.6.0" From dda94ebaa955fa5874b522d3960fe69c0d4dbd01 Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Wed, 27 Nov 2024 11:51:56 -0800 Subject: [PATCH 3/3] changelog --- geo/CHANGES.md | 3 +++ 1 file changed, 3 insertions(+) diff --git a/geo/CHANGES.md b/geo/CHANGES.md index d032c522e..4d776d9bb 100644 --- a/geo/CHANGES.md +++ b/geo/CHANGES.md @@ -2,6 +2,9 @@ ## Unreleased +- Fix crash in `BoolOps` by updating `i_overlay` to 1.9.0. + - + ## 0.29.2 - 2024.11.15 - Pin `i_overlay` to < 1.8.0 to work around [recursion bug](https://github.com/georust/geo/issues/1270).