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

Replace osm2lanes algorithm with muv_osm #233

Merged
merged 28 commits into from
Mar 13, 2024
Merged

Conversation

ginnyTheCat
Copy link
Contributor

@ginnyTheCat ginnyTheCat commented Feb 25, 2024

Start replacing the osm2lanes logic with muv_osm as planned in #232. There is still quite some work to do, both on the muv side and on this translation layer. Which osm2lanes test pass should give a good indication for now.

This will replace the currently quite messy demo currently live on https://muv.lelux.net/osm2streets-demo, while this branch is live on https://muv.lelux.net/osm2streets.

As this branch currently stands it resolves #231, #230, #222, #89 (only winds up as construction lane as a last resort; new LaneType?)

Copy link
Contributor

@dabreegster dabreegster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is really cool! I've read through the code and it makes cursory sense to me. I still don't have much time to dig into this, but please let me know what I can do to help.

osm2lanes/Cargo.toml Outdated Show resolved Hide resolved
osm2lanes/src/placement.rs Show resolved Hide resolved
osm2lanes/src/lib.rs Show resolved Hide resolved
@ginnyTheCat
Copy link
Contributor Author

We are now down to 3 failing osm2lanes tests.

failures:

---- tests::test_osm_to_specs stdout ----
For input (example from https://www.openstreetmap.org/way/8591383):
    lanes=1
    oneway=yes
    sidewalk=both
    cycleway:left=track
    oneway:bicycle=no
    highway=residential
Got:
    sbds
    v^^^
Expected:
    sbbds
    vv^^^

For input (example from https://www.openstreetmap.org/way/369623526):
    lanes=1
    oneway=yes
    sidewalk=both
    parking:lane:right=diagonal
    cycleway:left=opposite_track
    oneway:bicycle=no
    highway=residential
Got:
    sbdps
    v^^^^
Expected:
    sbbdps
    vv^^^^

For input (example from https://www.openstreetmap.org/way/539534598):
    highway=cycleway
    oneway=no
    segregated=no
Got:
    FF
    ^v
Expected:
    F
    ^

thread 'tests::test_osm_to_specs' panicked at osm2lanes/src/tests.rs:304:5:
3/25 spec tests failing

The first two are because of oneway:bicycle not being applied to cycleways as well. I saw the OSM links but am not sure if this is common or documented somewhere, as it doesn't seem to be quite common in my region. Maybe you have some information about those cases? That would be quite helpful.

@dabreegster
Copy link
Contributor

For those first two cases, https://wiki.openstreetmap.org/wiki/Tag:cycleway:left=track?uselang=en says cycleway:{left,right}=track implies riding in both directions by default. So even ignoring oneway:bicycle=no, shouldn't there be two cycle lanes next to each other in opposite directions? (in the osm2streets interpretation, where there isn't Direction = Both yet)

@ginnyTheCat
Copy link
Contributor Author

ginnyTheCat commented Mar 6, 2024

Oh, wow. I've never seen that. https://wiki.openstreetmap.org/wiki/Tag:cycleway%3Dtrack says cycleway=track is oneway by default. Also in reality I've never seen cycleway: (except highway=cycleway) being assumed oneway=no. I'm not sure how I should handle this now 😅.

Edit: cycleway=lane is oneway by default as well, which would make cycleway=track behaving differently quite confusing.

Another edit: https://wiki.openstreetmap.org/wiki/Tag:cycleway:right%3Dtrack has the same photo as the cycleway:left=track page, with the tags below containing cycleway:right:oneway=no. I'm suspecting the cycleway:left=track page is wrong/out of date.

@dabreegster
Copy link
Contributor

I'm also confused the more I read the two wiki pages. >_< Well, uh, this is exactly the kind of complexity that it's lovely for Muv to hide from downstream ;)

If I had to try and rationalize this, cycleway=track implies a cycleway on both sides of the road. Except in Belgium, I've never personally seen that situation being bidirectional on both sides -- there's usually not enough space. So defaulting in that case to one-way for each side makes sense. If someone has explicitly mapped cycleway:left and :right, then... actually, no, it's still strange for the default to assume bidirectional. I don't know enough about the tag history or proposal to comment.

@ginnyTheCat
Copy link
Contributor Author

Yea, I'm gonna leave it as oneway default. Explicit oneways are handled and in case we find any region where this is common (you mentioned Belgium) it's trivial to add a different default for that in muv (should a one line change).

Still I'm unsure whether oneway:bicycle=no on the main road should imply cycleway:oneway=no.

@tordans
Copy link

tordans commented Mar 7, 2024

A few quick notes because I am late to the thread :)

  • IMO the cycleway:side=track + oneway:bicycle=no is very missleading mapping. IMO the oneway:bicycle=no applies to the centerline, which would be wrong in the given example.
    image
  • IMO the right tagging for that case would be cycleway:side=track + cycleway:side:oneway=no, so the oneway is scoped.
  • For our work at Radverkehrsatlas.de (https://github.com/FixMyBerlin/atlas-geo/blob/develop/app/process/roads_bikelanes/bikelanes/InferOneway.lua) we assume that track is oneway=no and lane is oneway=yes which is how it is used in Germany except for so called "Hochbord Radwege" which usually have an explicit oneway=no-Tag. — When researching that I did not find any good documentation on assumed defaults either.
  • The segregated=no case is interesting. So in fact there are two lanes, but those lanes are not visible in any infrastructure. I tend to think that should still create the two lanes however without any indicator on separation.

@ginnyTheCat
Copy link
Contributor Author

The track situation is awfully confusing as while the cycleway page doesn't say anything about the topic, cyclway:left=track (implies oneway=no) is completly different from cyclway=track and cycleway:right=track (implies oneway).

I built a small visualization in overpass turbo, that really shows how inconsistent the coverage is due to this documentation nightmare. Cycletracks (not highway=cycleway, just cycleway:*=track) without a oneway value are black, explicitly tagged oneway=no blue and explicitly tagged oneway cycletracks green.
imagen
imagen
You can see how the oneway cycletracks and cycleways without oneway tagged are switching between each other which would make me think oneway is the default.

@dabreegster
Copy link
Contributor

If the tagging schema / docs themselves are so confusing and there's clear evidence of it being inconsistently mapped in practice, then how about:

  1. We raise this on mailing lists / Slack / the wiki discussion page, to make the schema more clear
  2. muv implements the simplest / most sane default assumption right now
  3. We either delete these osm2streets test cases as ambiguous, or fix how they're tagged upstream in OSM and update the tests.

I don't think figuring this out has to block this PR, because it gives so many other benefits. If there is a clear interpretation, the updated street explorer can help people find and fix these confusingly mapped cases

@ginnyTheCat
Copy link
Contributor Author

We raise this on mailing lists / Slack / the wiki discussion page, to make the schema more clear

I think that's a great idea

muv implements the simplest / most sane default assumption right now

From a simplicity to implement perspective they are both equal, but I feel like assuming oneway=no breaks less cases I think. Once we know the final interpretation muv_osm would adopt this and help people find the mistaggings as you said.

@ginnyTheCat ginnyTheCat marked this pull request as ready for review March 7, 2024 21:07
@tordans
Copy link

tordans commented Mar 8, 2024

We raise this on mailing lists / Slack / the wiki discussion page, to make the schema more clear

I think that's a great idea

I don't want to discourage you but pushing this through all the channels until the situations is actually better is a big pile of work. My take on the steps would be…

  1. gather some input on the current situation via a forum post with some evidence like shown above (it is likely, that there are local differences in how the non documented defaults are perceived)
  2. write a proposal draft that makes the case for the issue and the solution (see is_sidepath proposal); that includes possible deprecations, change to wiki pages, maybe even recommendations for editors
  3. discuss this proposal in forum and so on – takes a while to "simmer"
  4. bring the proposal to a voting
  5. use that agreement to go to the id-editor-schema-repo and add deprecations, new fields, new presets so the iD editor nudes people to do the right thing in the future
  6. try doing the same for JOSM presets
  7. probably start a MapRoulette Project to cleanup the existing mess and gather support to work through that

In very easy cases, only step 1 + updates to the wiki pages is possible.

We went through this with the parking tagging (except 7). It is possible, but a lot of very involved work.


What to do "instead"? I suggest to define your own sensible default, document that and go with it. As long it is clear how it works and can be changed in the tags if it's wrong and uses proper tags – which is all the case here – this will help guide the bigger community in the ride direction … over time.


Side note: I will likely try this process at some point to get rid of cycleway:null=track|lane|separate|none in favor of cycleway:both=track|lane|separate|none which is one of those historic issues that no one cleaned up but causes issues when processing and editing all the time…

@tordans
Copy link

tordans commented Mar 8, 2024

muv implements the simplest / most sane default assumption right now

I suggest to assume oneway=no for bicycles on cycleway:side=tracks and oneway=yes for bicycles on cycleway:side=lanes.

And in the example above, to apply the oneway:bicycle=no only to the centerline, which will create a second lane in MUV which would be a shared lane wich needs to be very narrow or overlapping. I consider this a tagging error that has to be fixed in OSM, not in the processing. But it is valid tagging eg. here https://www.openstreetmap.org/way/25509293 so it needs to be processed.


Update OMG: As always the current state of things is more messy. Reading https://wiki.openstreetmap.org/wiki/Key:oneway:bicycle where oneway:bicycle=no and https://wiki.openstreetmap.org/wiki/Talk:Key:oneway:bicycle where the tag is also discussed sound to me that somewhere sometime recently people considere(d) this tagging to be a good idea.

That is actually why we need stuff like this project to show them that their tagging is just not possible to interpret correctly.

@ginnyTheCat
Copy link
Contributor Author

Making that tagging consistent is definitely a long and hard journey. I will collect more information as I go along but probably won't start writing a draft for now.

We went through this with the parking tagging

The new street parking schema is amazing and i'm very thankful you went through with all those many steps.

But it is valid tagging eg. here

Exactly. This is one of the things that is an annoying side-effect of catering to mistagings. You have to choose mutually exclusively between handling the mistaging or parsing the valid situations the mistagings are appearing as. If you side with the mistaged version, the other version becomes unrepresentable.

which will create a second lane

That's how muv used to handle oneway:bicycle=no. Now it only creates one lane allowing cars in the forward, and bikes in both directions. The narrow second lane currently only appears when using something like cycleway=opposite_lane.

@ginnyTheCat
Copy link
Contributor Author

ginnyTheCat commented Mar 8, 2024

where the tag is also discussed sound to me that somewhere sometime recently people considere(d) this tagging to be a good idea.

I'm seeing the Talk page now as well. The messages seem to be from 2018. For me (and what I read in the wiki and encountered when mapping) cycleway=opposite_lane implies oneway:bicycle=yes (which is just a permission of access). Without cycleways being there it's just be a oneway that allows cycling in both directions (no infrastructure, only a sign allowing cycling in both directions).

@ginnyTheCat
Copy link
Contributor Author

I'm was confused with the first one and still am. Maybe I'm misunderstanding something.
imagen
This is what it looks like on my side. The ones with lanes=3 have 3 lanes, and the other ones 4.

@dabreegster
Copy link
Contributor

If I re-import the OSM data for seattle_triangle, it looks fine. Something must be strange with the older data in there. I'll reimport and update now.

Everything else looks fantastic; I think we're basically there! I noticed two small things, but they don't need to block merging:

@ginnyTheCat
Copy link
Contributor Author

I just noticed a small bug in the muv implementation of segregated.
imagen
The idea is that it's always appends the footway on the driving side (left with LHD, right with RHD) as the cycleway the segregated is on is likely a oneway following a road, so it doesn't sandwich the segregated footway between the cycleway and the footway.
This broke though for LHD and I just noticed because the small cycleway in the top is on the right. Now that it's fixed though
imagen
the footway on the bottom cycleway ofc also switches sides. With the segregated tag we can really only guess what side it belongs to. I think adding a sidewalk=left tag on the bottom cycleway is more practical than always appending segregated paths to the right even for LHD.

@dabreegster
Copy link
Contributor

I think adding a sidewalk=left tag on the bottom cycleway is more practical than always appending segregated paths to the right even for LHD.

Good catch, and yeah, this makes perfect sense. One of the things osm2streets eventually hopes to solve is figuring out the relationship between the separate cyclway/footwayand the main road, so that it could at least know if they're on the left or right, or reason about the polygon gap between them, or maybe consolidate/merge. If we get to that point, we could supply hints about the direction. But until then, the tagging is kind of ambiguous, and it's nice to see it so clearly, so people can try different tagging.

I think everything here is good to go! If we find more problems, we can just make more smaller changes. I'd like to add you to the project credits; do you want to be identified by your GH username, and do you want a link to something other than https://gitlab.com/LeLuxNet/Muv?

Copy link
Contributor

@dabreegster dabreegster left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Absolutely phenomenal work, I am so excited to cutover to this and resume effort on osm2streets

@ginnyTheCat
Copy link
Contributor Author

I think https://www.openstreetmap.org/way/668984461 should have a bus lane, but it looks like cycleway=share_busway maybe overrides it.

This is a odd one. It talks about 2 lanes, but then has a psv:lanes=designated at the end. As muv sees this value first, it disgards all the :lanes tags. I will handle this more gracefully in the future, but in most cases there isn't a lot we can do when the :lanes count mismatch.

But the main issue is that Motorvehicles are set to permit. I would either remove permitfrom the list of AccessLevels we deem as allowed, or make it rank lower than Yes. I'm not sure what makes more sense though.

looks like cycleway=share_busway maybe overrides it

cycleway=share_busway is still a TODO on the muv side and currently just added as a normal cycleway. In the future it would merge into the bus lane though.

I'm not sure how to interpret the "both sides" part

I think it makes sense to handle it the same way we did with the cycleways. Oneways with bus lanes on both sides could still tag bus_way:both=lane to get one on both sides.

we could supply hints about the direction

That's a super interesting idea as well and has a lot of potential.

I think everything here is good to go!

After that quick fix I think we're good to go.

do you want to be identified by your GH username

Yup, that's perfect

do you want a link to something other than https://gitlab.com/LeLuxNet/Muv?

No, I don't think so. Thanks a lot already.

@ginnyTheCat
Copy link
Contributor Author

I think everything here is good to go! If we find more problems, we can just make more smaller changes.

I agree. The translation layer is quite robust now so most bugs can just be fixed on the muv side with a simple cargo update muv-osm on the osm2streets side.

@dabreegster
Copy link
Contributor

Alright, merging this finally! Thanks so much for all of the work.

Probably Friday or so, I'll carve out time to update the Street Explorer UI, start exposing more Muv details, etc. If you're interested in reviewing the code or just keeping up with changes, I can describe the work in PRs.

I will handle this more gracefully in the future, but in most cases there isn't a lot we can do when the :lanes count mismatch.

In situations when the tagging is inconsistent or ambiguous, one idea is to collect a list of warnings. We can find an appropriate way to display them to the user (maybe in the lane editor).

@dabreegster dabreegster merged commit cd0fc2b into a-b-street:main Mar 13, 2024
@ginnyTheCat
Copy link
Contributor Author

If you're interested in reviewing the code or just keeping up with changes, I can describe the work in PRs.

Sounds great. I will definitely keep eyes open for that.

one idea is to collect a list of warnings

I saw the newer osm2lanes did that and I think that is a good idea.

@ginnyTheCat ginnyTheCat deleted the muv2 branch March 13, 2024 14:48
@tordans
Copy link

tordans commented Mar 16, 2024

@ginnyTheCat

I checked some cities with the visualization and aerial imagery and especially in Berlin most of the cycleway:{left,right,both}=track don't have a cycleway:{left,right,both}:oneway associated with them. From what I could see in the satellite view it looks like most of those cycleways are oneway though. Especially the dual carriageway below would have two-directional cycleways on both sides of the road then.
Maybe I'm misunderstanding something about the default but most two-way cycleways I've seen, were ususally mapped as a seperate highway=cycleway next to the main road.

"Even in Berlin" (where we have a quite active OSM + Cycling Community we have a lot of unfinished tagging to cleanup. I consider those cases where track is mapped on the centerline without a oneway incomplete. But it's just so much work to get good defaults and recommendations inside editors and to create QA tools that get used… (We are working on that with https://radverkehrsatlas.de/ in some parts).
My general take is, to move track to separate highway=path or highway=cycleway which makes it so much easier to map the needed details like oneway.

So my take is: It is OK for osm2streets to show the tagging mistake so mappers start fixing them…

(Sorry for the late reply…)

@ginnyTheCat
Copy link
Contributor Author

"Even in Berlin"

In most cities the visualization I built shows around 70% of cycleways not having any oneway tag (neither yes nor no) in most cities, while in Berlin it's more like 90%. From my memories of Berlin and most cities in Europe highway=cycleway are mostly just cycleways on sidewalks and therefore oneway by default. The bidirections cycleways common in North America are in most cases tagged as a highway=cycleway as they are more often seperated by poles, barriers, parking etc.

@matkoniecz
Copy link

matkoniecz commented Mar 28, 2024

I noticed it in openstreetmap/id-tagging-schema#1179

I would say that cycleway:left/cycleway:both/cycleway:right/cycleway =track is a format of fixme and should be replaced by properly mapped geometry with cycleway:left/cycleway:both/cycleway:right =separate on the main road.

But highway=tertiary cycleway=track oneway=yes oneway:bicycle=no may be used to map as one object oneway road with two-way cycleway along it

@tordans
Copy link

tordans commented Mar 28, 2024

But highway=tertiary cycleway=track oneway=yes oneway:bicycle=no may be used to map as one object oneway road with two-way cycleway along it

That tagging is so imprecise that we have to make so many assumptions …

  • assume that cycleway=track is actually cycleway:both=track
  • assumes that oneway:bicycle=no in combination with cw=track does not mean cycling is allowed in both directions where the vehicles drive … but instead applies to the track
  • and then we should assume that the track on both sides is free to be used in both directions, yes?

Is this how you would interpret it, @matkoniecz ?

style  left center right
centerline mapping - highway=tertiary cycleway=track oneway=yes oneway:bicycle=no -
separate mapping highway=cycleway/path oneway=no highway=tertiary cycleway:both=separate oneway=yes highway=cycleway/path oneway=no

Of course, there is also a way to say…

  • cycleway=track is only applied to the main direction of the road, so in most countries it should be interpreted as cycleway:right=track

In this case, the oneway:bicycle=no makes a little more sense. But still, the parser needs to behave differently for streets that are just highway=* + oneway=yes + oneway:bicycle=no (+ optional cycleway(:both)=no) (which is the only way to allow cycling is allowed in both directions (but vehicles are not) (example))

style  left center right
centerline mapping - highway=tertiary cycleway=track oneway=yes oneway:bicycle=no -
separate mapping - highway=tertiary cycleway:left=no cycleway:right=separate oneway=yes highway=cycleway/path oneway=no

@matkoniecz
Copy link

matkoniecz commented Mar 28, 2024

Is this how you would interpret it, @matkoniecz ?

I would also consider fixme=replace it by properly mapped cycleway as separate geometry, and replace it by cycleway:*=separate to be an implied tag.

I would not assume that it is cycleway:both=track - I would assume that it is two-way track on one, unspecified, side. At least in Poland, in Germany two oneway tracks may be more likely.

I have (still unfinished) project of visualising bicycle infrastructure and =track was going solely to QA section as serious mapping quality issue, with no attempt to guess what people meant by it.

@tordans
Copy link

tordans commented Mar 28, 2024

@matkoniecz glad we are on the same page :-).

And to pick up the (deleted) conversation from before, that is also why it was talking about scoping the oneway to the virtual cycleway like so…

… and which is why I consider the wiki very non helpful with this oneway tagging.

style  left center right
current - highway=tertiary oneway=yes cycleway=track oneway:bicycle=no -
more clear - highway=tertiary oneway=yes cycleway:left=track cycleway:left:oneway=no -

I have (still unfinished) project of visualising bicycle infrastructure and =track was going solely to QA section as serious mapping quality issue, with no attempt to guess what people meant by it.

Lets talk about https://github.com/FixMyBerlin/atlas-geo processing at some point. It would be great to see someone from non-germany pick this up and see if the processing does fit the local community styles. In a few month we will have proper docs for the category that we create and some time this year I want to create a German-wide map with some Maproulette projects to improve the tagging to make the processing work.

@matkoniecz
Copy link

… and which is why I consider the wiki very non helpful with this oneway tagging.

Well, wiki describes tagging as-is

if new better tagging is needed then tagging mailing list discussion/proposal process is needed, not only just editing wiki page

(and for new style I would keep also oneway:bicycle=no and have this new extra tagging as optional, for typical use such as routing this detail is not needed at all)

cycleway:left=track

If someone cares about high level of detail and clarity I would focus on getting rid of any track values and replace them by separately mapped geometries and (in this case) cycleway:left=separate

@matkoniecz
Copy link

Lets talk about https://github.com/FixMyBerlin/atlas-geo processing at some point. It would be great to see someone from non-germany pick this up and see if the processing does fit the local community styles. In a few month we will have proper docs for the category that we create and some time this year I want to create a German-wide map with some Maproulette projects to improve the tagging to make the processing work.

Is it by any chance serving tiles/data for Kraków, Poland? Or having some working setup step-by-step instructions?

@tordans
Copy link

tordans commented Mar 28, 2024

Lets talk about https://github.com/FixMyBerlin/atlas-geo processing at some point. It would be great to see someone from non-germany pick this up and see if the processing does fit the local community styles. In a few month we will have proper docs for the category that we create and some time this year I want to create a German-wide map with some Maproulette projects to improve the tagging to make the processing work.

Is it by any chance serving tiles/data for Kraków, Poland? Or having some working setup step-by-step instructions?

No, we only take the Germany dataset from Geofabrik and there are no plans to expand the region any time soon. There are no docs for this ATM but it most of it should be easy to use to set it up for other countries. Hence the "lets talk about" – I can tell you more in a screenshare or chat some time. Or maybe I will make it to SOTMEU…

@tordans
Copy link

tordans commented Mar 28, 2024

Just for clarity…

cycleway:left=track

If someone cares about high level of detail and clarity I would focus on getting rid of any track values and replace them by separately mapped geometries and (in this case) cycleway:left=separate

The same issue is there with lane, I just kept the track because we had it as an example before. (And the consensus is to not map lanes separate most of the time.)

@matkoniecz
Copy link

matkoniecz commented Mar 28, 2024

Then I would say that for lanes, if road in general allows bidirectional bicycle travel then - at least for purposes I know - oneway:bicycle=no is good enough.

Though inventing new optional additional more detailed tagging may be entirely fine and useful for some purposes (like AB!).

@ginnyTheCat
Copy link
Contributor Author

Well, wiki describes tagging as-is

The main problem with the wiki is that cycleway:left=track and cycleway:right=track are different from the oneway they imply with I doubt people tag like.

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.

Design of two-way streets with priority (= only one lane)
4 participants