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 "tourism=information" and "office" to lower priority in amenity-points query #3728

Merged
merged 4 commits into from
Apr 4, 2019

Conversation

jeisenbe
Copy link
Collaborator

@jeisenbe jeisenbe commented Mar 24, 2019

Fixes #3481

Changes proposed in this pull request:

  • Move tourism=information to the lower priority tourism query within project.mml
  • Move office to below the second tourism query

Explanation

This change makes sure that natural features and other amenities render first, and are not blocked by a tourism=information tag on the same node.

Since tourism offices are more important than most offices for a general map users, they can be tagged with office=* which would then be rendered as a blue office dot and text label after this change. Therefore, office is moved after tourism in the relevant layers within project.mml

A few features which are double-tagged with "office=*" and another major tag will be affected by this change, for example, an office in a building that is also tagged as "historic=manor". This should be a neutral or beneficial change, because offices are low-importance features for a general map.

Test renderings:

Before

After
tourism-office-new

Krzyzne Saddle

https://www.openstreetmap.org/node/1200864424#map=18/49.22847/20.04718
Before z18
z18-saddle-before
z19
z19-saddle-before

After z18
z18-krzyzne-saddle-after
z19
z19-krzyzne-saddle-after

Residenz des Botschafters

Double-tagged with office=diplomatic and historic=manor; this is one of the very rare possible combinations of office with a feature that is currently queried later that might be correct tagging (though I don't have any local knowledge about this area to confirm).
https://www.openstreetmap.org/way/23547938#map=19/52.48378/13.28926
z19 before
z19-residenz-botschafters-before
after
z19-residenz-botschafters-after

This change makes sure that natural features and other amenities render first, and are not blocked by a tourism=information tag on the same node
@Tomasz-W
Copy link

Please add #1372 with peaks priority.

@matkoniecz
Copy link
Contributor

@Tomasz-W It can be also done with a separate PR.

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.

Looks fine. Ultimately the prioritization of tags in the feature COALESCE() should be the same as the sorting order so prioritization of two tags on the same node is the same as for two nodes next to each other. And as said elsewhere prioritization should also be synchronized with starting zoom levels.

Managing all of this together by hand is next to impossible with the number of POI types we have meanwhile of course.

Relates to #3712.

@imagico
Copy link
Collaborator

imagico commented Apr 4, 2019

Since this heavily interferes with #3712 it would be good if you could re-base and re-test this with the new code structure.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Apr 4, 2019

I fixed the merge conflicts.

Thank you for #3712, it's a huge improvement, @pnorman

I notice that tourism=viewpoint ended up duplicated on two different lines after #3712:

                'tourism_' || CASE WHEN tourism IN ('viewpoint') THEN tourism ELSE NULL END,
                'barrier_' || CASE WHEN barrier IN ('toll_booth') AND way_area IS NULL THEN barrier ELSE NULL END,
                'waterway_' || CASE WHEN waterway IN ('dam', 'weir', 'dock') THEN waterway ELSE NULL END,
                'man_made_' || CASE WHEN man_made IN ('cross') AND way_area IS NULL THEN man_made ELSE NULL END,
                'historic_' || CASE WHEN historic IN ('wayside_cross', 'wayside_shrine') AND way_area IS NULL THEN historic ELSE NULL END,
                'tourism_' || CASE WHEN tourism IN ('viewpoint', 'attraction') THEN tourism ELSE NULL END
              ) AS feature,

I can remove one of the duplicates in this PR, or open another PR to discuss it.

@imagico
Copy link
Collaborator

imagico commented Apr 4, 2019

Yes, i think you can remove the duplication here just as well.

@jeisenbe
Copy link
Collaborator Author

jeisenbe commented Apr 4, 2019

OK, I think a node double-tagged with "tourism=viewpoint" and "man_made=cross" or "historic=wayside_shrine" should show the physical object preferentially (the cross or shrine), rather than the viewpoint icon, so I've removed "viewpoint" from the first line.

I also checked that the test renderings are the same as those previously shown above

@imagico imagico merged commit b19b961 into gravitystorm:master Apr 4, 2019
@imagico
Copy link
Collaborator

imagico commented Apr 4, 2019

Nice, thanks.

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.

4 participants