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

Look for kerbs where crossing meets traffic island #3786

Merged

Conversation

arrival-spring
Copy link
Contributor

Not sure how expensive the checks are, in terms of CPU etc., so not sure if it's worth it given that footway=traffic_island is fairly rare.

Tested and working

Happy to close if not wanted.

@matkoniecz
Copy link
Member

It is kind of barely used, 1500 uses worldwide ( https://taginfo.openstreetmap.org/keys/footway#values ).

screen

If added, also tests should be added to confirm that it works well.

I admit that I am not really motivated to analyze that code again given its complexity and how rare this tag is.

@westnordost
Copy link
Member

westnordost commented Feb 21, 2022

Looks ok. The performance should not be any issue here. However, it is necessary to add test case(s) for this. I'd suggest...

aaa = footway=crossing
bbb = footway=traffic_island
cccc = footway=sidewalk

1. aaaxbbb -> yields a kerb at x
2. bbbxccc -> yields no kerb at x
3. bbbxaaa -> yields no kerb at x
      c
      c
      c

I share @matkoniecz notion that it is barely used, so barely worth the effort. There is also the danger that we start supporting a tag that is not blessed by community consent - is this documented somewhere, was it discussed anywhere? It could always be that after such a discussion, consent is that this tag should not be used for one reason or the other and rather a certain replacement should be used.

@matkoniecz
Copy link
Member

It is documented at https://wiki.openstreetmap.org/wiki/Tag%3Afootway%3Dtraffic_island and appears to make sense to me and I see no complaints at Wiki about it.

Also, SC would not be actually adding it.

But... 1 504 uses worldwide...

@arrival-spring
Copy link
Contributor Author

Let's close it then

@gdabski
Copy link

gdabski commented Jan 26, 2023

I have user locally who insists on using this tag; result is missing info on kerbs, which mostly come from SC quests around here.

@westnordost
Copy link
Member

Hm well, it's up to 6 493 usages now since one year ago and it is documented. Looks like this can be revisited / implemented.

@westnordost westnordost reopened this Jan 26, 2023
@westnordost
Copy link
Member

The one thing missing though is test cases.

@arrival-spring
Copy link
Contributor Author

I'll have a look at adding some tests soon

@arrival-spring
Copy link
Contributor Author

Done, quite easy as there are already similar tests for sidewalk/crossing

All the tests pass and I've also manually tested that the quest also now shows up as expected

@westnordost
Copy link
Member

Great, thank you!

@westnordost westnordost merged commit 28201b1 into streetcomplete:master Jan 27, 2023
@arrival-spring arrival-spring deleted the traffic_island_kerbs branch May 6, 2023 09:18
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.

None yet

4 participants