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

Add more swimming icons #3422

Merged
merged 3 commits into from
Oct 7, 2018
Merged

Add more swimming icons #3422

merged 3 commits into from
Oct 7, 2018

Conversation

dryo
Copy link
Contributor

@dryo dryo commented Oct 2, 2018

Partially fixes #2018

Changes proposed in this pull request:

  • add icons for leisure=sports_centre + sport=swimming and leisure=swimming_area

@HolgerJeromin
Copy link
Contributor

HolgerJeromin commented Oct 3, 2018

thanks for your work!
Have you tested this?
I am pretty sure this is missing a sql change

@Tomasz-W Tomasz-W mentioned this pull request Oct 3, 2018
26 tasks
@dryo
Copy link
Contributor Author

dryo commented Oct 3, 2018

Hi @HolgerJeromin
Thanks for your appreciation and hint. I forgot to input the SQL changes. Did that now.
Unfortunately I currently don't have the resources to test. Maybe someone else with more equipment at hand has time for that?

@Adamant36
Copy link
Contributor

Adamant36 commented Oct 4, 2018

@dryo, I just tested it. It looks like you didn't add the code for them to rendered with their names.
For instance, this sports centre should say "Richmond Swim Center" under the icon.
sports centre no name
Here's the location of the place https://www.openstreetmap.org/#map=19/37.92399/-122.32714

At least the icon showed up. So, you know that works. I'll retest it and swimming areas once the extra code is added.

@dryo
Copy link
Contributor Author

dryo commented Oct 4, 2018

@Adamant36 A big thank you for testing! My main goal was to have icons but I see your point especially when it comes to sports centres. I added the code that should display the name label the same way it is with water parks.
If you have the time to test again I'd be very happy!

@Adamant36
Copy link
Contributor

Your welcome. Thanks for doing it. I'm pretty happy you did, because it lays the groundwork for the other sports icons to be added, which I've been meaning to do at some point.
Sports Center
https://www.openstreetmap.org/#map=18/37.92395/-122.32794 (node)
sports center node
https://www.openstreetmap.org/#map=19/37.89536/-122.29191 (way)
sports center way
Swimming Area
https://www.openstreetmap.org/#map=17/37.80839/-122.42381 (node)
swimming area node
https://www.openstreetmap.org/#map=19/37.89572/-122.25025 (way)
swimming area way
Thoughts
So, it seems like names are rendering now. Two side notes, one the swimming area way is also tagged as a bay and it seems to over ride the blue water rendering. Two, the swimming area way is only rendering as a point , instead of an area with a fill color. The bay thing isn't that important, but I still wanted to mention it. The fill color thing for swimming area ways might be worth not rendering and leaving as is. Since it could look weird to have a green area in the middle of blue water. It might be worth testing anyway though, which I'm willing to do if need be. @kocio-pl, whats your opinion on it?

@dryo
Copy link
Contributor Author

dryo commented Oct 5, 2018

@Adamant36 Thanks for testing (again)! It seems to work as expected.

First side note:
If I understand it correctly you meant to write "point" instead of "way" and with "blue water rendering" you meant that the name of the bay is now green and no longer blue. I guess you were referring to "Aquatic Park Cove". I don't see this as a problem, however let's hear some opinions about that.

Second side note:
This PRs intention was not to fill swimming areas with color. See discussion on #1490 for that. This pull request was only for #2018 leaving out the request for another icon for water parks because IMO there seemed no common support for this idea.

@HolgerJeromin @Tomasz-W @kocio-pl I'd see this PR as ready to merge. Your opinions please ;)

@Adamant36
Copy link
Contributor

@dryo, your welcome. Yeah, thats what I meant. Sorry for the confusion. I wasnt thinking that those things were problems, but I still wanted to bring them up anyway just in case. I agree with you that its ready to merge. Thanks for writing the code. If you want me to test anything else for you in the future, feel free to ping me.

@HolgerJeromin
Copy link
Contributor

swimming area combined with bay is not optimal / wrong tagging IMO. So I am fine with this side effect.

@Tomasz-W
Copy link

Tomasz-W commented Oct 5, 2018

There is 5-10°C in Poland now, so I'm not going to swim outdoor soon, but I would be happy with merging icon for both tags anyway ;) ;) and I'm OK with no-fill/outline for swimming areas.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Oct 6, 2018

Thanks for testing and discussing it. It looks good to me and I think it should be merged.

I like especially spotting leisure=swimming_area - together with beach and beach resort it makes interesting new type of objects we show:

taghistory 53

@kocio-pl kocio-pl merged commit 09bd4f1 into gravitystorm:master Oct 7, 2018
@kocio-pl
Copy link
Collaborator

kocio-pl commented Oct 7, 2018

@dryo Thanks for this code! My testing showed that it's working as expected.

Since this is your first contribution, maybe you're interested in fixing some other warm up issues (see #3298)? If you will need some help or guidance, just drop a line.

@HolgerJeromin
Copy link
Contributor

My testing showed that it's working as expected.

You tested that this does not change sports centres without sport=swimming. Did you?

@kocio-pl
Copy link
Collaborator

kocio-pl commented Oct 7, 2018

Checked - it doesn't change standard sport centers rendering.

@Tomasz-W Tomasz-W mentioned this pull request Jan 1, 2019
19 tasks
@jeisenbe jeisenbe added new features Requests to render new features POI labels Sep 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new features Requests to render new features POI
Projects
None yet
Development

Successfully merging this pull request may close these issues.

render water sport facilities (non waterpark)
6 participants