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

Improve road merging #1703

Merged
merged 5 commits into from
Nov 14, 2018
Merged

Conversation

zerebubuth
Copy link
Member

Connects to #1227.

There were two main issues with the road network:

  1. The merging we were already doing would generally not merge across a 4-way junction. The new code tries to merge continuous lines having a similar bearing at the junction together. This results in fewer points.
  2. Most geometry ops (including union and intersection/clipping) seem to make MultiLineStrings "simple" - which means that sibling lines within the MultiLineString do not cross and only meet each other at their endpoints. Therefore, clipping the road network to the tile boundary undoes all the work in the junction merging step. Instead, the merging step will create several MultiLineStrings, each one containing only the subset of LineStrings which don't intersect any sibling. This does mean we end up duplicating feature properties, but overall the saving is still worthwhile.

For 12/1206/1540, a tile containing many kind: minor_roads over Brooklyn, these changes resulted in a 11.7% reduction in the GeoJSON layer size, 6.7% reduction in the MVT layer size and 28.9% reduction in the number of geometry commands (i.e: points) associates with minor_road features.

There's still plenty of scope for improving tile size, as there are many properties that are probably superfluous and could be removed at this zoom level. The image below shows the tile with a dot at the first and last vertex for each line, coloured uniquely by properties:

image

This merges lines across junctions (i.e: multiple lines meeting at a point) where the lines continue on a similar bearing. This means that lines can be more aggressively simplified, and result in fewer shape points and smaller file sizes.

Also, drop small line segments part of larger MultiLineStrings, which is similar to how we drop small inners of MultiPolygons. If a single line segment is much smaller than a pixel, it's not really adding anything to the tile, but might still need 2 points to describe it.

Since many geometry operations will make a MultiLineString "simple", and this re-introduces intersections, we also separate features so that MultiLineStrings do not contain individual LineStrings which intersect.
…g box intersection to speed up intersection test. Sort items being added to buckets to try to prevent bbox over-expansion. Remove debugging code.
… properties. However now they must intersect if they do. Also, merge both before _and_ after simplifying, as simplification may make some things that previously intersected now disjoint.
@zerebubuth zerebubuth requested a review from rmarianski November 9, 2018 23:23
# setting the following will cause lines, or parts of multi-lines,
# shorter than 0.1px at nominal zoom to be dropped.
drop_short_segments: true
drop_length_pixels: 0.1
Copy link
Member

Choose a reason for hiding this comment

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

Thanks for making this configurable. Did you try a larger value like 0.2 or 0.5?

Copy link
Member Author

Choose a reason for hiding this comment

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

The issue that I saw in a few places was that of very small segments, often less than a (Mercator) metre. These seem to have been created by tiny gaps between adjacent landuse areas (which give the roads a landuse_kind, meaning they're merged separately).

Getting rid of these almost-points probably doesn't affect the appearance of the tile in this case, but I'm worried that it might in other places. I think if we find we have more substantial fragments of road, we might want to try either dropping the attributes which are preventing them from being merged, or altering the algorithms (e.g: intercut) so as not to create them in the first place.

Otherwise, we risk creating little gaps in the road network, and in some places where the attributes change in a short distance (e.g: bridge, bus routes, cycleways, surface, etc...) these little gaps might add up to something visible.

Copy link
Member

Choose a reason for hiding this comment

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

Sounds reasonable. Merge ahead!

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

@bcamper
Copy link
Collaborator

bcamper commented Nov 12, 2018

OMG return of the line merging!!! 👏👏👏 Let me know when there is a place I can try this out and I will do a comparison to see how it affects Tangram geometry count and memory usage.

@zerebubuth
Copy link
Member Author

I think we'll see an even bigger improvement after #1331. There's a lot of missed opportunities for merging in the image above, even on minor roads which we've stripped the names from already.

@zerebubuth zerebubuth merged commit d4f15bf into master Nov 14, 2018
@zerebubuth zerebubuth deleted the zerebubuth/1227-improve-road-merging branch November 14, 2018 11:00
zerebubuth added a commit that referenced this pull request Nov 20, 2018
The previous PR #1703, only made one pass over the road network. This meant that there were still many opportunities for further merging which were left untaken. Instead, we can loop over the merge step until all possible merges have been taken. Since each pass considers all mergeable points, but can only merge two lines in any given step, it should be possible to merge all points in `O(log N)` steps.

This patch also re-orders the steps for merging, simplification and grouping. The merging and grouping operations used to happen together, and we had two of them sandwiching a simplification step. However, this caused problems when the simplification step re-introduced junctions that had just been merged. This was happening because `simplify` was being called on the MultiLineString, which tries to honour the locations of the intersections of its component lines. Instead, the `merge_line_features` post-processor can internally simplify each individual LineString between merging and grouping those LineStrings into non-overlapping MultiLineStrings.
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.

4 participants