Skip to content

Cutover to osm2lanes - #890

Draft
dabreegster wants to merge 1 commit into
mainfrom
osm2lanes
Draft

dabreegster wants to merge 1 commit into
mainfrom
osm2lanes

Conversation

@dabreegster

@dabreegster dabreegster commented Apr 13, 2022

Copy link
Copy Markdown
Collaborator

This is far from ready, but figured it'd be better to track progress here instead of a-b-street/osm2lanes#71. My workflow right now (which assumes the dev env is setup) is:

# Reimport a sample map
./import.sh --raw --map montlake
# Load the UI in a mode where ctrl+tab switches between old/new version of the map
./data/diff_changed_map.sh data/system/us/seattle/maps/montlake.bin

Screenshot from 2022-04-13 22-32-42
Screenshot from 2022-04-13 22-32-40

Lane width is the main difference that jumps out. Even a service=alley looks super wide. I'll start looking at ways of specifying different values in Locale.

CC @droogmic

@droogmic

Copy link
Copy Markdown

Thanks, I see the issue now. I need to look for some national road building standards to get a good defaults for these.

@dabreegster dabreegster left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@droogmic, I decided to try finding some way to cutover to osm2lanes and patch around any "breaking" changes from A/B Street's perspective, so we could iterate faster. A few workarounds are here now, making my test Seattle map look close enough.

But I tried a map in the UK and hit a few new issues, which I'll work through next. If any of these merit issues or test cases in osm2lanes, I'll send a PR.

  1. transform/tags_to_lanes/modes/non_motorized.rs retains A/B Street's hack of treating shared walking+biking spaces as a [shoulder, cycle lane, cycle lane, shoulder]. But https://www.openstreetmap.org/way/139974130 became just a sidewalk, because bicycle=dismount. I think osm2lanes is correct here; I just need to audit some of my maps to see why I added the dismount case originally. I was probably trying to make the cycling graph somewhere in Seattle be connected for an experiment which is hopefully not needed anymore.

  2. Lots of ambiguous busways in practice, like https://www.openstreetmap.org/way/1027418221. I might try and add a hack around this to force some result. Until we have tooling to help mappers detect and fix these problems in bulk, it's not super reasonable to expect people to fix them everywhere. Or... I might just fix myself in the handful of maps I care about particularly now for work projects.

  3. Tags({"abst:endpt_back": "true", "abst:endpt_fwd": "true", "abst:osm_way_id": "1004419293", "abst:parking_inferred": "true", "abst:sidewalks_inferred": "true", "highway": "tertiary", "name": "Hillside Road", "parking:lane:both": "no_parking", "sidewalk": "both", "sidewalk:right": "yes", "sidewalk:right:surface": "asphalt"}): unsupported: -- The error doesn't say what's unsupported? I'll add some osm2lanes test cases to figure out what's up

