Skip to content

Exclude more features from places layer at low zooms#1734

Merged
nvkelso merged 12 commits intomasterfrom
nvkelso/1729-less-places-labels-fix
Dec 14, 2018
Merged

Exclude more features from places layer at low zooms#1734
nvkelso merged 12 commits intomasterfrom
nvkelso/1729-less-places-labels-fix

Conversation

@nvkelso
Copy link
Copy Markdown
Member

@nvkelso nvkelso commented Dec 14, 2018

Connects to #1729 to limit places layer features to only >= +0.5 of the nominal zoom instead of >= +1 to reduce amount of features (and reduce file size, translations multiply!), especially at low zooms.

This better matches how Natural Earth is curated starting in version 4.1 for min_zoom.

For example, this will remove the 1.7 features above from the zoom 1 tile, but they'll show up in zoom 2 tile and collide out min_zoom 2.0 features – good).

@nvkelso nvkelso changed the title limit filter to +0.5 instead of full zoom up; limit overstuffing Exclude more features from places layer at low zooms Dec 14, 2018
@nvkelso nvkelso requested a review from zerebubuth December 14, 2018 08:32
Comment thread vectordatasource/transform.py Outdated
_, props, _ = feature
min_zoom = props.get('min_zoom')
if min_zoom is not None and min_zoom <= nominal_zoom + 1:
if min_zoom is not None and min_zoom <= nominal_zoom + 0.5:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rather than change the zoom range we include in the tile (i.e: N to N+1), I think it would be better to change the min zoom we're applying from NE. For example, changing the setting code to assign props['min_zoom'] = min_zoom + 0.5 and similar for max_zoom. The two methods should be mathematically identical.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Hmm, I don't like changing the min_zoom values to be inconsistent with Natural Earth – mostly because its had for me to wrap my head around as the author of Natural Earth! – but also because it's it's just values in the x.6 to x.9 range that should be ceiling'd up to x+1.0 to be more consistent?

Either way the places layer should be < nominal_zoom +1 not <= to avoid the Poland example?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Attempting fix in 9914c27.

@nvkelso
Copy link
Copy Markdown
Member Author

nvkelso commented Dec 14, 2018

@zerebubuth Ready for another look.

FYI: I did implement modified version of min_zoom changes, but ignored updating the max_zoom suggestion as I think it has less impact / that wasn't necessary in the QGIS doc / less important for the size now.

_, props, _ = feature
min_zoom = props.get('min_zoom')
if min_zoom is not None and min_zoom <= nominal_zoom + 0.5:
if min_zoom is not None and min_zoom < nominal_zoom + 1:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

👍 thanks!

Comment thread vectordatasource/transform.py Outdated
# don't overstuff features into tiles when they are in the
# long tail of won't display, but make their min_zoom
# consistent with when they actually show in tiles
if ceil(min_zoom) == trunc(min_zoom) + 1:
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I think this is the same as "if min_zoom is an integer", in which case the if/else isn't necessary and we can just write props['min_zoom'] = ceil(min_zoom) for everything (because ceil(x) = x for integers).

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

What I'm trying to do:

  • 1.0 stays 1.0
  • 1.1 stays 1.1
  • 1.2 stays 1.2
  • 1.3 stays 1.3
  • 1.4 stays 1.4
  • 1.5 stays 1.5 (probably fails now)
  • 1.6 goes to 2.0
  • 1.7 goes to 2.0
  • 1.8 goes to 2.0
  • 1.9 goes to 2.0

Can you recommend a quick fix for that, please?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Something like: props['min_zoom'] = ceil(min_zoom) if min_zoom % 1 > 0.5 else min_zoom

Which basically says "if the fractional part is more than half round it up, otherwise keep it the same". If you prefer, it might be more readable as:

if min_zoom % 1 > 0.5:
  min_zoom = ceil(min_zoom)
props['min_zoom'] = min_zoom

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Fix in ec05844.

Copy link
Copy Markdown
Member

@zerebubuth zerebubuth 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 now, thanks! (Although flake8 disagrees 😠 )

@nvkelso
Copy link
Copy Markdown
Member Author

nvkelso commented Dec 14, 2018

🎉 Huzzah! Merging.

@nvkelso nvkelso merged commit db14e4f into master Dec 14, 2018
@nvkelso nvkelso deleted the nvkelso/1729-less-places-labels-fix branch December 14, 2018 18:58
Comment thread yaml/water.yaml
# - {kind: ocean}
# table: shp
- filter: {meta.source: shp}
min_zoom: 0
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ooops, I think this might have been the polygon, not the ocean label....

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

oh, shoot!

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

ah: meta.source: shp is for the openstreetmapdata.com source.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Looks like they are all nodes:

image

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