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

differentiate guidepost from general tourism=information display #3198

Merged
merged 1 commit into from
May 23, 2018

Conversation

nebulon42
Copy link
Contributor

@nebulon42 nebulon42 commented Apr 26, 2018

Ref #1978, #3105.

Adds a guidepost icon on z19 and drops rendering of guidepost nodes before z19.

before z18
z18_before

after z18
z18_after

before z19
z19_before

after z19
z19_after

@kocio-pl
Copy link
Collaborator

If it's Atom, then you can turn it off by disabling Preferences -> Whitespace -> Remove Trailing Whitespace.

@kocio-pl
Copy link
Collaborator

Link for testing place:

https://www.openstreetmap.org/#map=19/46.90772/13.53294

@SomeoneElseOSM
Copy link
Contributor

SomeoneElseOSM commented Apr 27, 2018

Excellent idea - I'd also suggest handling trail_blaze and route_marker (see taginfo) as well as guidepost since they're even less significant. See here (and the link via the changelog to the repository) for what I went with, but ignore the PNFS and Sustrans markers since they're UK-specific.

@matthijsmelissen
Copy link
Collaborator

Could you perhaps first fix the whitespace issues in a separate PR?

@nebulon42
Copy link
Contributor Author

@matthijsmelissen Done so now in #3197. and @kocio-pl also opened #3200.
@SomeoneElseOSM Definitely worth handling, but better in a separate PR.

@jragusa
Copy link
Contributor

jragusa commented Apr 28, 2018

Several guideposts dedicated for hiking in both France and Switzerland (I don't know for other countries but I suppose too) mention a name and even an elevation. Would it be possible to add these features to the icon ?

@kocio-pl
Copy link
Collaborator

We have the code to reuse, so it should be possible. However could you show such examples?

@nebulon42
Copy link
Contributor Author

@jragusa That could be useful, I'll have a look.

@nebulon42
Copy link
Contributor Author

@kocio-pl Here are some instances in Austria: https://overpass-turbo.eu/s/ymd.

@nebulon42 nebulon42 force-pushed the guidepost branch 4 times, most recently from 95f4c41 to 0b1685c Compare April 28, 2018 19:52
@nebulon42
Copy link
Contributor Author

Additionally now the name and/or the elevation is displayed:
guidepost_name
guidepost_name_ele
guidepost_ele

@kocio-pl
Copy link
Collaborator

Sorry, I wanted the photo example and some explanation, not the link to data set. 😄

If I get it right, they show the real elevation they are located at? But what is their name then? Isn't it the same as name and elevation of the peak or saddle for example?

@nebulon42
Copy link
Contributor Author

The name is usually a place name, but can also be the name of the saddle and/or peak. Elevation is of course the real elevation. The important part here is that both name and elevation are signed on the guidepost as can be seen in e.g. https://wiki.openstreetmap.org/wiki/File:Signpost.jpg.

@kocio-pl
Copy link
Collaborator

I see. OK, so I like this solution. After all they won't eclipse peaks or saddles, because they will be rendered very late.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Apr 29, 2018

Maybe it would better to make the code more compact and easier to extend with other types, like:

[feature = 'tourism_information'][information != 'guidepost'][zoom >= 17],
[feature = 'tourism_information'][information = 'guidepost'][zoom >= 19] {
  marker-file: url('symbols/information.12.svg');
  [information = 'guidepost'] { marker-file: url('symbols/guidepost.svg') };
  marker-placement: interior;
  marker-fill: @amenity-brown;
  marker-clip: false;
}

@Tomasz-W
Copy link

@nebulon42 Do you have plans to take care of rest of tourism=information icons #1978 (comment) and/ or other related issues #304 #324 ? It would be a big progress in tourism-elements rendering in Carto.

@nebulon42
Copy link
Contributor Author

@kocio-pl I have updated the code to be more DRY.
@Tomasz-W I have no immediate plans, but I'll put it on my list. :)

project.mml Outdated
@@ -1969,6 +1971,7 @@ Layer:
'leisure_' || CASE WHEN leisure IN ('nature_reserve') THEN leisure ELSE NULL END
) AS feature,
name,
tags->'information' as information,
Copy link
Collaborator

Choose a reason for hiding this comment

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

What's this line for in text-poly-low-zoom layer? I have tested that it works without this code as well on all the zoom levels.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Same thing like #3196. :)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Oh, boy, not again... 😄

Did you have a problem without this code or it's just in case? Did you try with updated Docker image (I mean with a new Kosmtik 😄 ).

I'm ready to merge this code without this line any time. Especially since production rendering servers have no such problem and there are no other mid/high zoom features that I'm aware of which need such addition. Would you like to split it, so we could discuss the low zoom mystery separately?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll do some tests again when I have time.

Copy link
Collaborator

@matthijsmelissen matthijsmelissen left a comment

Choose a reason for hiding this comment

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

I'm fine with merging this as is.

Thanks @nebulon42 !

@kocio-pl
Copy link
Collaborator

kocio-pl commented May 7, 2018

What do you think about adding a mysterious code line?

@kocio-pl
Copy link
Collaborator

@matthijsmelissen ping?

@matthijsmelissen
Copy link
Collaborator

@nebulon42 Can you get it to work without the 'mysterious line' as well? If so, I'd prefer to merge that version.

@nebulon42
Copy link
Contributor Author

I have removed the line but did not do additional testing.

@kocio-pl kocio-pl merged commit f120683 into gravitystorm:master May 23, 2018
@kocio-pl
Copy link
Collaborator

Thanks, I've tested it and it works.

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.

6 participants