Comment thread raw_map/src/lane_specs.rs Outdated
// Workaround common incorrect tagging
tags.checked_insert("sidewalk", "no").unwrap();
} else if k == "sidewalk" && v == "separate" && cfg.inferred_sidewalks {
// Make blind guesses

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

sidewalk=separate is unsupported (because it's something osm2lanes alone can't really handle...), so to match existing behavior, I'm doing some rewriting here

Comment thread raw_map/src/lane_specs.rs Outdated
.flatten()
.collect::<Vec<_>>();

// No shoulders on unwalkable roads

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I spotted shoulders on motorways. That's intentional, right? The definition of shoulder is fuzzy, I guess. If we mean it as a place for vehicles to pull over in an emergency, then it's reasonable to assume they exist on motorways. A/B Street currently treats shoulders as a space where technically pedestrians can walk, but it's dangerous / unpleasant to. In the US, it's generally illegal / extremely inadvisable to walk on the shoulder of most interstates. So I'm doing this post-processing here, to make sure pedestrians in the simulation don't find some clever shortcuts

Comment thread raw_map/src/lane_specs.rs Outdated
if part == "designated" {
back_side[idx].lt = LaneType::Bus;
}
// Use our own widths for the moment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

I'd like to cutover ASAP and iterate on smaller issues. I realized the width issue is easy to workaround -- just override with the current A/B Street config. We can remove this later.

@dabreegster

Copy link
Copy Markdown
Collaborator Author

I made some more progress, continuing the strategy of fudging the input and output of osm2lanes to match current behavior a bit better. Some of the changes:

  • Found a bug in prior abst code where bus lanes on most roads had the wrong width value!
  • Some roads lose shoulders on one side, which is closer to reality. I'll just need to check carefully in places like North Seattle; shoulders were added previously to make sure the walking graph is reasonable
  • highway=footway, bicycle=dismount becomes just a walkable lane. I'm happy to "lose" a few of those previous bike trails; they were always a hack.
  • Patching around living streets, oneway=reversible, few other things
  • Bidirectional cycletracks are transformed into two separate lanes; the simulation layer further down the chain doesn't understand bidi lanes except for sidewalks
  • Attempting to hack around multiple bus schemas. Some attempts in Bristol, a map I need to be very careful about right now, have confusing results

I kind of want to wrap this up quickly instead of leaving it hanging, but it's really taking time to crunch through all the issues. :\

@droogmic

Copy link
Copy Markdown

| I kind of want to wrap this up quickly instead of leaving it hanging, but it's really taking time to crunch through all the issues. :\

I think osm2lanes is still a little bit immature to avoid having to deal with lots of issues.

@dabreegster

Copy link
Copy Markdown
Collaborator Author

I rebased against A/B Street changes and also updated osm2lanes. I left off looking at changes in Bristol particularly, as I have a work project centered there. ./import.sh --raw --map --city=gb/bristol
Main problem was... oh hey, it's gone, Bristol's actually looking better with osm2lanes now! Bus lanes at https://www.openstreetmap.org/way/987715676 showing up now.

@droogmic

Copy link
Copy Markdown

any opinion on what the next most problematic thing is for you?
cyclist=dismount, more bus stuff, or road widths?

@dabreegster

Copy link
Copy Markdown
Collaborator Author

Probably road widths.

But based on my quick diff yesterday, I bet we could do the cutover soon. I'll try to dedicate a few hours tomorrow to trying it and at least seeing what other problems come up. Once we get past this initial hump, subsequent improvements (removing overrides on the A/B Street side and trusting osm2lanes I/O more directly) can be done gradually. And osm2lanes can keep evolving at whatever speed; I'll re-import everything and repeat validation every so often.

@dabreegster

Copy link
Copy Markdown
Collaborator Author

https://www.openstreetmap.org/way/22772836
cycleway=opposite without oneway=yes oneway:bicycle=no

This is the most common error affecting a hi-pri map. I'll probably hack around it for the moment...

@dabreegster

Copy link
Copy Markdown
Collaborator Author

Ignore above.

@droogmic, I think I'm getting close -- I'm checking some last things, then I'll attempt a full reimport. Once I get that to a point where I'm happy, I'd prefer to upload the changed data and merge quickly to avoid hassle in my other work. (Branching code is easy, but not for data.) Are you interested in reviewing the code here? I don't expect it to be particularly understandable without lots of context.

Once this first cutover is done, iterating on smaller fixes will be much easier on my end, though I'll still probably batch the full reimports and validation on my end. Cleanup like:

  • removing is_road
  • doing less transformation of tags
  • doing less transformation of osm2lanes output (like trusting widths)
  • cleaning up all the places that insert an inferred sidewalk or parking tag (a weird old technical decision that's no longer valid and is mostly just confusing)

@droogmic

Copy link
Copy Markdown

Are you interested in reviewing the code here?

not particularly, unless you either, want me to understand it generally, understand the workarounds you are still doing, or want me to find defects

@dabreegster

Copy link
Copy Markdown
Collaborator Author

not particularly, unless you either, want me to understand it generally, understand the workarounds you are still doing, or want me to find defects

Alright, feel free to ignore this PR then.

I imported everything and managed to provoke a few crashes. I'll track down where when I get some time and file + fix bugs:

  • somewhere in london, newcastle, auckland: "capacity overflow" in osm2lanes::transform::tags_to_lanes::road::RoadBuilder::from -- I suspect something's trying to allocate a huge vector. If I had to blindly guess, there's a "-1" we're parsing as a usize
  • somewhere in seatle: unexpected speed 3025 mph', /home/dabreegster/.cargo/git/checkouts/osm2lanes-0a9401f46878d04a/df0e7bb/osm2lanes/src/transform/tags_to_lanes/separator/semantic.rs:27:18

@droogmic

Copy link
Copy Markdown

Is it hard for you to find the way ID associated with the failures?

Comment thread raw_map/src/lane_specs.rs Outdated
fwd_side.push(fwd(LaneType::Biking));
}
Err(err) => {
error!("osm2lanes broke on {:?}: {}", orig_tags, err);

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I guess the tracing we are missing here is way_id

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

It's awkwardly embedded in the list of tags as abst:way_id. I'll drag it out and print a URL to make for faster debugging.

There were straight-up panics though, I'm adding some debugging to get examples

@dabreegster
dabreegster force-pushed the osm2lanes branch 3 times, most recently from 5786339 to 27f067e Compare May 1, 2022 20:28
Comment on lines +463 to +475
// Hack for Geneva, which maps sidewalks as separate ways
if !tags.contains_key(osm::SIDEWALK)
&& tags.is("oneway", "yes")
&& name.city == CityName::new("ch", "geneva")
{
tags.insert(osm::SIDEWALK, "both");

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

I guess I should get some test cases from Geneva ;)

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

@ThomasDagier uncovered some gnarly bus tagging there, but otherwise, nothing too special here. Hacking around sidewalks is unfortunately common in this codebase.

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.

2 participants