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

Move small amenities to z18 #4044

Merged
merged 1 commit into from
Mar 19, 2020

Conversation

jeisenbe
Copy link
Collaborator

Related to #4002, #1745 and PR #1884

Changes proposed in this pull request:

  • This PR, similar to closed PR Pushing small amenities to higher zoom levels #1884, moves amenity=atm and amenity=post_box to z18, though in this case they are currently rendered only at z19, while previously they were rendered at z17.
  • Also emergency phones are moved back to z18 from z19. Previously they had been rendered at z17

This is an attempt to compromise. While these features were moved from z17 to z19 without consensus, rather than attempting to move them back to z17 we can use z18.

In rural areas these features should be shown at z17, while in some very large cities (e.g. Warsaw) there may be many icons in one screenshot at that zoom level due to the high density of features in capital cities.

@kocio-pl
Copy link
Collaborator

What rule would we have after that for choosing zoom levels? I have proposed size rule as rule of the thumb and all the other proposed rules were more subjective or not applicable for some types of objects.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Feb 27, 2020 via email

@kocio-pl
Copy link
Collaborator

I have thought more about the size and would be more following it currently than then, because it's simple, universal and used already in this style (at different scales).

Usually places of worship are indeed larger (higher, which also applies to at least some peaks).

Large size might make a feature significant at lower scales, but it is not the most important consideration.

Which leads me to a fundamental question about importance, which is - for whom it is important and specifically why do you think it applies only to low zoom?

@imagico
Copy link
Collaborator

imagico commented Mar 3, 2020

This is an attempt to compromise. While these features were moved from z17 to z19 without consensus, rather than attempting to move them back to z17 we can use z18.

More specifically this change would be a partial revert of #2993, #3372 and #3445.

While i don't mind this change in principle if it helps moving us back towards consensus i am unsure if it actually does.

With these changes (as in particular became clear in the discussion on #3372) the massive schism in the fundamental approach to cartographic design that we struggle with became very visible to everyone i think.

I don't think ultimately trying to make compromises between two very different and fundamentally opposite strategies will lead us very far. Yet it is good to try addressing this matter as one of the key manifestations for the loss of consensus on cartographic strategy we struggle with now.

Regarding the change itself and independent of the whole backstory - Essentially most has already been said in #1884 back in 2015.

This PR, similar to closed PR gravitystorm#1884, moves amenity=atm and amenity=post_box to z18, though in this case they are currently rendered only at z19. Also emergency phones are moved back to z18 from z19
@jeisenbe jeisenbe force-pushed the small-amenity-zoom branch from 9112f79 to 67024b2 Compare March 7, 2020 05:28
@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Mar 7, 2020

Rebased for v5.0.0

@Adamant36
Copy link
Contributor

I had suggested a while back that z18 is kind of an under used zoom level that could be used sort of in this way, except more planned out. Just rendering random things there just to compromise though would cause it to be an unplanned out icon dump in a similar way to how z19 currently is. That said, I think z18 has some potential as a place for related icons that don't fit in well at z17 or z19, or something.

Copy link
Collaborator

@imagico imagico left a comment

Choose a reason for hiding this comment

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

Works fine.

@jeisenbe
Copy link
Collaborator Author

I will wait till after v5.0.0 to merge this.

@jeisenbe jeisenbe merged commit 3187a7a into gravitystorm:master Mar 19, 2020
@jeisenbe jeisenbe deleted the small-amenity-zoom branch March 19, 2020 01:49
jeisenbe added a commit that referenced this pull request Mar 19, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants