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

Public transport shelter should not be labeled with elevation #4313

Merged
merged 4 commits into from
Jul 27, 2022

Conversation

GoutamVerma
Copy link
Contributor

Fixes #4270

Changes proposed in this pull request:
- OR (amenity = 'shelter' AND tags->shelter_type NOT IN (public_transport, picnic_shelter))

@jeisenbe
Copy link
Collaborator

Thank you for opening this PR, @GoutamVerma

Could you create “before” and “after” images of an object which will have a different rendering after this change?

I would always recommend doing this, to make sure that your changes have the expected effect and that there are no unintentional side effects from the change.

@jeisenbe
Copy link
Collaborator

@GoutamVerma Could you create “before” and “after” images of an example where this change will affect the rendering?

@imagico
Copy link
Collaborator

imagico commented Jul 17, 2021

This requires testing - otherwise it is non-controversial i think.

Copy link
Contributor

@pitdicker pitdicker left a comment

Choose a reason for hiding this comment

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

Before:
before
After (after applying the suggested change):
after

And don't know the policy, but you maybe also have to squash the commits.

B.t.w. my screenshot may look a little off, because I used an export from a small area from openstreetmap.org for testing.

project.mml Outdated Show resolved Hide resolved
Copy link
Collaborator

@pnorman pnorman left a comment

Choose a reason for hiding this comment

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

With the changes merged this should now be working, but I wonder about doing this for a total of 109 nodes.

@pnorman
Copy link
Collaborator

pnorman commented Jun 5, 2022

This could use a review from someone else. I'm not opposed to the change, although don't think it's worth it for the small number of nodes.

@jeisenbe
Copy link
Collaborator

jeisenbe commented Jun 5, 2022

According to https://taginfo.openstreetmap.org/tags/amenity=shelter#combinations only 0.83% of amenity=shelter features have an ele= tag, which is currently 3,369 objects. This PR appears to affect 126 nodes and 228 closed ways for a total of 354 objects, about 11% of those with currently rendered elevation - https://overpass-turbo.eu/s/1j2K

37% percent of features lack a shelter_type= tag and they would still have elevation rendered potentially - even though some are bus or picnic shelters in urban areas where the elevation is not useful on a general map. There are 1179 of these, some of which probably should not be rendered - https://overpass-turbo.eu/s/1j2L - for example there 23 on my city, all of which appear to be public transport or picnic shelters which lack a shelter_type= tag, though in Europe most are in the Alps where the ele= might be useful.

Of the values with more than 1000 uses, only lean_to, basic_hut and field_shelter are clearly rural or wilderness features; sun_shelter or weather_shelter are usual rural but sometimes urban. There are only 1636 of these - https://overpass-turbo.eu/s/1j2M - about half of the total that we currently render.

gazebo and pavilion are likely to be found in gardens or parks in developed areas were their elevation is unlikely to relevant, and we have already determined ele= is not relevant for public_transport and picnic_shelter

Perhaps the better solution is to remove the ele= text from all amenity=shelter features?

tag | number | percentage | wiki | description
public_transport | 129 758 | 49.48% | ✔ | A small shelter at bus stops or platforms along the rail or bus routes
picnic_shelter | 58 250 | 22.21% | ✔ | A structure on picnic sites to protect from rain.
gazebo | 18 235 | 6.95% | ✔ | A gazebo
weather_shelter | 17 372 | 6.62% | ✔ | A structure to protect from rain.
lean_to | 12 334 | 4.70% | ✔ | A lean-to is a shed with typically three walls located in the countryside intended for camping.
pavilion | 8 733 | 3.33% | ✔ | A Pavilion
basic_hut | 5 627 | 2.15% | ✔ | A basic hut is a small building intended to provide basic shelter and sleeping accommodation.
sun_shelter | 2 917 | 1.11% | - |  
field_shelter | 1 439 | 0.55% | ✔ | A field shelter is a building located in the countryside intended to provide shelter for animals (e.g., horses)
building | 991 | 0.38% | - |  
rock_shelter | 970 | 0.37% | - |  
parking | 724 | 0.28% | - |  
wildlife_hide | 543 | 0.21% | ✔ |  
shopping_cart | 440 | 0.17% | - |  
dugout | 424 | 0.16% | - |  
roof | 397 | 0.15% | - |  

In contrast, there are over 4000 tourism=alpine_hut with an ele tag, and 1/2 million natural=peak with ele which we render, with a clearer justification. We are not yet rendering mountain_pass=yes - see #244 - which has over 20,000 features with ele= where rendering ele would be relevant. Also 2000 tourism=wilderness_hut have an ele tag, but currently it is not rendered. These seem very similar to tourism=alpine_hut and probably should have ele rendering added)

In conclusion, could make sense to either expand this PR to exclude more values of shelter_type (gazebo, pavilion at least) or to only to include the smaller number of shelter types that one would find in the mountains (weather_shelter, lean_to, basic_hut, sun_shelter...). Either way it would be a bit of extra code for a small number of features, less than 2000.

A simpler solution would be to remove ele rendering for all amenity=shelter features, and perhaps add it for tourism=wilderness_hut (and eventually mountain_pass=yes)

@jeisenbe
Copy link
Collaborator

jeisenbe commented Jun 5, 2022

Looking a bit more, I now realize that most amenity=shelter with ele but no shelter_type in Europe are in the Alps, where the info might be useful, unlike the case here in the USA where most seem to be untagged public transport shelters or picnic shelters.

Also shelter_type=field_shelter never is used with ele so it can be ignored (they are usually on farms), and shelter_type=sun_shelter is almost never used with ele (except a few times in Los Angeles). There are only about 100 total with ele and a shelter_type which is suspect - https://overpass-turbo.eu/s/1j2O - so a more complex query that just looking for public_transport, picnic_shelter will have little effect, unless we drop all the shelters without shelter_type.

@jeisenbe
Copy link
Collaborator

jeisenbe commented Jun 5, 2022

I’m not certain the best course of action. @imagico do you still support merging this PR? It looked like you were in favor above.

@imagico
Copy link
Collaborator

imagico commented Jun 6, 2022

I concur with your analysis that

  • some shelter types would be good to render with elevation while others not
  • unfortunately many shelters, both remote in mountain areas and urban ones, don't have a shelter_type
  • for the moment special casing shelter_type=public_transport seems a bit inefficient in light of the low number of cases where this has an effect (because ele values are rarely tagged on those anyway).

I would still be moderately in support of this idea because

(a) the alternative to complete remove ele rendering from amenity=shelter without shelter_type would mean a significant loss of useful information in many mountain regions.
(b) the alternative of only positively rendering ele on certain shelter_type values does not seem practicable because the shelter_type classification does not really much correlate with the significance of the ele information for the map user and point (a) would still apply.

The idea of extending the list of shelter_type values to be not rendered with ele is worth discussing but i am not sure if there are values where this is similarly clear as with shelter_type=public_transport. There are plenty of gazebos at viewpoints in mountain areas for example where ele values are highly useful.

I would however prefer it if the introduction of a special case for ele on shelter_type=public_transport would be combined with a symbol variant (i.e. adjusting #4317 to use a symbol that is a minor variation of the generic shelter symbol). That would help mappers to intuitively understand the differences in labeling logic. And it would help support mappers in more precise classification of shelters which could help us - and other styles - to improve our rendering logic in the future.

@pnorman pnorman merged commit 4c12195 into gravitystorm:master Jul 27, 2022
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.

Public transport shelter should not be labeled with elevation
5 participants