-
Notifications
You must be signed in to change notification settings - Fork 121
earlier bike roads (and walking routes) #1198
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
Conversation
This reverts commit 826a239.
I've fixed the migrations in e6817df based on conversation with @rmarianski – it's just the version'd For tests, note that @zerebubuth's changes in vector-datasource/pull/1193 for when the walking route and bike route network property is dropped impacts which props I can test for in his https://github.com/tilezen/vector-datasource/pull/1193/files#diff-fc49e5c256610bc0b32a7de5a0520835. |
@zerebubuth This is finally ready for review :) |
data/migrations/v1.3.0-line.sql
Outdated
tags -> 'whitewater' = 'portage_way') | ||
AND mz_calculate_min_zoom_roads(planet_osm_line.*) IS NOT NULL; | ||
|
||
CREATE INDEX new_planet_osm_line_roads_geom_index ON planet_osm_line USING gist(way) WHERE mz_road_level IS NOT NULL; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It shouldn't be necessary to re-create the indexes, they should be updated along with the rows in the UPDATE
statement above.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 392c88e.
queries.yaml
Outdated
properties: | ||
- walking_network | ||
- walking_shield_text |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This means that walking networks will be kept on the roads until zoom 7, but the earliest min_zoom
for a walking route is zoom 9. This could block opportunities for merging roads at low zooms. Would we display walking data at z7-8? If not, we should remove this data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point, I'll remove the data properties at those zooms.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Fixed in 7cd5ffc.
I also removed the path name at early zooms to enable more line merging – that was necessary for v1.0 when we didn't have the walking network information but is now getting in the way.
Tests pass (again again), comments address. Merging :) |
Connects to #1172 to show bike and walking related ways a lot earlier, and #1214 for dropping early all_* network and shield texts.
min_zoom
change)