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

playground=sandpit with sand as background-color #3230

Closed
wants to merge 1 commit into from

Conversation

tordans
Copy link

@tordans tordans commented May 12, 2018

Changes proposed in this pull request:

A sandpit is always made of sand and therefore can and should be displayed with the color of sand. However, in contrast to the general sand rendering, the playground should only be rendered in higher zoom levels since the size of the sandpit is most likely small and will only clutter the map when zoomed out.

Testcase 1 Karl-Marx-Platz

https://www.openstreetmap.org/#map=19/52.47447/13.44328

Before

bildschirmfoto 2018-05-12 um 08 33 35

After

zoom-19

Testcase 2 Kirchgasse

https://www.openstreetmap.org/#map=19/52.47799/13.44190

Before

bildschirmfoto 2018-05-12 um 08 33 20

After

bildschirmfoto 2018-05-14 um 06 46 48

A sandpit is always made of sand and therefore can and should be displayed as sand. However, in contrast to the general sand rendering, the playground should only be rendered in higher zoom levels since the size of the sandpit is most likely small and will only clutter the map when zoomed out.

Details about at `playground=sandpit` at https://wiki.openstreetmap.org/wiki/Key:playground#playground-values
Details about the tag usage https://taginfo.openstreetmap.org/tags/playground=sandpit
@kocio-pl
Copy link
Collaborator

Hi! Did you try to install Docker-based testing environment?:

https://github.com/gravitystorm/openstreetmap-carto/blob/master/DOCKER.md

@pnorman
Copy link
Collaborator

pnorman commented May 12, 2018

With no changes to the MML I don't believe this PR will work when you test it.

@polarbearing
Copy link
Contributor

playground =* tags playground equipment. We decided in #3161 not to render them.
This PR would add the sandpit as a special case of equipment.

The sandpit is already rendered in sand-yellow when natural=sand is added.

@tordans
Copy link
Author

tordans commented May 13, 2018

@polarbearing thanks for the hint with natural=sand. I am testing it ATM (Change Commit).
However, I still think that the sandpit should be displayed as sand, even without this additional tag. My understanding is, I should not be tagging for renderer, but for meaning.

I know about PR #3161: The way read the discussion is, that special icons for where disliked, especially because they where confusing to read with the general playground icon in the middle. So IMO this PR has nothing to do with that.

@kocio-pl
Copy link
Collaborator

Yes, I also think that this is a special case worth adding. We want a semantic database and avoid adding natural=sand as less precise.

@tordans
Copy link
Author

tordans commented May 14, 2018

I updated the PR Description with the "after" shots based on my testing of natural=sand.

Only >=18

Based on the screesnhots I think: Lets only show the sandpit for >= 18. Otherwise very small sandpits just clutter the map.

About the code

Can anyone who knows how this works take over? It will take me a long while to setup a dev-env. I might have to migrate it into an regular issue.

Here are some more screenshots of different zoom levels

Zoom 19

zoom-19

Zoom 18

zoom-18 (there are 3 sandpit in a row)

Zoom 17

zoom-17b

Zoom 16

zoom-16

Zoom 15

zoom-15

@kocio-pl
Copy link
Collaborator

Based on the screesnhots I think: Lets only show the sandpit for >= 18. Otherwise very small sandpits just clutter the map.

Sounds good for me.

Can anyone who knows how this works take over? It will take me a long while to setup a dev-env. I might have to migrate it into an regular issue.

There's no hurry and I prefer that more people can use our toolset and produce proper code. We have too many issues and ideas, and too few regular coders.

Do you have some problem with Docker containers?

@polarbearing
Copy link
Contributor

The sand in the pit is natural. There is no artificial sand, so natural=sand is not tagging for the renderer, it is the actual fact.

@dieterdreist
Copy link

dieterdreist commented May 14, 2018 via email

@dieterdreist
Copy link

dieterdreist commented May 14, 2018 via email

@pnorman
Copy link
Collaborator

pnorman commented May 17, 2018

why not making the zoom level depending on the area size in pixels? While many sandpits are small, a huge one will appear odd when rendered only on very high zooms

We don't generally select fills based on area except for performance reasons at sizes that are hard to see.

I'm not convinced that rendering a playground feature the same as natural=sand is a good idea. Our natural=sand rendering is designed to fit in with other nature features in the style, and a playground pit serves quite different purposes, even if it has the same material. It may also look the same from above, but we're not painting an aerial image, we're drawing a map.

This is unrelated to the reason I'm closing this issue, which is that the author hasn't been able to test it or get it working, and doesn't know when they'll be able to do so.

@pnorman pnorman closed this May 17, 2018
@dieterdreist
Copy link

dieterdreist commented May 21, 2018 via email

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.

5 participants