From afda666735fdc2ba64b804352209cb8a74b1158d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristoffer=20=C3=96dmark?= Date: Thu, 24 Oct 2024 14:10:54 +0200 Subject: [PATCH 1/2] change the boolops trait to result, to allow starting to implement early error returns in the used algorithms --- geo/src/algorithm/bool_ops/mod.rs | 44 ++++++++++++++++++++--------- geo/src/algorithm/bool_ops/tests.rs | 11 ++++---- jts-test-runner/src/runner.rs | 4 +-- 3 files changed, 38 insertions(+), 21 deletions(-) diff --git a/geo/src/algorithm/bool_ops/mod.rs b/geo/src/algorithm/bool_ops/mod.rs index 29d2ad1be5..8d29ae2a2e 100644 --- a/geo/src/algorithm/bool_ops/mod.rs +++ b/geo/src/algorithm/bool_ops/mod.rs @@ -1,7 +1,23 @@ +use std::{error::Error, fmt::Display}; + use geo_types::{MultiLineString, MultiPolygon}; use crate::{CoordsIter, GeoFloat, GeoNum, Polygon}; +#[derive(Debug)] +pub enum BooleanError { + Crash, +} + +impl Display for BooleanError { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str("Crash") + } +} +impl Error for BooleanError {} + +pub type BooleanResult = Result; + /// Boolean Operations on geometry. /// /// Boolean operations are set operations on geometries considered as a subset @@ -25,17 +41,17 @@ use crate::{CoordsIter, GeoFloat, GeoNum, Polygon}; pub trait BooleanOps: Sized { type Scalar: GeoNum; - fn boolean_op(&self, other: &Self, op: OpType) -> MultiPolygon; - fn intersection(&self, other: &Self) -> MultiPolygon { + fn boolean_op(&self, other: &Self, op: OpType) -> BooleanResult>; + fn intersection(&self, other: &Self) -> BooleanResult> { self.boolean_op(other, OpType::Intersection) } - fn union(&self, other: &Self) -> MultiPolygon { + fn union(&self, other: &Self) -> BooleanResult> { self.boolean_op(other, OpType::Union) } - fn xor(&self, other: &Self) -> MultiPolygon { + fn xor(&self, other: &Self) -> BooleanResult> { self.boolean_op(other, OpType::Xor) } - fn difference(&self, other: &Self) -> MultiPolygon { + fn difference(&self, other: &Self) -> BooleanResult> { self.boolean_op(other, OpType::Difference) } @@ -47,7 +63,7 @@ pub trait BooleanOps: Sized { &self, ls: &MultiLineString, invert: bool, - ) -> MultiLineString; + ) -> BooleanResult>; } #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord)] @@ -61,51 +77,51 @@ pub enum OpType { impl BooleanOps for Polygon { type Scalar = T; - fn boolean_op(&self, other: &Self, op: OpType) -> MultiPolygon { + fn boolean_op(&self, other: &Self, op: OpType) -> BooleanResult> { let spec = BoolOp::from(op); let mut bop = Proc::new(spec, self.coords_count() + other.coords_count()); bop.add_polygon(self, 0); bop.add_polygon(other, 1); - bop.sweep() + Ok(bop.sweep()) } fn clip( &self, ls: &MultiLineString, invert: bool, - ) -> MultiLineString { + ) -> BooleanResult> { let spec = ClipOp::new(invert); let mut bop = Proc::new(spec, self.coords_count() + ls.coords_count()); bop.add_polygon(self, 0); ls.0.iter().enumerate().for_each(|(idx, l)| { bop.add_line_string(l, idx + 1); }); - bop.sweep() + Ok(bop.sweep()) } } impl BooleanOps for MultiPolygon { type Scalar = T; - fn boolean_op(&self, other: &Self, op: OpType) -> MultiPolygon { + fn boolean_op(&self, other: &Self, op: OpType) -> BooleanResult> { let spec = BoolOp::from(op); let mut bop = Proc::new(spec, self.coords_count() + other.coords_count()); bop.add_multi_polygon(self, 0); bop.add_multi_polygon(other, 1); - bop.sweep() + Ok(bop.sweep()) } fn clip( &self, ls: &MultiLineString, invert: bool, - ) -> MultiLineString { + ) -> BooleanResult> { let spec = ClipOp::new(invert); let mut bop = Proc::new(spec, self.coords_count() + ls.coords_count()); bop.add_multi_polygon(self, 0); ls.0.iter().enumerate().for_each(|(idx, l)| { bop.add_line_string(l, idx + 1); }); - bop.sweep() + Ok(bop.sweep()) } } diff --git a/geo/src/algorithm/bool_ops/tests.rs b/geo/src/algorithm/bool_ops/tests.rs index 11373dcbdf..6bbd3dccbe 100644 --- a/geo/src/algorithm/bool_ops/tests.rs +++ b/geo/src/algorithm/bool_ops/tests.rs @@ -179,7 +179,7 @@ fn test_clip_adhoc() -> Result<()> { let mls = MultiLineString::try_from_wkt_str(wkt2) .or_else(|_| LineString::::try_from_wkt_str(wkt2).map(MultiLineString::from)) .unwrap(); - let output = poly1.clip(&mls, true); + let output = poly1.clip(&mls, true)?; eprintln!("{wkt}", wkt = output.to_wkt()); Ok(()) } @@ -209,16 +209,17 @@ fn test_issue_885_big_simplified() -> Result<()> { } #[test] -fn test_issue_894() { +fn test_issue_894() -> Result<()> { use geo_test_fixtures::multi_polygon; let a: MultiPolygon = multi_polygon("issue-894/inpa.wkt"); let b = multi_polygon("issue-894/inpb.wkt"); let c = multi_polygon("issue-894/inpc.wkt"); - let aib = a.intersection(&b); // works - b.intersection(&c); // works - let intersection = aib.intersection(&c); + let aib = a.intersection(&b)?; // works + b.intersection(&c)?; // works + let intersection = aib.intersection(&c)?; println!("{}", intersection.to_wkt()); + Ok(()) } #[test] diff --git a/jts-test-runner/src/runner.rs b/jts-test-runner/src/runner.rs index eacf3946fd..d7a27984d7 100644 --- a/jts-test-runner/src/runner.rs +++ b/jts-test-runner/src/runner.rs @@ -324,9 +324,9 @@ impl TestRunner { }; let actual = match (a, b) { - (Geometry::Polygon(a), Geometry::Polygon(b)) => a.boolean_op(b, *op), + (Geometry::Polygon(a), Geometry::Polygon(b)) => a.boolean_op(b, *op)?, (Geometry::MultiPolygon(a), Geometry::MultiPolygon(b)) => { - a.boolean_op(b, *op) + a.boolean_op(b, *op)? } _ => { info!("skipping unsupported Union combination: {:?}, {:?}", a, b); From d2dcb8213f8ea09c558f4cc7e7587331ebad9990 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Kristoffer=20=C3=96dmark?= Date: Thu, 24 Oct 2024 14:14:19 +0200 Subject: [PATCH 2/2] fix changelog --- geo/CHANGES.md | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/geo/CHANGES.md b/geo/CHANGES.md index 4b7c7895d9..8c73db7ccd 100644 --- a/geo/CHANGES.md +++ b/geo/CHANGES.md @@ -1,7 +1,8 @@ # Changes ## Unreleased - +* Change BooleanOps trait to return an ErrorType instead of crashing when given bad input + * * Implement getter methods on `AffineTransform` to access internal elements. * * Fix issue in Debug impl for AffineTransform where yoff is shown instead of xoff