-
-
Notifications
You must be signed in to change notification settings - Fork 47
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
Pre-collide places layer point features and reduce density / overlaps #300
base: main
Are you sure you want to change the base?
Conversation
…duce density / overlap
@@ -55,15 +57,30 @@ static int getSortKey(double minZoom, int kindRank, int populationRank, long pop | |||
|
|||
private static final ZoomFunction<Number> LOCALITY_GRID_SIZE_ZOOM_FUNCTION = | |||
ZoomFunction.fromMaxZoomThresholds(Map.of( | |||
6, 32, | |||
7, 64 | |||
3, 24, |
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 GRID_SIZE sequence could be simplified, but needs to match the zoom steps in the GRID_LIMIT sequence below.
10, 1, | ||
11, 1, | ||
14, 2, | ||
15, 3 |
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.
You could argue the zoom 15 step could be a larger int, or not limited at all
@@ -287,12 +304,14 @@ public void processOsm(SourceFeature sf, FeatureCollector features) { | |||
//feat.setSortKey(minZoom * 1000 + 400 - populationRank * 200 + placeNumber.incrementAndGet()); | |||
feat.setSortKey(getSortKey(minZoom, kindRank, populationRank, population, sf.getString("name"))); | |||
|
|||
// This is only necessary when prepping for raster renderers | |||
feat.setBufferPixels(16); |
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.
If Protomaps is only targeting vector renderers than this could be set to 0 to reduce file size
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.
I think we still want a buffer for drawing townspots correctly very close to tile edges?
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.
If 256 isn't evenly dvisible by your label grid size then you need a bit extra in order for label grid squares to be aware of points in the square from neighboring tiles otherwise the density might get off a bit around tile boundaries. Looks like 24 is the only problematic one. I think a buffer of 16 should be the minimum that works with label grid of 24.
If you want to go smaller you could either switch label grid to a power of 2 or use FeatureMerge.removePointsOutsideBuffer
in postProcess
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.
I think the min buffer pixel limit should be label grid size - gcd(256, label grid size)
- I should probably update planetiler to use that limit and post-filter to whatever you set here so users don't need to now or care about that implementation detail...
@nvkelso here is a Maperture example with the default style (light). It seems like the effect on the style is negligible, but tiles should be much lighter now. Other styles (like Mamata's) or raw unfiltered display in QGIS will have less places because their style filters based around minzoom are different. Is that the intended effect? Otherwise LGTM |
@nvkelso I may have mis-evaluated the above, how are you viewing zoom levels in QGIS? suspect that may not be consistent with either Leaflet/Tangram or MapLibre GL |
I'm using https://github.com/kgjenkins/qgis-zoom-level and I believe it displays Leaflet zoom levels (256x256 display pixels) and not maplibre / tile data ones (512x512 display pixels) |
Same, 256px display pixels so Leaflet zooms. |
I need to do more testing to understand why the preview you linked to mostly looks the same... |
@nvkelso yes, I confirmed that it looks thinned out in QGIS. let me double check the tileset from that Maperture link was the right branch... |
@nvkelso the Maperture link has been updated - should be able to "view boxes" and visualize a significant improvement in hidden labels. |
@bdon New Mapeture link where I've shrunk the texts and icon padding. I tried floating the text around the townspot but that adds too many text labels for an apples to apples comparison, and shows poorly in dense areas with long names like around New York city. Adding sort-key solved the flickering in New York and Barcelona etc. But to also solve in Delhi where the min_zoom is correct but the population_rank is wrong, you'd want to modify this to use the Something like (see src though):
A few before and afters: |
There are some test failures related to the issue @msbarry pointed out, will merge this in for 4.1... |
@msbarry is there an easy way to limit the grid thinning to certain zooms? I'd prefer it not start until say zoom 7. |
Yeah
You mean you want at zoom <= 7 right? There's a |
@msbarry No, zoom > 7 as the early zooms are highly curated and it's OK for say Oakland and San Francisco to appear next to each other in the same collision grid cell. |
ah OK so if you want to apply to zooms >= 7 you can do something like |
Fixes #287 and fixes #116 and related to label flickering in #286 and #256.
Adjusts the collision grid size and sort functions.
Testing in on the
california
make target, we see dramatic reduction in features from zooms 5 to 13, with the most reduction in zooms 8, 9, and 10:The archive size and feature count remained constant.