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

Sidewalk quest don't show up for majority of roads, since the maxspeed quest is not enabled (US) #2448

Closed
RubenKelevra opened this issue Jan 3, 2021 · 15 comments
Labels
feedback required more info is needed, issue will be likely closed if it is not provided

Comments

@RubenKelevra
Copy link
Contributor

How to Reproduce
We currently have a filter when to show the sidewalk quest, based on the tagged speed.

This conflicts with the not enabled max speed quest for the US, resulting in no sidewalk quests showing up at all - since many streets are not yet tagged with a max speed.

Something similar is true for the lit-quest, which also doesn't show up.

Do we really need to assume, that there's no sidewalk when there's no maxspeed tag? I thought we just want to filter out ways with very low maxspeed.

Versions affected
v29-beta1

@matkoniecz
Copy link
Member

matkoniecz commented Jan 3, 2021

This conflicts with the not enabled max speed quest for the US, resulting in no sidewalk quests showing up at all - since many streets are not yet tagged with a max speed.

Sidewalk quest has filtering out maxspeed < 8, maxspeed =5 mph, maxspeed=walk.

Ways without maxspeed are allowed.

So "resulting in no sidewalk quests showing up at all - since many streets are not yet tagged with a max speed." is not correct.

I thought we just want to filter out ways with very low maxspeed.

That is what happens.
https://github.com/streetcomplete/StreetComplete/blob/master/app/src/main/java/de/westnordost/streetcomplete/quests/sidewalk/AddSidewalk.kt#L27

resulting in no sidewalk quests showing up at all

Please, give link to an affected location. I am betting that sidewalk is tagged or that it is filtered out due to separately mapped footways or more important quests are shown and hiding it. Try disabling all quests except sidewalk one.

@matkoniecz matkoniecz added feedback required more info is needed, issue will be likely closed if it is not provided and removed bug labels Jan 3, 2021
@westnordost
Copy link
Member

That is what happens.
https://github.com/streetcomplete/StreetComplete/blob/master/app/src/main/java/de/westnordost/streetcomplete/quests/sidewalk/AddSidewalk.kt#L27

Hm, looking at the filter now, it is pretty useless. Roads with maxspeed=5 will not be filtered out.

@westnordost
Copy link
Member

westnordost commented Jan 3, 2021

and (
  !maxspeed
  or maxspeed > 8
  or (maxspeed ~ ".*mph" and maxspeed !~ "[1-5] mph")
)

Does this make sense?

@matkoniecz
Copy link
Member

I will also filter RO:urban, RU:urban and similar (see https://taginfo.openstreetmap.org/keys/maxspeed#values )

@westnordost
Copy link
Member

Hmmm. Wonder if this isn't overding it a little...

or source:maxspeed and source:maxspeed !~ .+zone:?([1-9]|[1-2][0-9]|30)
or maxspeed:type and maxspeed:type !~ .+zone:?([1-9]|[1-2][0-9]|30)
or zone:maxspeed and zone:maxspeed !~ .+zone:?([1-9]|[1-2][0-9]|30)
or maxspeed and maxspeed !~ .+zone:?([1-9]|[1-2][0-9]|30)

@westnordost
Copy link
Member

@matkoniecz Look at the commits I've done just now. You alright with this?

@RubenKelevra
Copy link
Contributor Author

Which feedback is required? :)

@matkoniecz
Copy link
Member

Please, give link to an affected location. I am betting that sidewalk is tagged or that it is filtered out due to separately mapped footways or more important quests are shown and hiding it. Try disabling all quests except sidewalk one.

As your stated reason ("conflicts with the not enabled max speed quest for the US") does not really work

@tapetis
Copy link
Contributor

tapetis commented Jan 8, 2021

The changes in commit 7e1f7da lead to the AddCycleway quest being shown in "Tempo-30-Zonen" in Germany. These residential highways are often tagged as maxspeed=30 and maxspeed:source=DE:zone30 (StreetComplete tags these streets in the same way). The NOT_IN_30_ZONE_OR_LESS filter should stop this quest from appearing for such highways, but because the maxspeed key is included in the MAXSPEED_TYPE_KEYS list, the maxspeed=30 leads to one entry in the series of logical disjunctions being true. This is because the key exists and the regular expression does not match the value (it does not include the word "zone").

private val NOT_IN_30_ZONE_OR_LESS = MAXSPEED_TYPE_KEYS.joinToString(" or ") {
"""$it and $it !~ ".*zone:?([1-9]|[1-2][0-9]|30)""""
}

val MAXSPEED_TYPE_KEYS = listOf(
"source:maxspeed",
"zone:maxspeed",
"maxspeed:type",
"zone:traffic",
"maxspeed"
)

or highway = residential and (
maxspeed > 30
or (maxspeed ~ ".*mph" and maxspeed !~ "([1-9]|1[0-9]|20) mph")
or $NOT_IN_30_ZONE_OR_LESS
)

@westnordost
Copy link
Member

Uff, damn, you are right. Would you like to create a PR for a fix? I think your excellent detective work in the code should also lead to that your work is credited in the changelog. (Also, I am still pondering how this can be fixed...)

@tapetis
Copy link
Contributor

tapetis commented Jan 8, 2021

I am also not sure how to fix this.

However, I want to additionally note that the used regular expression also matches values like DE:zone:40 or DE:zone:1000, because the first alternative from the capturing group already matches the numbers 1–9 and thus the following digits do not matter.

@westnordost
Copy link
Member

@westnordost
Copy link
Member

westnordost commented Jan 8, 2021

Ah, misunderstanding. Overpass uses the regex operator "~" for "find in". In StreetComplete, the regex operator "~" means "matches entire string"

@tapetis
Copy link
Contributor

tapetis commented Jan 8, 2021

Yes, you are correct. I did not know that StreetComplete internally uses Kotlin's matches function, which implicitly adds a ^ to the start and a $ to the end of the pattern.

@westnordost
Copy link
Member

Hm, I see no other way to easily solve this than to remove maxspeed there again. Sorry, Romania, Russia :-|

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback required more info is needed, issue will be likely closed if it is not provided
Projects
None yet
Development

No branches or pull requests

4 participants