-
Notifications
You must be signed in to change notification settings - Fork 120
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
Always be simplifying (Natural Earth low zooms) #2015
Conversation
Connects with #2014. - Shifts simplification mostly to zoom 0 from 8 - Start simplifying transit layer - Truncate min_zoom to 1 decimal place always
sort: vectordatasource.sort.landuse | ||
area-inclusion-threshold: 1 | ||
roads: | ||
geometry_types: [LineString, MultiLineString] | ||
simplify_start: 8 | ||
simplify_start: 4 |
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.
Features only come in starting at zoom 4, so simplifying before then is not necessary
- vectordatasource.transform.remap_viewpoint_kinds | ||
- vectordatasource.transform.unpack_viewpoint_claims | ||
area-inclusion-threshold: 1 | ||
transit: | ||
geometry_types: [LineString, MultiLineString, Polygon, MultiPolygon] | ||
simplify_start: 5 |
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.
Oops, the transit layer wasn't simplified at all!? No wonder it's large.
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.
Zoom 5 because:
vector-datasource/yaml/transit.yaml
Line 26 in 2eb4486
min_zoom: 5
@@ -152,7 +152,7 @@ layers: | |||
water: | |||
geometry_types: [Point, MultiPoint, Polygon, MultiPolygon, LineString, MultiLineString] | |||
simplify_before_intersect: true | |||
simplify_start: 8 | |||
simplify_start: 0 |
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.
All the transforms already run on Natural Earth and OSM features, the simplify_start
is independent of those and evaluated on each feature on the layer.
@peitili Can you generate some after tiles for this PR, please? These 4 coords:
There's not really tests to add as we're not changing what features or properties are included. So we need to do a visual and tile size comparison. |
Water and Earth are already merged above
@@ -777,7 +779,7 @@ post_process: | |||
- fn: vectordatasource.transform.merge_line_features | |||
params: | |||
source_layer: boundaries | |||
start_zoom: 8 | |||
start_zoom: 0 |
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.
@peitili I added this config change since your review.
This looks to be doing the right thing, yielding an average of 38% file size savings (ranges from 25% to 60%). Once the boundaries fix is also applied this is likely to be even better (see below). The boundary merging is working, though the geometry precision wasn't matching the other simplified linework (more obvious in the overzoomed Bubble Wrap screenshot below). Fixed via 909d7a5. The geometry is degenerate over Natural Earth's raw features, but this isn't noticeable except in QGIS overzooming quite a bit. It does make the water and earth features inconsistent with each other, and with the NE coastline. But again not noticeable at 100% zoom or 200% zoom in web map. One solution is to disable the earth layer and draw that with map background only (applying water features over that). For OSM we generate water boundary lines, perhaps we should do that for NE now that it's simplified, too? Anon. At zoom 5 when transit comes in things get a lot bigger because of unrelated complexity in feature properties (those should really be dropping out). Should address with separate PR. Screenshots for the 2/1/2 tile at 512 px sizing: Overzoomed in Bubble Wrap: In QGIS: Overzoomed: Overzoomed, again again: |
sort: vectordatasource.sort.pois | ||
area-inclusion-threshold: 1 | ||
boundaries: | ||
geometry_types: [Polygon, MultiPolygon, LineString, MultiLineString] | ||
simplify_before_intersect: true | ||
simplify_start: 8 | ||
simplify_start: 0 |
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.
@peitili this change is since PR review, based on build QA. The feature merging was working, but the linework detail was significantly more detailed that the water or earth layers on visual inspection. See PR comment for images.
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.
Since boundaries appear in the 0/0/0 tile and zoom 1 tiles, this will have largest impact there on p50 size (not just p99).
Connects with #2014.
Todo:
min_zoom
is a float (but don't say the precision.