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

Render remaining tourism_information features #3246

Merged
merged 1 commit into from
Jun 20, 2018
Merged

Render remaining tourism_information features #3246

merged 1 commit into from
Jun 20, 2018

Conversation

jragusa
Copy link
Contributor

@jragusa jragusa commented May 26, 2018

Fixes #304
Fixes #324
Fixes #1978

Changes proposed in this pull request:
This PR completes #3198 by adding rendering of audioguide, board, map, office, tactile_map, tactile_model and terminal

Test rendering with links to the example places:

information=audioguide
https://www.openstreetmap.org/node/3066454413
tourism_audioguide

information=board
https://www.openstreetmap.org/node/2857924837
script-version
tourism_board
i-version
tourism_board2

information=map
https://www.openstreetmap.org/node/4939116583
tourism_map

information=office
https://www.openstreetmap.org/way/130442457
z=17
tourism_office_z17
z=19
tourism_office_z19
with other brown amenities
https://www.openstreetmap.org/node/5537445277
tourism_office

information=tactile_map
https://www.openstreetmap.org/node/3571444993
tourism_tactile_map

@jragusa
Copy link
Contributor Author

jragusa commented May 26, 2018

Some remarks:

  • tourism=officeis displayed earlier (z=17), is it ok ?
  • I don't have tactile_model in my data.osm but I can find out elsewhere if needed as well as audioguide with a name.
  • I moved all related icons in a specific folder tourism.
  • Sometimes map icon is displayed above a track or a path which may reduce the readability of the icon
  • Finally, I would also proposed to render guideposts at z=18 (following @Tomasz-W, because there is not so much features in countryside or in mountain area and they are important landmarks.

@kocio-pl
Copy link
Collaborator

Thanks! I plan to review and test it later.

  1. I think tourism office is like other offices and shops, so z17+ is perfect for me.

  2. You can export small areas in OSM format just using the website:

https://www.openstreetmap.org/export

Import them using the same osm2pgsql command as for PBF files.

  1. Unfortunately guideposts are very popular part of the city information systems, so I would not elevate their rendering level. If some tags will be invented to help make the difference between outdoor and city usage, I would be happy to render the former type earlier.

@dieterdreist
Copy link

dieterdreist commented May 26, 2018 via email

@kocio-pl
Copy link
Collaborator

Touristic offices are usually located in the urban areas, so I would not like to see them earlier, because of problems with city items clutter.

When they are located near some outdoor attractions, they are probably similar in importance with gift shops or ticket shops, so still nothing to be shown earlier for me.

@Tomasz-W
Copy link

As brown is heavily loaded by gastronomic, historic and other tags, and information=* objects are not so important, I would like to see test renderings of them in man-made grey. We could also consider to render most of these icons in grey, but left generic and information=office icons in brown.

@nebulon42 related to #3198

@Tomasz-W
Copy link

@jragusa please add information=terminal icon:

information terminal

Gist link: https://gist.github.com/Tomasz-W/e8cf9ed9647381ab59370a6ce3a8f312

@kocio-pl
Copy link
Collaborator

I hope you will make a rendering test for man_made gray?

@jragusa
Copy link
Contributor Author

jragusa commented May 28, 2018

Sorry, I was too tired last evening to prepare the renderings. Here they are with
man-made grey colour:

information=audioguide
https://www.openstreetmap.org/node/1412978425
tourism_audioguide_grey

information=board
https://www.openstreetmap.org/node/2857924837
tourism_board_grey

information=guidepost
https://www.openstreetmap.org/#map=19/46.21323/6.46334
tourism_guidepost_grey

information=map
https://www.openstreetmap.org/node/4939116583
tourism_map_grey

information=office and information=terminal
https://www.openstreetmap.org/node/2473539607
https://www.openstreetmap.org/way/380875391
tourism_terminal

@Tomasz-W
Copy link

Now I'm sure that most of these icons should be grey, but I still thinking what to do with information=office and generic "i" icon. My proposition:

  • information=office -> brown or office-blue (it's something more complicated than a free standing object)
  • generic icon -> grey

@dieterdreist
Copy link

dieterdreist commented May 28, 2018 via email

@kocio-pl
Copy link
Collaborator

generic icon -> grey

👍

offices in OSM are mostly about places you can't just step in (as opposed to shops).

I think the difference is not that clear, and you can walk in some of them, but you just won't directly buy things most of the time. I think brown is good, since the main function of information office is getting informations, not buying (though one can buy maps or gifts also).

@Tomasz-W
Copy link

Tomasz-W commented May 29, 2018

@jragusa
summary:

  • audioguide, board, map ,tactile_map, tactile_model, terminal, guidepost -> grey
  • generic icon -> grey
  • office - > brown

@jragusa
Copy link
Contributor Author

jragusa commented May 30, 2018

following last recommendations:
https://www.openstreetmap.org/#map=19/46.26756/6.84134
tourism_brown grey

@jragusa
Copy link
Contributor Author

jragusa commented May 31, 2018

@kocio-pl Do you want I squash commits into one for merging ?

@kocio-pl
Copy link
Collaborator

I always prefer it, but that's optional. It's just my more relaxed time now, but I can test and merge it in the upcoming days.

@jragusa
Copy link
Contributor Author

jragusa commented May 31, 2018

ok, it's done

@Tomasz-W
Copy link

Tomasz-W commented Jun 8, 2018

@jragusa As this PR is ready to merge, are you interested in write code for simillar issue of #3088 ? It's even less complicated, because there is only one colour for all icons (memorial-brown).

@@ -316,17 +316,32 @@
marker-clip: false;
}

[feature = 'tourism_information'] {
[information != 'guidepost'][zoom >= 17],
[feature = 'tourism_information'][zoom >= 17] {
Copy link
Collaborator

Choose a reason for hiding this comment

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

You need to exclude all the cases that should be rendered later, like audioguide or map, because otherwise they will be rendered with standard icon from z17 and as a specific icon from z19.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok I thought that it should be the correct behaviour as they currently appear at z17 with the generic icon. So you prefer that only office is displayed at z17 ?

@jragusa
Copy link
Contributor Author

jragusa commented Jun 12, 2018

@kocio-pl if last commits are ok for you I can squash them

@kocio-pl
Copy link
Collaborator

Thanks, that looks good! I will check it soon, I hope.

@kocio-pl
Copy link
Collaborator

Ok, this all seems to be working good, you can squash it and I'm ready to merge it.

@jragusa
Copy link
Contributor Author

jragusa commented Jun 13, 2018

Done

I also homogenised the code of label with icon. It's now more efficient and clean

@kocio-pl
Copy link
Collaborator

Please don't add the foreign code changes like:

(tags->'office') IS NOT NULL

It needs a separate PR.

@jragusa
Copy link
Contributor Author

jragusa commented Jun 14, 2018

I squashed one commit in excess. It's ok now

@kocio-pl kocio-pl merged commit fbd9d4c into gravitystorm:master Jun 20, 2018
@kocio-pl
Copy link
Collaborator

Thanks for you work and updates, sorry for such long delay with re-testing.

@jragusa
Copy link
Contributor Author

jragusa commented Jun 21, 2018

No worries, it's summer time :)

@Tomasz-W
Copy link

@jragusa Hey, I've found some problems with icon + name rendering of some of information=* subtypes. While eg. map or generic icon rendering looks ok, names of information=board or information=terminal are on the down left side of the icon, examples:
https://www.openstreetmap.org/node/4935294691
https://www.openstreetmap.org/node/5679218763
https://www.openstreetmap.org/node/4215316042
Can you take a look on this and try to fix it?

@jragusa
Copy link
Contributor Author

jragusa commented Aug 22, 2018

Indeed, some artefacts remain in the svg files (filled in grey):

tourism_bug

That's concern audioguide, board and terminal. I will make a PR to fix this bug.

@Tomasz-W
Copy link

Tomasz-W commented Oct 6, 2018

I don't know how it happened, but I noticed that there is board.svg icon used for information=tactile_model when it's actualy a horizontal 3D map in most cases, so it's surely closer to a map than a board and shoud be rendered by map.svg icon. @jragusa Can you make a PR fixing it?

Example: https://www.openstreetmap.org/node/4362875819

@jragusa jragusa deleted the tourism_information branch October 7, 2018 11:21
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