From 6b7874b34ef221c113ae8e87ecbf80a43ef51d7c Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Fri, 11 Oct 2024 17:12:28 -0700 Subject: [PATCH 1/2] Fix `Contains` edge cases for EMPTY geometries And add new tests cases from jts@6a9af07059671bdc6e62542c9739137ab53fd4d8 --- geo/CHANGES.md | 3 + geo/src/algorithm/contains/line_string.rs | 5 +- geo/src/algorithm/contains/polygon.rs | 5 +- .../testxml/misc/TestRelateEmpty.xml | 1114 +++++++++++++++++ jts-test-runner/src/lib.rs | 2 +- jts-test-runner/src/runner.rs | 2 + 6 files changed, 1128 insertions(+), 3 deletions(-) create mode 100644 jts-test-runner/resources/testxml/misc/TestRelateEmpty.xml diff --git a/geo/CHANGES.md b/geo/CHANGES.md index 882a0da474..88ef46d917 100644 --- a/geo/CHANGES.md +++ b/geo/CHANGES.md @@ -40,6 +40,9 @@ * * Change IntersectionMatrix::is_equal_topo to now consider empty geometries as equal. * +* Fix `(LINESTRING EMPTY).contains(LINESTRING EMPTY)` and `(MULTIPOLYGON EMPTY).contains(MULTIPOINT EMPTY)` which previously + reported true + * ## 0.28.0 diff --git a/geo/src/algorithm/contains/line_string.rs b/geo/src/algorithm/contains/line_string.rs index 6378e0a93f..06c836064a 100644 --- a/geo/src/algorithm/contains/line_string.rs +++ b/geo/src/algorithm/contains/line_string.rs @@ -1,7 +1,7 @@ use super::{impl_contains_from_relate, impl_contains_geometry_for, Contains}; use crate::algorithm::Intersects; use crate::geometry::*; -use crate::{CoordNum, GeoFloat, GeoNum}; +use crate::{CoordNum, GeoFloat, GeoNum, HasDimensions}; // ┌────────────────────────────────┐ // │ Implementations for LineString │ @@ -110,6 +110,9 @@ where T: GeoNum, { fn contains(&self, rhs: &LineString) -> bool { + if self.is_empty() || rhs.is_empty() { + return false; + } rhs.lines().all(|l| self.contains(&l)) } } diff --git a/geo/src/algorithm/contains/polygon.rs b/geo/src/algorithm/contains/polygon.rs index d707c84a58..28d53f8c7f 100644 --- a/geo/src/algorithm/contains/polygon.rs +++ b/geo/src/algorithm/contains/polygon.rs @@ -1,7 +1,7 @@ use super::{impl_contains_from_relate, impl_contains_geometry_for, Contains}; use crate::geometry::*; -use crate::Relate; use crate::{GeoFloat, GeoNum}; +use crate::{HasDimensions, Relate}; // ┌─────────────────────────────┐ // │ Implementations for Polygon │ @@ -53,6 +53,9 @@ where impl Contains> for MultiPolygon { fn contains(&self, rhs: &MultiPoint) -> bool { + if self.is_empty() || rhs.is_empty() { + return false; + } rhs.iter().all(|point| self.contains(point)) } } diff --git a/jts-test-runner/resources/testxml/misc/TestRelateEmpty.xml b/jts-test-runner/resources/testxml/misc/TestRelateEmpty.xml new file mode 100644 index 0000000000..6ed6128208 --- /dev/null +++ b/jts-test-runner/resources/testxml/misc/TestRelateEmpty.xml @@ -0,0 +1,1114 @@ + + + Test relate predicates against cases containing EMPTY geometries. + + + + + P/P - empty point VS empty point + + POINT EMPTY + + + POINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + P/L - empty point VS empty line + + POINT EMPTY + + + LINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + P/A - empty point VS empty polygon + + POINT EMPTY + + + POLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + P/mP - empty Point VS empty MultiPoint + + POINT EMPTY + + + MULTIPOINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + P/mL - empty Point VS empty MultiLineString + + POINT EMPTY + + + MULTILINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + P/mA - empty Point VS empty MultiPolygon + + POINT EMPTY + + + MULTIPOLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + P/GC - empty Point VS empty GC + + POINT EMPTY + + + GEOMETRYCOLLECTION EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + + L/P - empty line VS empty point + + LINESTRING EMPTY + + + POINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + L/L - empty line VS empty line + + LINESTRING EMPTY + + + LINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + L/A - empty line VS empty polygon + + LINESTRING EMPTY + + + POLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + L/mP - empty line VS empty MultiPoint + + LINESTRING EMPTY + + + MULTIPOINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + L/mL - empty line VS empty MultiLineString + + LINESTRING EMPTY + + + MULTILINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + L/mA - empty line VS empty MultiPolygon + + LINESTRING EMPTY + + + MULTIPOLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + L/GC - empty line VS empty GC + + LINESTRING EMPTY + + + GEOMETRYCOLLECTION EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + + + A/P - empty polygon VS empty line + + POLYGON EMPTY + + + POINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + A/L - empty polygon VS empty line + + POLYGON EMPTY + + + LINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + A/A - empty polygon VS empty polygon + + POLYGON EMPTY + + + POLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + A/mP - empty polygon VS empty MultiPoint + + POLYGON EMPTY + + + MULTIPOINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + A/mP - empty polygon VS empty MultiLineString + + POLYGON EMPTY + + + MULTILINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + A/mP - empty polygon VS empty MultiPolygon + + POLYGON EMPTY + + + MULTIPOLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + A/mP - empty polygon VS empty GC + + POLYGON EMPTY + + + GEOMETRYCOLLECTION EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + + mP/P - empty MultiPoint VS empty point + + MULTIPOINT EMPTY + + + POINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mP/L - empty MultiPoint VS empty line + + MULTIPOINT EMPTY + + + LINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mP/A - empty MultiPoint VS empty polygon + + MULTIPOINT EMPTY + + + POLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mP/mP - empty MultiPoint VS empty MultiPoint + + MULTIPOINT EMPTY + + + MULTIPOINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mP/mL - empty MultiPoint VS empty MultiLineString + + MULTIPOINT EMPTY + + + MULTILINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mP/mA - empty MultiPoint VS empty MultiPolygon + + MULTIPOINT EMPTY + + + MULTIPOLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mP/GC - empty MultiPoint VS empty GC + + MULTIPOINT EMPTY + + + GEOMETRYCOLLECTION EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + + mL/P - empty Multiline VS empty point + + MULTILINESTRING EMPTY + + + POINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mL/L - empty Multiline VS empty line + + MULTILINESTRING EMPTY + + + LINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mL/A - empty Multiline VS empty polygon + + MULTILINESTRING EMPTY + + + POLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mL/mP - empty Multiline VS empty MultiPoint + + MULTILINESTRING EMPTY + + + MULTIPOINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mL/mL - empty Multiline VS empty MultiLineString + + MULTILINESTRING EMPTY + + + MULTILINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mL/mA - empty Multiline VS empty MultiPolygon + + MULTILINESTRING EMPTY + + + MULTIPOLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mL/GC - empty Multiline VS empty GC + + MULTILINESTRING EMPTY + + + GEOMETRYCOLLECTION EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + + + mA/P - empty Multipolygon VS empty line + + MULTIPOLYGON EMPTY + + + POINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mA/L - empty Multipolygon VS empty line + + MULTIPOLYGON EMPTY + + + LINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mA/A - empty Multipolygon VS empty polygon + + MULTIPOLYGON EMPTY + + + POLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mA/mP - empty Multipolygon VS empty MultiPoint + + MULTIPOLYGON EMPTY + + + MULTIPOINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mA/mL - empty Multipolygon VS empty MultiLineString + + MULTIPOLYGON EMPTY + + + MULTILINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mA/mA - empty Multipolygon VS empty MultiPolygon + + MULTIPOLYGON EMPTY + + + MULTIPOLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + mA/GC - empty Multipolygon VS empty GC + + MULTIPOLYGON EMPTY + + + GEOMETRYCOLLECTION EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + + + GC/P - empty GC VS empty line + + GEOMETRYCOLLECTION EMPTY + + + POINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + GC/L - empty GC VS empty line + + GEOMETRYCOLLECTION EMPTY + + + LINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + GC/A - empty GC VS empty polygon + + GEOMETRYCOLLECTION EMPTY + + + POLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + GC/mP - empty GC VS empty MultiPoint + + GEOMETRYCOLLECTION EMPTY + + + MULTIPOINT EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + GC/mL - empty GC VS empty MultiLineString + + GEOMETRYCOLLECTION EMPTY + + + MULTILINESTRING EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + GC/mA - empty GC VS empty MultiPolygon + + GEOMETRYCOLLECTION EMPTY + + + MULTIPOLYGON EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + GC/GC - empty GC VS empty GC + + GEOMETRYCOLLECTION EMPTY + + + GEOMETRYCOLLECTION EMPTY + + true + false + false + false + false + true + true + false + false + false + false + + + + + + P/P - empty Point VS Point + + POINT EMPTY + + + POINT (1 1) + + true + false + false + false + false + true + false + false + false + false + false + + + + + P/L - empty Point VS LineString + + POINT EMPTY + + + LINESTRING (1 1, 2 2) + + true + false + false + false + false + true + false + false + false + false + false + + + + P/L - empty Point VS Polygon + + POINT EMPTY + + + POLYGON ((1 1, 1 2, 2 1, 1 1)) + + true + false + false + false + false + true + false + false + false + false + false + + + + + + diff --git a/jts-test-runner/src/lib.rs b/jts-test-runner/src/lib.rs index a765e4817d..2a7fceb048 100644 --- a/jts-test-runner/src/lib.rs +++ b/jts-test-runner/src/lib.rs @@ -74,7 +74,7 @@ mod tests { // // We'll need to increase this number as more tests are added, but it should never be // decreased. - let expected_test_count: usize = 2697; + let expected_test_count: usize = 2957; let actual_test_count = runner.failures().len() + runner.successes().len(); match actual_test_count.cmp(&expected_test_count) { Ordering::Less => { diff --git a/jts-test-runner/src/runner.rs b/jts-test-runner/src/runner.rs index e01579d150..eacf3946fd 100644 --- a/jts-test-runner/src/runner.rs +++ b/jts-test-runner/src/runner.rs @@ -12,6 +12,7 @@ use geo::{GeoNum, Relate}; const GENERAL_TEST_XML: Dir = include_dir!("$CARGO_MANIFEST_DIR/resources/testxml/general"); const VALIDATE_TEST_XML: Dir = include_dir!("$CARGO_MANIFEST_DIR/resources/testxml/validate"); +const MISC_TEST_XML: Dir = include_dir!("$CARGO_MANIFEST_DIR/resources/testxml/misc"); #[derive(Debug, Default, Clone)] pub struct TestRunner { @@ -376,6 +377,7 @@ impl TestRunner { for entry in GENERAL_TEST_XML .find(&filename_filter)? .chain(VALIDATE_TEST_XML.find(&filename_filter)?) + .chain(MISC_TEST_XML.find(&filename_filter)?) { let file = match entry { DirEntry::Dir(_) => { From a93d68cbfc9441384f76883ddac5c18222a3416a Mon Sep 17 00:00:00 2001 From: Michael Kirk Date: Fri, 11 Oct 2024 18:19:58 -0700 Subject: [PATCH 2/2] benchmarks showing why we bother having custom Contains impl rather than always using the (typically more correct Relate.is_contains) AFAIK there are no remaining correctness issues, but their has been a long tail of bugs with anything not using the (slower) Relate trait. --- geo/benches/contains.rs | 96 ++++++++++++++++++++++++++++++++++++++++- 1 file changed, 94 insertions(+), 2 deletions(-) diff --git a/geo/benches/contains.rs b/geo/benches/contains.rs index 7d32178754..2632f6c205 100644 --- a/geo/benches/contains.rs +++ b/geo/benches/contains.rs @@ -1,6 +1,7 @@ use criterion::{criterion_group, criterion_main, Criterion}; -use geo::contains::Contains; -use geo::{coord, point, polygon, Line, Point, Polygon, Rect, Triangle}; +use geo::algorithm::{Contains, Convert, Relate}; +use geo::geometry::*; +use geo::{coord, point, polygon}; fn criterion_benchmark(c: &mut Criterion) { c.bench_function("point in simple polygon", |bencher| { @@ -145,6 +146,97 @@ fn criterion_benchmark(c: &mut Criterion) { assert!(criterion::black_box(&rect).contains(criterion::black_box(&polygon))); }); }); + + c.bench_function( + "LineString not contains LineString (Contains trait)", + |bencher| { + let ls_1: geo::LineString = geo_test_fixtures::poly1(); + let ls_2: geo::LineString = geo_test_fixtures::poly2(); + + bencher.iter(|| { + assert!(!ls_1.contains(&ls_2)); + }); + }, + ); + + c.bench_function( + "LineString not contains LineString (Relate trait)", + |bencher| { + let ls_1: geo::LineString = geo_test_fixtures::poly1(); + let ls_2: geo::LineString = geo_test_fixtures::poly2(); + + bencher.iter(|| { + assert!(!ls_1.relate(&ls_2).is_contains()); + }); + }, + ); + + c.bench_function( + "LineString contains LineString (Contains trait)", + |bencher| { + let ls_1: LineString = geo_test_fixtures::poly1(); + let mut ls_2 = LineString::new(ls_1.0[1..].to_vec()); + ls_2.0.pop(); + + bencher.iter(|| { + assert!(ls_1.contains(&ls_2)); + }); + }, + ); + + c.bench_function("LineString contains LineString (Relate trait)", |bencher| { + let ls_1: geo::LineString = geo_test_fixtures::poly1(); + let mut ls_2 = LineString::new(ls_1.0[1..].to_vec()); + ls_2.0.pop(); + + bencher.iter(|| { + assert!(ls_1.relate(&ls_2).is_contains()); + }); + }); + + c.bench_function("MultiPolygon contains MultiPoint (Contains trait)", |bencher| { + let p_1: Polygon = Polygon::new(geo_test_fixtures::poly1(), vec![]); + let p_2: Polygon = Polygon::new(geo_test_fixtures::poly2(), vec![]); + let multi_poly = MultiPolygon(vec![p_1, p_2]); + let multi_point: MultiPoint = geo::wkt!(MULTIPOINT (-60 10,-60 -70,-120 -70,-120 10,-40 80,30 80,30 10,-40 10,100 210,100 120,30 120,30 210,-185 -135,-100 -135,-100 -230,-185 -230)).convert(); + + bencher.iter(|| { + assert!(multi_poly.contains(&multi_point)); + }); + }); + + c.bench_function("MultiPolygon contains MultiPoint (Relate trait)", |bencher| { + let p_1: Polygon = Polygon::new(geo_test_fixtures::poly1(), vec![]); + let p_2: Polygon = Polygon::new(geo_test_fixtures::poly2(), vec![]); + let multi_poly = MultiPolygon(vec![p_1, p_2]); + let multi_point: MultiPoint = geo::wkt!(MULTIPOINT (-60 10,-60 -70,-120 -70,-120 10,-40 80,30 80,30 10,-40 10,100 210,100 120,30 120,30 210,-185 -135,-100 -135,-100 -230,-185 -230)).convert(); + + bencher.iter(|| { + assert!(multi_poly.relate(&multi_point).is_contains()); + }); + }); + + c.bench_function("MultiPolygon not contains MultiPoint (Contains trait)", |bencher| { + let p_1: Polygon = Polygon::new(geo_test_fixtures::poly1(), vec![]); + let p_2: Polygon = Polygon::new(geo_test_fixtures::poly2(), vec![]); + let multi_poly = MultiPolygon(vec![p_1, p_2]); + let multi_point: MultiPoint = geo::wkt!(MULTIPOINT (-160 10,-60 -70,-120 -70,-120 10,-40 80,30 80,30 10,-40 10,100 210,100 120,30 120,30 210,-185 -135,-100 -135,-100 -230,-185 -230)).convert(); + + bencher.iter(|| { + assert!(multi_poly.contains(&multi_point)); + }); + }); + + c.bench_function("MultiPolygon not contains MultiPoint (Relate trait)", |bencher| { + let p_1: Polygon = Polygon::new(geo_test_fixtures::poly1(), vec![]); + let p_2: Polygon = Polygon::new(geo_test_fixtures::poly2(), vec![]); + let multi_poly = MultiPolygon(vec![p_1, p_2]); + let multi_point: MultiPoint = geo::wkt!(MULTIPOINT (-160 10,-60 -70,-120 -70,-120 10,-40 80,30 80,30 10,-40 10,100 210,100 120,30 120,30 210,-185 -135,-100 -135,-100 -230,-185 -230)).convert(); + + bencher.iter(|| { + assert!(multi_poly.relate(&multi_point).is_contains()); + }); + }); } criterion_group!(benches, criterion_benchmark);