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

allow several presets to be mapped as vertices #1233

Merged
merged 1 commit into from
Jul 10, 2024

Conversation

k-yle
Copy link
Collaborator

@k-yle k-yle commented May 25, 2024

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 and Subway Station are incorrectly labelled as Train Station when mapped as a vertex, since the train station preset allows verticies, unlike these two presets.

Copy link

🍱 You can preview the tagging presets of this pull request here.

Copy link
Collaborator

@tordans tordans left a 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",
Copy link
Collaborator

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.

Copy link
Collaborator Author

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

data/presets/natural/sinkhole.json Show resolved Hide resolved
data/presets/marker.json Show resolved Hide resolved
data/presets/man_made/mineshaft.json Show resolved Hide resolved
data/presets/man_made/fuel_pump.json Show resolved Hide resolved
data/presets/amenity/ticket_validator.json Show resolved Hide resolved
@k-yle
Copy link
Collaborator Author

k-yle commented Jul 9, 2024

I removed a few that are definitely not 'clear-cut', kept the inconsistency issues (for now)

@tordans
Copy link
Collaborator

tordans commented Jul 10, 2024

Thanks for the video, that was very convincing :-)

@tordans tordans merged commit b36e802 into openstreetmap:main Jul 10, 2024
5 checks passed
@tordans tordans added the new-label changes the name, aliases or terms of a preset label Jul 10, 2024
@k-yle k-yle deleted the kh/vertex branch July 10, 2024 21:27
@tordans
Copy link
Collaborator

tordans commented Aug 9, 2024 via email

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
new-label changes the name, aliases or terms of a preset
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants