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

Process boundaries from RAWR tiles the same way we do from SQL. #362

Merged
merged 6 commits into from
Jan 22, 2019

Conversation

zerebubuth
Copy link
Member

We don't query boundaries at all from the planet_osm_lines table, instead we select the polygons and generate linestring boundaries from them. This is to ensure that boundaries are correctly oriented with the interior on the left.

However, we weren't doing this for RAWR tiles, and were using the lines from planet_osm_lines in their original orientation. The planet_osm_lines table contains multiple copies of the line; from the original way, plus any boundary relations it is part of. This meant that the two linestrings making up a section of border were in the same direction, and therefore the boundary would be flipped if the geometry was "used" by the wrong side first (which depends on the order in the file, which is non-deterministic).

The fix for this is a bit of an egregious hack; it just drops any boundaries linestrings and turns polygons into their boundaries (clipped to the tile). This matches the SQL in vector-datasource, but is vector-datasource-specific processing which doesn't seem to belong in tilequeue, but I couldn't figure a less hacky way to do it without major refactoring.

Connects to tilezen/vector-datasource#1770.

Copy link
Member

@iandees iandees left a comment

Choose a reason for hiding this comment

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

Looks good. One question about terminology but otherwise seems fine to me.

tilequeue/query/rawr.py Outdated Show resolved Hide resolved
…he use of the word 'boundary' both for the features in the 'boundaries' layer, and the line separating the interior of the polygon from the exterior.
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

between a line and a polygon. The main idea is to remove points, and any
other geometry which might throw a wrench in the works.
"""
from shapely.geometry import MultiLineString
Copy link
Member

Choose a reason for hiding this comment

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

Move import to top?

Copy link
Member Author

Choose a reason for hiding this comment

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

Yup, good idea. Fixed in 92ba304.

# nasty hack: in the SQL, we don't query the lines table for
# boundaries layer features - instead we take the rings (both outer
# and inner) of the polygon features in the polygons table - which are
# also called the "boundary" of the polygon. the back below replicates
Copy link
Member

Choose a reason for hiding this comment

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

the hack below?

Copy link
Member Author

Choose a reason for hiding this comment

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

Well spotted, thanks! Fixed in f97dd53.

@zerebubuth zerebubuth merged commit 146dfbe into master Jan 22, 2019
@zerebubuth zerebubuth deleted the zerebubuth/fix-rawr-boundaries branch January 22, 2019 09:55
zerebubuth added a commit that referenced this pull request Feb 1, 2019
We include buffered land into the boundaries table, but we want to keep it as a polygon, not turn it into a set of outer/inner ring linestrings.
zerebubuth added a commit that referenced this pull request Feb 1, 2019
Exclude buffered land polygons from changes made in #362.
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