Skip to content
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

Fix draw order for low-zoom amenity points #4836

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

kaneap
Copy link
Contributor

@kaneap kaneap commented Jul 4, 2023

Fixes #2431 Also partially addresses #3880 for low zoom amenity points.

Changes proposed in this pull request:

Test rendering with links to the example places:

Bus stops now render above shelters:

Location: https://www.openstreetmap.org/#map=16/43.0436/-87.9453
Before
busstop_before

After
busstop_after

Cave no longer obscured by drinking water

A cave entrance (starting zoom 15) is no longer obscured by a drinking water tap (starting zoom 17) at zoom 17. Location: https://www.openstreetmap.org/#map=17/46.32509/9.42185
Before
cave_before

After
cave_after

Large tower no longer obscured by minor dish

A major radio tower (height 374m and starts rendering at zoom 14) is no longer obscured by a small communication dish (starting render zoom 17) at zoom 17. Location: https://www.openstreetmap.org/#map=17/43.11092/-87.92899
Before
tower_before

After
tower_after

kaneap and others added 5 commits June 30, 2023 20:09
for amenity points levels 1-15 add minzoom so  that they render with the correct priority (i.e. low zoom features don't get covered by ones that appear at higher levels)
@imagico
Copy link
Collaborator

imagico commented Jul 5, 2023

Thanks for working on this.

This change as is seems to partially duplicate the zoom level logic from amenity-points.mss inline in SQL code in project.mml with a CASE statement. I have not checked if it does so accurately but i will assume for the moment it does. The starting zoom level in SQL is used to order the features in rendering while the actual filtering of what is shown at what zoom level is done in MSS based on the separately implemented logic there.

This is indeed the simplest way to approach what i suggested in #2431 (comment). However, this would ultimately mean duplicating the whole zoom level logic from amenity-points.mss into SQL. This is not desirable i think. As explained in #3880 (comment) - if you have the starting zoom level in SQL already there is no point to also have it in MSS in addition. Instead you can do the filtering in SQL as well. Apart from this duplication issue this seems in principle a viable approach to address #3880.

Personally i would not be in favor of trying to address #3880 by inlining the zoom level logic in project.mml. I would rather go the route sketched in #3880 (comment) to generate the necessary SQL code via script from a formulation of the zoom level logic that is designed for maintainability and scalability (allowing us in particular to better manage #3635).

Since @pnorman has in the past been opposed to all suggestions made to modularize SQL code of the style i would like his opinion on this and what he considers the best way to address #3880.

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.

Bus stops should have priority over shelters
2 participants