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

Fix definition of overlaps for boundaries #354

Merged
merged 3 commits into from
Nov 2, 2018

Conversation

zerebubuth
Copy link
Member

For boundary geometries, we want to include the row in the result of a query when some part of its boundary is inside the bounding box. Originally, we used st_intersects for this, but it had the problem that it would include rows which had very large (and complex) boundaries for tiles which were completely in the interior of the polygon, which led to unnecessary data being returned and processed.

In tilezen/vector-datasource#1116, a new bbox_overlaps test was added, using st_overlaps. This no longer included polygons for which the tile bounding box was entirely with the interior. However, it also didn't include polygons which were themselves entirely within the interior of the bounding box, which meant some small countries (e.g: Liechtenstein) went missing at some zoom levels!

Relationship Description Intersects? Overlaps? Box contains poly? Result we want
rel-disjoint Box & poly are disjoint False False False False
rel-overlaps Box & poly overlap, but not entirely True True False True
rel-contains-bbox Poly entirely contains box True False False False
rel-contains-poly Box entirely contains poly True False True True

This patch changes the definition of the bbox_overlaps test to also include polygons contained by the tile bounding box.

For boundary geometries, we want to include the row in the result of a query when some part of its boundary is inside the bounding box. Originally, we used `st_intersects` for this, but it had the problem that it would include rows which had very large (and complex) boundaries for tiles which were completely in the interior of the polygon, which led to unnecessary data being returned and processed.

In tilezen/vector-datasource#1116, a new `bbox_overlaps` test was added, using `st_overlaps`. This no longer included polygons for which the tile bounding box was entirely with the interior. However, it also didn't include polygons which were themselves entirely within the interior of the bounding box, which meant some small countries (e.g: Liechtenstein) went missing at some zoom levels!

This patch changes the definition of the `bbox_overlaps` test to also include polygons contained by the tile bounding box.
@zerebubuth zerebubuth merged commit 8f2805c into master Nov 2, 2018
@zerebubuth zerebubuth deleted the zerebubuth/bbox-contains-overlaps branch November 2, 2018 15:25
Copy link
Member

@rmarianski rmarianski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

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.

3 participants