Skip to content

Water layer too big #1714

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

Merged
merged 8 commits into from
Nov 29, 2018
Merged

Water layer too big #1714

merged 8 commits into from
Nov 29, 2018

Conversation

zerebubuth
Copy link
Member

Incorporating the area look up table from #1477 (comment) and a "curve fitted" pixel-area name rule, we can reduce the number of water features (which weren't getting rendered anyway) in the tile. Additionally, we were missing simplification for water and earth layers at zoom 8 (nominal). With those changes, a zoom 8 (nominal) 512px tile at 7/30/43 looks like this (previous values in red, behind, layer in front in blue is new):

image

On tile 7/30/43, which is pretty much a worst-case scenario and has thousands of tiny lakes, we get a 97% reduction in total size (from 2.4MB to 62kB). The reduction in the number of features is massive - partly because we're being more selective about the features at this zoom and partly because we're merging a lot more lakes that we're able to strip the names off:

Kind Before After Reduction
lake 1 0 100%
riverbank 115 2 98%
water 2858 40 98%

The bytes per kind shows similar reduction:

Kind Before After Reduction
lake 141 0 100%
riverbank 90992 1888 98%
water 2396628 60309 97%

@zerebubuth zerebubuth requested a review from nvkelso November 28, 2018 20:07
@zerebubuth
Copy link
Member Author

Connects to #1477.

yaml/water.yaml Outdated
@@ -139,39 +160,39 @@ filters:
kind: ocean
table: ne
- filter: {featurecla: Alkaline Lake}
min_zoom: 0
min_zoom: *water_polygon_min_zoom
Copy link
Member

@nvkelso nvkelso Nov 28, 2018

Choose a reason for hiding this comment

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

For Natural Earth features we should just trust their min_zoom data property.

Looks like before we were always "overstuffing" by using 0, but the data property already does curated filtering for us.

Here and elsewhere...

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 point. Does this YAML pre-date NE v4? I think we might have been relying on the filter which removes features smaller than a pixel a little too much. Fixed in 67aa978.

- [ 4, 5000000000 ]
- [ 5, 700000000 ]
- [ 6, 500000000 ]
- [ 7, 160000000 ]
Copy link
Member

Choose a reason for hiding this comment

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

Suggest removing lines for zooms 0-7 as those are from Natural Earth and should be taken from the data, including the junk comment copy-pasted from Walkabout for # limit show smaller landuse areas to higher zooms.

Copy link
Member

Choose a reason for hiding this comment

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

(Should add comment to that effect... &/or keep filters as backstop against future OSM / NE zoom transition changes?)

Copy link
Member Author

Choose a reason for hiding this comment

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

Sounds good. Clipped to 7 and quickly tested locally. Looks okay to me. Fixed in 67aa978.

yaml/water.yaml Outdated
- [ 11, 200000 ]
- [ 12, 50000 ]
- [ 13, 20000 ]
default: 14
Copy link
Member

Choose a reason for hiding this comment

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

This seems to now show tiny lakes and swimming pools at zoom 14 instead of zoom 16 max? Although in practice browsing around Cupertino (link below) most swimming pools were getting zoom 14.xxx incorrectly (and even showing before the surrounding house footprints!)

Suggest adding zoom 14, 15, 16, and default 17 instead:

        - [ 14,      2000 ]
        - [ 15,      1000 ]
        - [ 16,      400 ]
      default: 17

I've been browsing around Cupertino to gain some of these numbers:

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 91c05f0.

@@ -161,21 +161,21 @@ layers:
- vectordatasource.transform.add_id_to_properties
- vectordatasource.transform.detect_osm_relation
- vectordatasource.transform.remove_feature_id
- vectordatasource.transform.truncate_min_zoom_to_2dp
- vectordatasource.transform.truncate_min_zoom_to_1dp
Copy link
Member

Choose a reason for hiding this comment

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

All the min_zooms are ints in the area transform below... although switching the NE values to be data driven may sometimes introduce some float values with tenths. So this doesn't hurt, but it probably doesn't help as much now that we're switching to the area lookup table.

Copy link
Member Author

Choose a reason for hiding this comment

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

Some things are still using the previous, floating point min_zoom calculation:

  • dock
  • fountain
  • bay
  • strait
  • fjord
  • basin
  • reef

Some of these (bay, strait, fjord) are (I think?) only used to generate label points, but they'll still inherit the min_zoom from their parent.

Overall, given the limitations of fractional min_zoom anyway, it seemed like it was worth cutting down the level of precision. It's a simple change if we want to increase it again later.

# NOTE: the 270 factor comes from curve fitting the area table from the
# walkabout style, with 270 being the min, ranging up to 1070.
where: >-
area < 270.0 * pixel_area
Copy link
Member

Choose a reason for hiding this comment

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

💯

Copy link
Member

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

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

Wow, HUGE savings!

A couple comments but overall looking very good :)

Copy link
Member

@nvkelso nvkelso left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@zerebubuth zerebubuth merged commit 09f9714 into master Nov 29, 2018
@zerebubuth zerebubuth deleted the zerebubuth/1477-water-layer-too-big branch November 29, 2018 16:47
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.

2 participants