-
Notifications
You must be signed in to change notification settings - Fork 160
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
allow several presets to be mapped as vertices #1233
Conversation
🍱 You can preview the tagging presets of this pull request here. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I looked into those features and added my notes as inline comments. I think we should be a bit hesitant to broaden the geometry because it will be harder to remove those change later (because we will promote usage and thereby increase usage; and because its harder to take something away than add…).
I know you had your use cases for those changes. Please take my comments as nudge to double check them. Or to add a bit more of context so we alter know why we did it :).
@@ -8,6 +8,7 @@ | |||
], | |||
"geometry": [ | |||
"point", | |||
"vertex", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
IDK…
My -1
: The wiki sounds like we want those to be bigger areas. And there is presets/public_transport/platform/_light_rail_point.json
which sounds more like the thing one wants to glue to a railway way?
My +1
: There are other presets that have the vertex already. Why is this so inconsistent…?
- station_aerialway.json has it
- station_bus.json doesn't
- station_ferry.json has it
- station_light_rail.json doesn't (yet)
- station_monorail.json doesn't
- station_subway.json doesn't
- station_train.json has it
- station_tram.json doesn't
- station_trolleybus.json doesn't
- station.json doesn't
I did not do any analysis how this is used because I lack the tooling to do this quickly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason I proposed this change was less about the tagging style and more about the inconsistency:
Screen.Recording.2024-07-09.215053.mp4
If you drag a Light Rail Station
onto a way, then the preset degenerates down to Train Station
, which is pretty confusing. So ideally, all these presets would be allowed as verticies, or none of them.
I did not do any analysis how this is used because I lack the tooling to do this quickly.
Here's an overpass query to compare points vs vertices. There might be a more efficient query, but I'm not an overpass expert
I removed a few that are definitely not 'clear-cut', kept the inconsistency issues (for now) |
Thanks for the video, that was very convincing :-) |
That is an interesting case. However I would consider the
https://wiki.openstreetmap.org/wiki/Tag:waterway=soakhole tag to be the
primary tag to describe this feature as the water part makes it
something else. We might want to add a preset for those and add
vertex there. Having the sinkhole as vertix and potentially part of a way
feels too unintuitive and we should not promote it IMO.
Am Do., 8. Aug. 2024 um 17:15 Uhr schrieb Martin Raifer <
***@***.***>:
… ***@***.**** commented on this pull request.
------------------------------
In data/presets/natural/sinkhole.json
<#1233 (comment)>
:
> "point",
+ "vertex",
"area"
For reference: these are sometimes mapped as part of waterway. Here's an
example: https://www.openstreetmap.org/node/1060282427
—
Reply to this email directly, view it on GitHub
<#1233 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAA3HSKPZS76LY7V7ISSE7LZQODPJAVCNFSM6AAAAABIIU4ZTKVHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZDEMRYGE4TCNZQG4>
.
You are receiving this because you modified the open/close state.Message
ID: ***@***.***
com>
|
When micromapping features, it's common for these features to be part of a
wall
, or the edge of a building.iD currently creates an annoying warning when these feature are mapped as vertices, despite the wiki pages not mentioning anything about point vs vertex.
This PR also fixes a bug. Currently
Light Rail Station
andSubway Station
are incorrectly labelled asTrain Station
when mapped as a vertex, since the train station preset allows verticies, unlike these two presets.