Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Delegate Geometry::intersects to PreparedGeometry::intersects #775

Merged
merged 1 commit into from
Dec 13, 2022

Conversation

dbaston
Copy link
Member

@dbaston dbaston commented Dec 13, 2022

This PR changes Geometry::intersects and Geometry::disjoint to use PreparedGeometry instead of RelateOp. Relative to the RelateOp implementation, the prepared intersection is more robust (e.g., #766) and is faster for both complex and simple polygons, even if the object is only used a single time.

A quick benchmark shows the performance difference in cases of relatively complex polygons (left) and triangles (right), using the GEOS perf_intersection, which combines index querying, intersects testing, and intersection overlay.

image

@dr-jts
Copy link
Contributor

dr-jts commented Dec 13, 2022 via email

@dbaston
Copy link
Member Author

dbaston commented Dec 13, 2022

I would expect an improvement, but I need to put together some suitable geometries for testing. Will do as a separate PR.

@dr-jts
Copy link
Contributor

dr-jts commented Dec 13, 2022

covers might show even more improvement than contains, since it is a semantically simpler predicate, with a simpler implementation.

@dbaston dbaston force-pushed the intersects-delegate-prepared branch from 57a1399 to 8b42258 Compare December 13, 2022 16:37
@dbaston dbaston merged commit 69678c4 into libgeos:main Dec 13, 2022
@dbaston
Copy link
Member Author

dbaston commented Dec 13, 2022

So far I'm not seeing any improvement on "contains" in the initial test case (P/P with 13% of contains tests returning true). Will continue looking into it.

image

@dr-jts
Copy link
Contributor

dr-jts commented Dec 13, 2022

You should test with Points and small (2-point) LineStrings as well, to ensure there is no performance regression in those cases (or if there is, that it's acceptable).

In JTS using PreparedPolygon against points is slightly slower than the RelateOp-based predicate. This is likely due to building an index for the Point-in-Polygon test, rather than the simple O(N) evaluation used in RelateOp.

Running with polygon size = 1000
Iterations per run = 1

runCoversNonPrep : 865 ms
runCoversPrepared : 48 ms
runCoversPrepNoCache : 663 ms

runIndexPointInAreaLocator : 2 ms

runIntersectsNonPrep : 287 ms
runIntersectsPrepared : 7 ms
runIntersectsPrepNoCache : 355 ms

@dr-jts
Copy link
Contributor

dr-jts commented Dec 13, 2022

@dbaston In JTS I'm seeing that PreparedPolygon is slower in single-use mode than RelateOp for a test with target polygons of 1000 and 2000 pts, against a set of 1000 lines with 100 pts each:

-------  Running with polygon size = 10
runIntersectsNonPrep : 1921 ms
runIntersectsPrepCached : 229 ms
runIntersectsPrepNotCached : 352 ms

-------  Running with polygon size = 100
runIntersectsNonPrep : 1814 ms
runIntersectsPrepCached : 64 ms
runIntersectsPrepNotCached : 495 ms

-------  Running with polygon size = 1000
runIntersectsNonPrep : 1253 ms
runIntersectsPrepCached : 23 ms
runIntersectsPrepNotCached : 1485 ms

-------  Running with polygon size = 2000
runIntersectsNonPrep : 1300 ms
runIntersectsPrepCached : 22 ms
runIntersectsPrepNotCached : 2577 ms

I'm pretty sure that I carried out this kind of analysis a while ago, and decided that the tradeoff was a difficult call to make. Perhaps what's required is a switch based on the size of the target geometry.

@dr-jts
Copy link
Contributor

dr-jts commented Dec 13, 2022

Also, I expect that a new implementation of RelateOp (RelateNG probably) will be faster than PreparedGeometry, since it can choose to only index linework which might potentially interact (i.e that lies inside the intersection of the input envelopes). It will be more robust as well.

That in itself probably doesn't weigh against this PR, since this change can be backed out when RelateNG arrives. But the performance regression for large geometries requires some thought.

@dbaston
Copy link
Member Author

dbaston commented Dec 13, 2022

Can you point me to those benchmarks?

Disregard - I see they're in https://github.com/locationtech/jts/tree/0911609ba8e7d7ce0cacaaccb3216bc5576b8516/modules/core/src/test/java/test/jts/perf/geom/prep

@dbaston
Copy link
Member Author

dbaston commented Dec 14, 2022

In JTS I'm seeing that PreparedPolygon is slower in single-use mode than RelateOp for a test with target polygons of 1000 and 2000 pts, against a set of 1000 lines with 100 pts each:

I ported your benchmark and I see the same trend in GEOS, although RelateOp doesn't become faster there are until around 3,000 points in the sine star.

@dbaston
Copy link
Member Author

dbaston commented Dec 14, 2022

An "incrementally prepared geometry" resolves the issue. Too hacky?

algorithm::locate::PointOnGeometryLocator*
PreparedPolygon::
getPointLocator() const
{
    if(! ptOnGeomLoc) {
        ptOnGeomLoc.reset(new algorithm::locate::SimplePointInAreaLocator(&getGeometry()));

        return ptOnGeomLoc.get();
    } else if (!indexedPtOnGeomLoc) {
        indexedPtOnGeomLoc.reset(new algorithm::locate::IndexedPointInAreaLocator(getGeometry()));
    }

    return indexedPtOnGeomLoc.get();
}

@dr-jts
Copy link
Contributor

dr-jts commented Dec 14, 2022

An "incrementally prepared geometry" resolves the issue. Too hacky?

Very clever! Looks good to me. A line or two of explanatory doco would be nice.

@dbaston
Copy link
Member Author

dbaston commented Dec 15, 2022

Followed up (with some benchmarking) in #777

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants