Skip to content

feat: curve A/P PIP for collections (rebase of #1322) - #1502

Closed
grootstebozewolf wants to merge 4 commits into
libgeos:mainfrom
grootstebozewolf:feat/curve-pip-collections-rebase
Closed

feat: curve A/P PIP for collections (rebase of #1322)#1502
grootstebozewolf wants to merge 4 commits into
libgeos:mainfrom
grootstebozewolf:feat/curve-pip-collections-rebase

Conversation

@grootstebozewolf

Copy link
Copy Markdown

Summary

Rebase of @dbaston’s #1322 onto current main (that PR is CONFLICTING). Same feature, merge conflicts resolved, plus a small empty-point-set hardening.

Behaviour (from #1322)

  • SimplePointInAreaLocator::isAnyPointContained / isEveryPointContained for multi-point vs multi-surface
  • Curved A/P paths for contains / intersects / disjoint (incl. MultiSurface)
  • Tests: MultiPoint×MultiSurface, PostGIS #5832 intersects pin
  • Keeps existing automatic-linearization CAPI tests (merged alongside)

Extra on this rebase

  • Empty point sets: isAny → false, isEvery → true (vacuous); null-safe getCoordinate
  • CAPI tests for empty MULTIPOINT / POINT EMPTY on contains / intersects / disjoint

Supersedes / unblocks #1322. Happy for @dbaston to take over authorship if preferred.

Test plan

  • Conflict markers resolved; branch builds on GitHub CI
  • CI green on this PR
  • Manual: MultiSurface contains interior MultiPoint; exterior point fails contains; empty MultiPoint vacuous contains / disjoint

Co-authored-by: Dan Baston (original #1322)
Assisted-by: xAI Grok (rebase + empty multipoint guards)

@grootstebozewolf

Copy link
Copy Markdown
Author

Next rung — local verification + Contains semantics fix

CI status: workflows show action_required (first-time contributor; needs a maintainer to approve the run).

Local self-check (WSL, Release, test_geos_unit):

Suite Result
capi::GEOSContains 14/14
capi::GEOSIntersects 17/17
capi::GEOSDisjoint 5/5

Extra commit on this PR: DE-9IM Contains for curved A/P was using “in or on” (!= EXTERIOR), i.e. covers-like. Fixed via isAreaContainingPoints:

  • empty point set → false (needs II=T)
  • boundary-only → false
  • boundary + interior multipoint → true
  • boundary still intersects

@dbaston (or any maintainer): please approve CI when you have a moment.


Assisted-by: xAI Grok

dbaston and others added 4 commits August 10, 2026 08:13
Rebase of dbaston#1322 onto current main:

- SimplePointInAreaLocator: isAnyPointContained / isEveryPointContained
- Geometry: curved contains / intersects / disjoint for A/P (incl. MultiSurface)
- CAPI tests: MultiPoint×MultiSurface, PostGIS #5832
- Keep existing automatic-linearization tests (merge conflicts resolved)

Additions on the rebase:

- Empty point sets: isAny → false, isEvery → true (vacuous); null-safe getCoordinate
- Empty MULTIPOINT / POINT EMPTY CAPI tests for contains / intersects / disjoint

Co-authored-by: Dan Baston <dbaston@gmail.com>
Assisted-by: xAI Grok
isEveryPointContained treated boundary as inside (covers-like). Contains
needs II=T: empty point sets false; boundary-only multipoints false;
boundary+interior true.

Local: capi::GEOSContains 14/14, GEOSIntersects 17/17, GEOSDisjoint 5/5.

Assisted-by: xAI Grok
@grootstebozewolf
grootstebozewolf force-pushed the feat/curve-pip-collections-rebase branch from 886eaf0 to 4f7ec39 Compare August 10, 2026 06:28
@grootstebozewolf

Copy link
Copy Markdown
Author

Rebased onto post-#1500 main

#1500 (GeometrySplitter / GeometryNoder for #1497) is merged — rebased this branch cleanly onto current main (14fd2d7).

Force-pushed feat/curve-pip-collections-rebase (same 4 commits, new hashes). Still mergeable; still needs maintainer CI approval (first-time contributor).

Local (WSL Release) after rebase: capi::GEOSContains 14/14, GEOSIntersects 17/17, GEOSDisjoint 5/5; MultiSurface×MultiPoint A/P pins green via geosop.

Next GEOS rung (oracle-driven): #1505 for covers L/P float robustness (#968).

Assisted-by: xAI Grok

@dbaston

dbaston commented Aug 12, 2026

Copy link
Copy Markdown
Member

Please try to limit the verbosity of these pull requests.

I'm going to close this -- I don't think these special-case implementations are worth it when a curve-aware predicate engine should be in the future.

@dbaston dbaston closed this Aug 12, 2026
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