Skip to content

Commit

Permalink
Fix "panic" in BoolOps by introducing i_overlay
Browse files Browse the repository at this point in the history
We've had several reports of crashes in our BooleanOps implementation
over the years. Some of them have been addressed, but there remains a
long tail of crashes.

FIXES #913, #948, #976, #1053, #1064, #1103, #1104, #1127, #1174, #1189, 1193

The root of the issue (as I understand it) is that floating point
rounding errors break the invariants of our sweep line algorithm. After
a couple years, it seems like a "simple" resolution is not in sight.

In the meanwhile, the i_overlay crate appeared. It uses a strategy that
maps floating point geometries to a scaled fixed point grid, which
nicely avoids the kind of problems we were having.

Related changes included:

Included are some tests that cover all reports in the issue tracker of the existing BoolOps panic'ing

JTS supports Bops between all geometry types. We support a more limited
set:

1. Two 2-Dimensional geometries `boolean_op`.
2. A 1-Dimenstinoal geometry with a 2-D geometry, which we call `clip`.

So this maps JTS's Line x Poly intersection tests to our Clip method.

- rm unused sweep code now that old boolops is gone

  I marked a couple fields as `allow(unused)` because they are used for
  printing debug repr in tests.

Speed up benches by only benching local boolop impl by default
  • Loading branch information
michaelkirk committed Oct 28, 2024
1 parent aa6964d commit 4915dfc
Show file tree
Hide file tree
Showing 18 changed files with 883 additions and 1,336 deletions.
4 changes: 4 additions & 0 deletions geo-bool-ops-benches/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,10 @@ edition = "2021"

# See more keys and their definitions at https://doc.rust-lang.org/cargo/reference/manifest.html

[features]
# If you want to bench against the `geo-booleanop` crate as a comparison, enable this feature.
bench-foreign-booleanop = []

[dependencies]
geo = { path = "../geo" }
geo-types = { path = "../geo-types" }
Expand Down
56 changes: 29 additions & 27 deletions geo-bool-ops-benches/benches/boolean_ops.rs
Original file line number Diff line number Diff line change
@@ -1,12 +1,8 @@
use std::f64::consts::PI;

use criterion::{measurement::Measurement, *};
use geo::{
algorithm::{BooleanOps, Rotate},
Relate,
};
use geo::algorithm::{BooleanOps, Rotate};

use geo_booleanop::boolean::BooleanOp as OtherBooleanOp;
use rand::{thread_rng, Rng};
use rand_distr::Standard;

Expand Down Expand Up @@ -60,33 +56,39 @@ fn run_complex<T: Measurement>(c: &mut Criterion<T>) {
);
});

group.bench_with_input(
BenchmarkId::new("rgbops::intersection", steps),
&(),
|b, _| {
#[cfg(feature = "bench-foreign-booleanop")]
{
use geo::algorithm::Relate;
use geo_booleanop::boolean::BooleanOp as OtherBooleanOp;

group.bench_with_input(
BenchmarkId::new("rgbops::intersection", steps),
&(),
|b, _| {
b.iter_batched(
polys.sampler(),
|(_, _, poly, poly2)| OtherBooleanOp::intersection(poly, poly2),
BatchSize::SmallInput,
);
},
);

group.bench_with_input(BenchmarkId::new("rgbops::union", steps), &(), |b, _| {
b.iter_batched(
polys.sampler(),
|(_, _, poly, poly2)| OtherBooleanOp::intersection(poly, poly2),
|(_, _, poly, poly2)| OtherBooleanOp::union(poly, poly2),
BatchSize::SmallInput,
);
},
);
});

group.bench_with_input(BenchmarkId::new("rgbops::union", steps), &(), |b, _| {
b.iter_batched(
polys.sampler(),
|(_, _, poly, poly2)| OtherBooleanOp::union(poly, poly2),
BatchSize::SmallInput,
);
});

group.bench_with_input(BenchmarkId::new("geo::relate", steps), &(), |b, _| {
b.iter_batched(
polys.sampler(),
|(poly, poly2, _, _)| poly.relate(poly2).is_intersects(),
BatchSize::SmallInput,
);
});
group.bench_with_input(BenchmarkId::new("geo::relate", steps), &(), |b, _| {
b.iter_batched(
polys.sampler(),
|(poly, poly2, _, _)| poly.relate(poly2).is_intersects(),
BatchSize::SmallInput,
);
});
}
});
}

Expand Down
2 changes: 2 additions & 0 deletions geo/CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -127,6 +127,8 @@
* <https://github.com/georust/geo/pull/1148>
* Point in `Triangle` and `Rect` performance improvemnets
* <https://github.com/georust/geo/pull/1057>
* Fix crashes in `BooleanOps`
* <https://github.com/georust/geo/pull/1234>

## 0.27.0

Expand Down
1 change: 1 addition & 0 deletions geo/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -30,6 +30,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 = "1.7.2"

[dev-dependencies]
approx = ">= 0.4.0, < 0.6.0"
Expand Down
Loading

0 comments on commit 4915dfc

Please sign in to comment.