-
Notifications
You must be signed in to change notification settings - Fork 829
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
Add natural=peninsula
labels
#4778
Conversation
Thanks for the pull request.
This change would render them the same way we had rendered So lets not make the same mistake twice. I would be fine with rendering |
Okay, this sounds fine. I'd just like to see these supported in whatever way makes the most sense. |
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.
Design wise this is fine - but we don't want to select the starting zoom level based on way_area due to the bad experience with that in the past with other features. If you have the time i would encourage you to test if starting peninsula labels at z15 or even z14 would work - we start bays at z14, though of course bays are on water so their labels are less likely to conflict with other labels.
In the long terms we could - both for peninsulas and for bays - consider using data preprocessing for meaningful importance rating and label placement independent of how the features are mapped. See past discussion in #3634 and also #3720. Since this would be a somewhat more substantial project unlikely to be completed by volunteers and not tied specifically to OSM-Carto it would be a good candidate for anyone looking into sponsoring meaningful work in support of better quality in OSM community maps.
# Conflicts: # project.mml
I do think that starting zoom will need to be 14 like Although I agree entirely that allowing a starting zoom based on area leads to bad mapping, I worry that |
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.
Looks fine to me now, thanks for keeping working on this.
There are two now unnecessary changes in the low zoom layer that should be removed - see inline comments.
As far as the starting zoom level is concerned - @dch0ph has a point that z14 might be more consistent with the other features we render with labels - natural=bay
but also natural=cape
. I had a quick look over the data and there are some cases where z14 would be too early to show a label - but these are not that frequent. Which could be partly because peninsula mapping is so far concentrated on higher latitudes.
Anyway - i would be fine with adding this as is but i would also fine with starting at z14 already (and accepting that a revision of the starting zoom level might be necessary if this starts being used in high density in the future - as we have done for other features in the past).
Thanks for the feedback. I'm fine merging as-is and then considering revisions later. |
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.
Label design of these is further discussed in #5062 |
Fixes #3897.
natural=peninsula
was approved in 2019 and has grown in usage since then. The tag is used for various large landmasses that often appear on maps. For example: the Arabian, Kamchatka, Delmarva, Jutland, and Tiburon Peninsulas. I this we can just apply the styling forplace=island
given their similarities as landforms surrounded by water.I didn't yet spin up a development instance of this project for testing, but I can try to do so later.