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

Reduce priority of tourism=attraction and render from z17 #3603

Merged

Conversation

jeisenbe
Copy link
Collaborator

@jeisenbe jeisenbe commented Dec 27, 2018

Fixes #3545
Related to #1063 (comment)

Changes proposed in this pull request:

  • Add generic "dot" icon for tourism=attraction
  • Render tourism=attraction only at z17 and above
  • Move tourism=attraction to low-priority label, after other features, so that it will only render if there is no other more specific tag (this fixes "tourism" refrains "man_made" node from rendering "name" #3545 "tourism=attraction stops man_made=cross from rendering")
  • Change text to standard font, render text and dot in tourism color ( #660033 now)

Explanation:

  • Features tagged tourism=attraction will currently render earlier than z17 if tagged on a large area (closed way or multi polygon), however there is no area fill or icon
  • Area rendering was previously rejected in Add area render for tourism=attraction #1257 "Add area rendering for tourism=attraction"
  • This PR will add a generic icon (a small circle) to show the location of the node or center of the area
  • Large areas will no longer get a label before z17, because this invites misuse of the tag to get a text label to render on features that would otherwise not be rendered
  • Generally we should avoid showing text name labels without an icon, outline or area fill, because this can lead to mistagging for rendering. This is particularly prone to vandalism when it can be used to render text at very early zoom levels (eg before z15)
  • The feature is moved to the low-priority layer, and last in the SQL query, so that it will only be rendered if it is the only tag on the feature. This fixes a problem where wayside crosses and man_made=cross that are also tagged tourism=attraction do not render properly.

Test rendering with links to the example places:

Singapore zoos
http://openstreetmap.org/#map=16/1.4034/103.7966
z16 - unchanged before/after
z16-zoos-after
z17 Before
z17-singapore-zoo-660033
z17 After
z17-zoo-after

Universal Studios and Kidzania
http://openstreetmap.org/#map=17/1.25438/103.82532
z17 before
z17-universal-studios-660033
z17 after
z17-universal-kidzania-after

Change text to standard font, render text and dot in tourism color
@kocio-pl
Copy link
Collaborator

How would it compare to shop violet dot? I believe this will be too similar.

Also please make as small changes as possible and put them into separate PRs. We have a lot of discussions which push such complex propositions to the edge of being never finished and merged, which would be waste of your great work.

@Adamant36
Copy link
Contributor

Looks like one of your before photos screwed up in the upload process.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Dec 27, 2018 via email

@jeisenbe
Copy link
Collaborator Author

I hope it's not too complicated. This seemed to be the smallest change that would properly fix #3545 at the time. I assumed the icon and text-dy were needed, though perhaps that is incorrect?

We could try without adding the generic dot, but it's not great to show a text label without an icon or outline or area, as @gravitystorm mentioned in
#1063 (comment)

There are several purple shop dots in the test images above, by the Singapore Zoo entrance (upper left, near some orange fast food dots), and in the retail area "Resort World Sentosa" in the last image.

Here's z18, where the shop icons show, to compare with the tourism dot and color:
z18-zoos-after

@Adamant36
Copy link
Contributor

Hhhmmm looks different enough in my opinion. Maybe the shop color could be slightly lightened to or something so they are that much easier to tell apart though.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Dec 27, 2018

For me the fact that you have used comma in the title and listed 4 changes in one PR is a simple indicator that this is complex solution.

When they are rendered near each other with a full icon I can see the difference, but when it comes to scattered dots they look the same, so I would not go with that.

@jeisenbe
Copy link
Collaborator Author

Here's the location of the original issue:
https://www.openstreetmap.org/#map=19/-23.08121/-47.98788
Node with man_made=cross and tourism=attraction

Before z17
z17-cross-before
After z17
z17-cross-after

@jeisenbe
Copy link
Collaborator Author

We could try a small square or circle instead of the dot:

  1. Solid Square, 4x4 pixels:
    z17-zoo-squares

  2. Hollow Circle, 6 pixels:
    z17-zoo-circles

@kocio-pl
Copy link
Collaborator

Yes, that sounds sane for me.

@jeisenbe
Copy link
Collaborator Author

  1. Small circles, 4 pixel:
  • not as recognizably circular as 2. (above), but still clearly different than the filled dots.
    z17-zoom-small-circles

@jragusa
Copy link
Contributor

jragusa commented Dec 27, 2018

circles could be in conflict with spring (#3461). Additionally, they are also used with major cities at low zoom although it's different zoom level.

@jeisenbe
Copy link
Collaborator Author

  1. Square icon, 3 pixel width:
    3-pixel-squares-textdy4

The square icon is clearly different from the circular dots, yet is unobtrusive, and no new svg icon is needed.

I've pushed a new commit with this change.

(I would also be happy to remove the icon from this PR, if this is too many changes to review in one pull request).

@Adamant36
Copy link
Contributor

Square icons are for specific things. They aren't going to be confused with transportation squares in this context. So, I think this would be a good usage of them.

@kocio-pl
Copy link
Collaborator

I have just checked that 85% has name:

https://taginfo.openstreetmap.org/tags/tourism=attraction#combinations

Yet I'm not sure how the rest (15%) will look with just a dark violet square. Especially for areas. What do you think about it?

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Dec 29, 2018 via email

@kocio-pl
Copy link
Collaborator

Hard to say, Taginfo provides only simple stats.

@Adamant36
Copy link
Contributor

The question is would not rendering it if it doesn't have a name incentives adding fake names. Or is it that 100% of tourist attractions have legitimate names that it would encourage people to add. It could probably go either way. Although its more then likely that most attractions have names and the 15% that don't just haven't been added yet. If that's the case, I say make the rendering exception or if not, don't.

@jragusa
Copy link
Contributor

jragusa commented Dec 29, 2018

Some features were tagged with tourism=attraction because of the lack of rendering of the initial tag, for example waterway=waterfall, and I suppose that several of them are not still corrected.

@jeisenbe
Copy link
Collaborator Author

I've added a commit. Now the icon will only render if the name is not null.

@matthijsmelissen
Copy link
Collaborator

I think the example images are mistagging - attraction=tourism is not intended for theme park attractions, but only for things that are tourist attractions themselves (the theme park would be, but the attractions are not, in my opinion).

For regular tourist attractions, I don't think a dot would improve the rendering situation.

@kocio-pl
Copy link
Collaborator

After some more thinking I would also not like to add dots/squares. All the other propositions (including moving rendering of general "attractions" to z17 to not encourage abusing this tag) are OK for me.

My proposition is to remove dot rendering and then I can go on with code review.

@jeisenbe jeisenbe force-pushed the attraction-low-priority branch from 7d1f2bd to 2309507 Compare December 30, 2018 14:34
@jeisenbe
Copy link
Collaborator Author

Ok, I've removed the generic icons for now, as they are not required to solve the main issue. The code should be ready for review

My opinion is that tourism=attraction is most useful when it is used along with attraction=*; the most common values of attraction are animal (eg zoo animal), roller coaster, water slide, summer toboggan, etc - all theme park or zoo features. I almost all other cases there is a better primary tag for a feature that the generic tourism=attraction.

I also note that @gravitystorm previously recommended that features should have either an icon or an area rendering of some sort, in this comment:
#1063 (comment)

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Dec 30, 2018

For the record, here are a couple of examples of why we should no longer render the text labels sooner than z17.

This is a real-world example of a very large "tourism=attraction", apparently the whole lake area, or perhaps the surrounding caldera too? It renders on z13, as I happened to notice while looking for areas with patterns:
z13-sete-cidades-mistagging

An arbitrarily large area tagged as "tourism=attraction" renders even on z10.
[Not real data]
z10-wamena-tourism-tagging-for-renderer

@kocio-pl kocio-pl changed the title Move tourism=attraction to low-priority layer, render from z17 with dot Move tourism=attraction to low-priority layer and render from z17 Dec 30, 2018
@jeisenbe
Copy link
Collaborator Author

I've removed the two lines from amenity_low_priority and amenity_low_priority_poly in project.mml because we are not going to render an icon.

Since no icon will be rendered for tourism_attraction, it is not necessary to have the feature in these two layers now.
@jeisenbe jeisenbe force-pushed the attraction-low-priority branch from e86c53b to da66353 Compare December 30, 2018 23:51
@jeisenbe jeisenbe changed the title Move tourism=attraction to low-priority layer and render from z17 Reduce rendering priority of tourism=attraction and render from z17 Dec 30, 2018
@jeisenbe jeisenbe changed the title Reduce rendering priority of tourism=attraction and render from z17 Reduce priority of tourism=attraction and render from z17 Dec 30, 2018
@matthijsmelissen matthijsmelissen merged commit da66353 into gravitystorm:master Jan 3, 2019
@jeisenbe jeisenbe deleted the attraction-low-priority branch January 17, 2019 02:55
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.

"tourism" refrains "man_made" node from rendering "name"
5 participants