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

Adds presets for Traffic Sign, Speed Limit Sign, and City Limit Sign #5333

Merged
merged 2 commits into from
Sep 28, 2018

Conversation

quincylvania
Copy link
Collaborator

@quincylvania quincylvania commented Sep 27, 2018

Closes #5331.

Each preset has a two variants—point and vertex—to account for different direction fields. The vertex variant uses the direction_vertex field and the point variant uses the basic direction field.

This also includes a small code change to make the maxspeed field compatible with node entities. This lets the Speed Limit Sign use the maxspeed field.

… each with point/vertex variants to account for different direction fields

Makes the maxspeed field compatible with node entities
@bhousel
Copy link
Member

bhousel commented Sep 27, 2018

Hey @quincylvania - thanks again for all your work on the presets recently 🙇

I think it's great that these presets support both street signs either mapped as their own point alongside the way or as a vertex along a way.

When reviewing this, I noticed an issue with the directional tagging of street signs. This is not actually an issue in what you did, but more an oddity in the osm-wiki-recommended way of tagging street sign directions.

For reference...

https://wiki.openstreetmap.org/wiki/Key:traffic_sign#As_a_separate_node

Create a separate node beside the road at the position of the actual sign... You can use the direction=* tag to describe the facing orientation of the sign by using an angle or cardinal direction.

Cool! iD supports this, and it's what you did...

https://wiki.openstreetmap.org/wiki/Key:traffic_sign#As_part_of_a_way

Create a new node within the relevant way next to the sign... You can use traffic_sign:forward=* to specify that this sign affects vehicles moving in the same direction as the OSM way. The opposite direction can be tagged with traffic_sign:backward=*. Formerly the direction=* key has also been used to describe the affected direction. But its common meaning has changed to Relative to Geographic North.

Whoa! So that's a problem. Your presets actually use direction=forward/backward here, which iD supports for other things like stop and give_way signs, and is the normal traditional way of doing things.

But the wiki says we are supposed to do something like traffic_sign:forward=US:R1, and we can't really do that. A preset needs to be based on a "toplevel" tag like traffic_sign=* not traffic_sign:forward=* or traffic_sign:backward=* (remember seamark? some of their tags have this issue too).

So my recommendation is for you to change the sign-as-vertex presets so that traffic sign direction tagging works exactly like traffic signal direction tagging.

From: https://wiki.openstreetmap.org/wiki/Key:traffic_signals:direction
iD already has
data/presets/fields/traffic_signals/direction.json to support the tag traffic_signals:direction=forward/backward

...and I'm proposing to create:
data/presets/fields/traffic_sign/direction.json to support the tag traffic_sign:direction=forward/backward

I will draft a message to the tagging mailing list today to let people know about the issue and that iD is going a different "direction" from what the wiki recommends (hah).

Replaces the direction_vertex field in the vertex Traffic Sign presets with the traffic_sign/direction field
@quincylvania
Copy link
Collaborator Author

Hi @bhousel, thanks for the feedback!

I hadn't realized the direction issue beforehand. Your solution sounds good to me; I have made the changes in the latest commit. Conveniently, iD already renders view cones for any tag with the :direction suffix.

screen shot 2018-09-27 at 5 01 07 pm

Let me know when we're clear to merge!

@bhousel
Copy link
Member

bhousel commented Sep 28, 2018

I hadn't realized the direction issue beforehand. Your solution sounds good to me; I have made the changes in the latest commit. Conveniently, iD already renders view cones for any tag with the :direction suffix.

Awesome thanks for the quick fix @quincylvania 👍

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.

2 participants