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 amenity=clock icon #3135

Closed
wants to merge 1 commit into from
Closed

Conversation

james2432
Copy link
Contributor

@james2432 james2432 commented Mar 20, 2018

Fixes #3070
See comment below due to rework
From https://www.openstreetmap.org/#map=19/45.01689/-75.64647 (node)
Z18:
image
Z19:
image

@james2432 james2432 changed the title [WIP] Issue #3070 Add amenity clock icon [WIP] Add amenity clock icon Mar 20, 2018
@kocio-pl kocio-pl changed the title [WIP] Add amenity clock icon [WIP] Add amenity=clock icon Mar 20, 2018
@james2432 james2432 changed the title [WIP] Add amenity=clock icon Add amenity=clock icon Mar 20, 2018
@HolgerJeromin
Copy link
Contributor

We had this idea in the past:

I think all allowed clocks should be rendered on zoom 19, but only clocks with visibility=area should be on zoom 18.

@Tomasz-W
Copy link

Please include in code:

@lakedistrictOSM
Copy link

Will places tagged with both amenity=clock and historic=monument
(like https://www.openstreetmap.org/node/456474545) get rendered with the clock icon or the monument icon?

@Tomasz-W
Copy link

Will places tagged with both amenity=clock and historic=monument
(like https://www.openstreetmap.org/node/456474545) get rendered with the clock icon or the monument icon?

I've checked this combination with Overpass Turbo and Google Earth, most of examples doesn't have any other function than being a clock, so amenity=clock tag should have a pririty there IMO.

@james2432
Copy link
Contributor Author

From: https://www.openstreetmap.org/#map=19/35.78917/-78.82883

image
Reworked to clock zoom 18 and needs visibility=area

@dieterdreist
Copy link

dieterdreist commented Mar 21, 2018 via email

@Tomasz-W
Copy link

Tomasz-W commented Mar 22, 2018

Reworked to clock zoom 18 and needs visibility=area

@james2432 We mean to render all clocks on zoom 19 (visibility=* doesn't matter) and clocks with visibility=area on zoom 18

@dieterdreist
Copy link

dieterdreist commented Mar 22, 2018 via email

@james2432
Copy link
Contributor Author

@Tomasz-W I understand now, will tweak :) 👍

@kocio-pl
Copy link
Collaborator

It's important to test how does it look with churches/towers and stations - people may tag them on such objects, not only standalone clocks, and this might be making visual noise.

@james2432
Copy link
Contributor Author

Aparently they are never tagged with churches/towers:
https://taginfo.openstreetmap.org/tags/amenity=clock#combinations

@kocio-pl
Copy link
Collaborator

I meant "on the wall of the churches/towers", not "the churches/towers which are considered to be clocks" - this wouldn't make sense for me.

@james2432
Copy link
Contributor Author

do you have an example of a clock that is near a church?

@kocio-pl
Copy link
Collaborator

No, I don't.

@james2432 james2432 force-pushed the amenity_clock branch 2 times, most recently from 0537d22 to d542cb7 Compare March 24, 2018 23:59
@Tomasz-W
Copy link

I'm considering which tag should have priority, but in cases which I can imagine at the moment:
-amenity=clock > man_made=tower
-amenity=clock < man_made=tower + tower:type=bell_tower

@kocio-pl
Copy link
Collaborator

I would exclude at least support=wall ("The feature is supported by a wall.") and support=wall_mounted ("The feature is mounted on a wall."). Feel free to suggest something more, which indicates it's not a standalone clock or just a part of something more important (see https://wiki.openstreetmap.org/wiki/Key:support ). The example:

https://www.openstreetmap.org/node/2470273778

@HolgerJeromin
Copy link
Contributor

I would exclude at least support=wall

This could lead to dropping this tag to get it rendered....

@Tomasz-W
Copy link

What about amenity=clock + advertising=column ? Which tag is more important in your opinion?

@kocio-pl
Copy link
Collaborator

@Tomasz-W The problem of objects with multiple tags is hard, basically I just rely on current code. Otherwise we should have complicated tools for chopping and moving some queries.

@HolgerJeromin Cheating is always possible, but there should be a way for a proper tagging to not make a visual mess in typical cases.

@kocio-pl
Copy link
Collaborator

kocio-pl commented Apr 4, 2018

@james2432 How are your plans about updating this code?

@james2432
Copy link
Contributor Author

Proof of exclusion for support = wall and support=wall_mounted
https://www.openstreetmap.org/way/458324854 (way, support=wall_mounted) (z19 ... z20 was also tested)
image

https://www.openstreetmap.org/node/5507892508 (node, support=wall_mounted) (z19 ... z20 was also tested)
image

https://www.openstreetmap.org/way/442511850 (way, support=wall) (z19 ... z20 was also tested)
image

https://www.openstreetmap.org/node/4294148104 (node, support=wall) (z19 ... z20 was also tested)
image

Also added the tags-> in parenthesis when using IS

project.mml Outdated
@@ -1519,6 +1519,8 @@ Layer:
tags->'tower:type' as "tower:type",
tags->'castle_type' as castle_type,
tags->'information' as information,
tags->'visibility' as "visibility",
tags->'support' as support,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why "visibility" is in quotes and support is not?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

purly esthetical, will add the quotes to follow others

project.mml Outdated Show resolved Hide resolved
project.mml Outdated
@@ -2343,6 +2349,7 @@ Layer:
WHERE building IS NOT NULL
AND building NOT IN ('no')
AND name IS NOT NULL
AND NOT tags->'tower:type' IN('bell_tower')
Copy link
Collaborator

Choose a reason for hiding this comment

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

Lacking space.

@james2432
Copy link
Contributor Author

https://github.com/james2432/openstreetmap-carto/blob/d0d9723662e8baaa5662e6091f172c7cea4dd8dd/project.mml#L2295

See purely esthetical, the other tags/column names are not enclosed with quotes here.

Added space.

Is there a coding standard that says all column names must be enclosed in quotes?

project.mml Outdated
@@ -2343,6 +2349,7 @@ Layer:
WHERE building IS NOT NULL
AND building NOT IN ('no')
AND name IS NOT NULL
AND NOT tags->'tower:type' IN ('bell_tower')
Copy link
Contributor

Choose a reason for hiding this comment

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

Better NOT IN, as above with building NOT IN ('no').

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed

@kocio-pl
Copy link
Collaborator

kocio-pl commented Jul 3, 2018

I might have few days off without the access to the computer, so I will look at it soon after I'm back.

@kocio-pl kocio-pl closed this Jul 3, 2018
@kocio-pl
Copy link
Collaborator

kocio-pl commented Jul 3, 2018

Sorry, wrong button...

@kocio-pl kocio-pl reopened this Jul 3, 2018
@kocio-pl
Copy link
Collaborator

This code is strange, it has two sections:

[feature = 'amenity_clock']["tower:type" != 'bell_tower'][feature != 'advertising_column'][zoom >= 18]
[feature = 'amenity_clock']["tower:type" != 'bell_tower'][feature != 'advertising_column']['support' != 'wall']['support' != 'wall_mounted'][zoom >= 19] { 

Wall clocks should never be shown, because they are just a part of tower (unlike clock towers with multiple faces for example), not the landmarks in themselves.

@Tomasz-W Tomasz-W mentioned this pull request Jul 11, 2018
26 tasks
@Tomasz-W
Copy link

@james2432 What's the state of this PR? Are you willing to finish it?

@james2432
Copy link
Contributor Author

well my laptop just died completely(motherboard), so I will be unable to continue this until I get a new one.

@Tomasz-W
Copy link

Oh, I understand. Anyway, I still hope that someday we will have these clocks and craft objects on map :)

@Adamant36
Copy link
Contributor

@kocio-pl, if I want to finish this do I do it by opening a new PR and noting that it closes this one along with the original issue or is there a certain procedure for that?

@kocio-pl
Copy link
Collaborator

New PR with closing comment will be OK.

@james2432
Copy link
Contributor Author

you can create a PR on my branch I will merge it, ai'm just unable to create new changes since I dont have a computer to work on it(using phone)

@Adamant36
Copy link
Contributor

Adamant36 commented Sep 16, 2018

OK cool. Creating a new PR here instead of on a branch might make it easier to deal with any subsequent issues that could come up.

@james2432, all that needs doing on it at this point is fixing rendering on walls?

@Adamant36
Copy link
Contributor

@james2432, I copied everything except the wall thing from your commit and although Kosmtik ran, I couldn't get it to display clocks on the map. If there anyway you could go over my code and see what I might be keeping them from displaying? https://github.com/Adamant36/openstreetmap-carto/tree/clock

@Tomasz-W
Copy link

@Adamant36 Oh, one more thing. Please change icon to "v1" version as it's obviously more clear than a "v2"

https://gist.github.com/Tomasz-W/07ed36017474669db82b11cb7f64dd26

@matkoniecz
Copy link
Contributor

I am really, really sorry but I think that showing all clocks is not a good idea (in other words I think that underlying #3070 should be rejected).

@Adamant36
Copy link
Contributor

I thought it was a little wierd myself, but it seemed like it had some support at the time.

Its generally easy to go back to an issue after some time has passed, or the disscussion has died down, and to close it because it doesn't make sense in retrospect. You might not get any complaints either. Since everyone has pretty much left the building at this point. It still might be worth leaving open due to the earlier support though.

I was still planning on working on it eventual if james2434 never came back.

What specific issue do you have with it? Maybe some clocks could be displayed instead of all clocks or something.

@james2432 james2432 closed this Dec 17, 2018
@Tomasz-W
Copy link

Certain clocks are described by tags like e.g. support=* or visibility=* . I believe that by some rendering conditions we could filter them and add to map worth ones like sun clocks functioning as tourist attactions or big clocks functioning as orientation points etc. I hope @Adamant36 will prepare test renderings someday to show that it won't mess the map, but make it richer.

@Adamant36
Copy link
Contributor

@Tomasz-W if you want to open a new issue for the display of prominent ones as @matkoniecz suggests in the original issue I'd be willing to work on it when I get some time if I can. That would at least allow for some discussion of the pros and cons in the meantime though.

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.