feat: added maxzoomForRendering and minzoomForRendering
#272
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This will allow rendering with `—maxzoom=6` while having zoom 6 rendered the same way as if `—maxzoom=14` `maxzoomForRendering` will be used for simplifly algorithms
# Conflicts: # planetiler-core/src/main/java/com/onthegomap/planetiler/FeatureCollector.java
https://github.com/onthegomap/planetiler/actions/runs/2528221604 ℹ️ Base Logs c9682d5ℹ️ This Branch Logs f20b99b |
|
Interesting that no tests break. Maybe you want to add some for the new behavior? Also, maybe this change could be somewhere reflected in the documentation? |
msbarry
reviewed
Jun 10, 2022
Comment on lines
104
to
105
| arguments.getInteger("minzoomForRendering", "minimum zoom level", MIN_MINZOOM), | ||
| arguments.getInteger("maxzoomForRendering", "maximum rendering zoom level (limit 14)", MAX_MAXZOOM), |
There was a problem hiding this comment.
For naming, I think maxzoom_for_render or render_maxzoom (let's stick with lower_camel_case) makes sense, but maybe add to the description that minzoom/maxzoom are "minimum/maximum tile zoom level to generate" and something in the render max zoom that it's the zoom level that contains fine details for overzooming?
On a side note I almost wonder if it is more a property of the profile? Maybe we can revisit as we get deeper into config-driven schemas.
| @@ -99,6 +101,8 @@ public static PlanetilerConfig from(Arguments arguments) { | |||
| arguments.getDuration("loginterval", "time between logs", "10s"), | |||
| arguments.getInteger("minzoom", "minimum zoom level", MIN_MINZOOM), | |||
| arguments.getInteger("maxzoom", "maximum zoom level (limit 14)", MAX_MAXZOOM), | |||
| arguments.getInteger("minzoomForRendering", "minimum zoom level", MIN_MINZOOM), | |||
There was a problem hiding this comment.
I don't think min zoom for rendering is really necessary - I see it's only used in CacheByZoom which just uses the thresholds to set a size for the value array ahead of time - so just using min zoom would be fine.
| @@ -29,7 +29,7 @@ private CacheByZoom(int minzoom, int maxzoom, IntFunction<T> supplier) { | |||
| * @return a cache for {@code supplier} by zom | |||
| */ | |||
| public static <T> CacheByZoom<T> create(PlanetilerConfig config, IntFunction<T> supplier) { | |||
| return new CacheByZoom<>(config.minzoom(), config.maxzoom(), supplier); | |||
| return new CacheByZoom<>(config.minzoomForRendering(), config.maxzoomForRendering(), supplier); | |||
There was a problem hiding this comment.
these zoom levels don't matter too much - they just define the size of the object array that stores cached values, you can just leave with minzoom/maxzoom so that minzoomForRendering isn't necessary.
There was a problem hiding this comment.
@msbarry actually it fails without this. I end up with an inifinite amount of error about out of bounds (sorry dont have the error here).
Good point, 1 new test in PlanetilerTest might make sense similar to the compact DB tests - basically run planetiler with That kind of smoke test also ensures we don't throw an exception when someone uses this option. |
|
@msbarry i am looking at writing the test. I get the idea of what you want to do but to be honest i am a bit lost on the features to add. what we need for the test is 2 have one feature only visible at zoom 8 and one visible at zoom 14.
The tricky part is that i cant do it with |
|
No problem, I'll take a look at adding a test for this. |
|
Kudos, SonarCloud Quality Gate passed! |
|
Looks good, thanks for adding! |
|
I think this logic is correct: We don't want --maxzoom=8 to cause small features to show up in the z8 unless you explicitly set --render-maxzoom=8 |
|
@msbarry I think you got this wrong (or this PR is not doing what i think it is). The point of |
|
I was interpreting The test verifies that the contents of z8 tiles is the same with maxzoom=8 or maxzoom=14, but only changes if you set render_maxzoom=8 (which clients would very rarely do). |
|
@msbarry damin Mike i am sorry you are 100% right, it is me who got the test mixed up in my head :( i am so so so sorry. Indeed in my case i use render_maxzoom=14 to get what i described before. But your test uses 8. |
|
Great feature. I have a few questions, because I'm not really sure if understand this correctly.
|
Hello! Overzooming only happens in the client. Planetiler assists the client by including more detail at the max zoom level for rendering that it generates. This PR made it so that setting max zoom lower just omits zoom levels above that, without including more detail at the max zoom level. If you actually do want more detail at that max zoom level, then use the max zoom level for rendering flag instead. Setting max zoom level for rendering less than max zoom results in undefined, I'll try adding an error message at startup that explains a little better. |
|
Aha, now I get it, thanks. Client (at least mine) will overzoom if you say that generated map is to zoom 8 (source in style.json) and you want max zoom 14 (for example in mapbox map). But when you have merged map with some parts (planet) to level 8 and some region (country) to 14 will:
I thought it's solving that problem. My mistake, sorry. |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This will allow rendering with
—maxzoom=6while having zoom 6 rendered the same way as if—maxzoom=14.maxzoomForRenderingwill be used for simplifly algorithms. The default isMAX_ZOOM(14) so by default whatevermaxzoomis used you will get the same render as ifmaxzoom=14