Skip to content
This repository has been archived by the owner on Mar 9, 2023. It is now read-only.

Cutover A/B Street to using osm2lanes #71

Open
dabreegster opened this issue Feb 8, 2022 · 16 comments
Open

Cutover A/B Street to using osm2lanes #71

dabreegster opened this issue Feb 8, 2022 · 16 comments
Assignees

Comments

@dabreegster
Copy link
Contributor

osm2lanes is nearly at a point where I'd like to make A/B Street take a dependency on it and remove the original copy of the osm2lanes logic. Just writing down some notes about how to transition, not sure when I'll have time to tackle this.

The steps are probably:

  1. Replace is_road, which initially decides which OSM ways to treat as roads in A/B Street
  2. Replace get_lane_specs_ltr
  3. Import a few maps and manually look for diffs -- I expect to find a few obvious ones, and iterate on osm2lanes or the way A/B Street calls it accordingly
  4. After most issues seem fixed, import all maps and do the usual screenshot diff-based validation procedure
  • Because the osm2lanes API is still in flux, I'm going to add the git dependency and explicitly pin to a version. It'll be moderate effort to keep A/B Street up-to-date with changes here, so I expect to upgrade osm2lanes in occasional batches
  • The biggest missing thing in osm2lanes is width per lane. In the short term, I'll probably just keep https://github.com/a-b-street/abstreet/blob/7fdfdd5e1042a55a7d788ff8470656be9580291b/map_model/src/objects/lane.rs#L495 and do the mapping on the A/B Street side. We'll eventually move this into osm2lanes and handle it properly (reading width tagging from OSM when it's present, using region-specific config)
  • A/B Street currently doesn't import footways, or it mangles them into weird cycleways with walkable shoulders: support highway=footway highway=steps highway=path osm2streets#81. Unless I fix that first (unlikely), I'll probably keep transforming things on the A/B Street side to match the current weird behavior.
  • For the initial cutover, I'll ignore separator lanes with markings (unless they're barriers), to match A/B Street's current lane types. Things like dashed or solid lines are currently just a rendering concept. Not sure how to properly model those yet.
  • Likewise, osm2lanes handles direction for bidirectional and untraversable lanes in a proper way. Some weird corners of A/B Street, like the low-traffic neighborhood "trace around the block" logic, currently depend on sidewalks having a forwards and backwards direction a certain way. I'll post-process the output of osm2lanes in the short-term, and clean up the hack eventually.
@dabreegster dabreegster self-assigned this Feb 8, 2022
@droogmic
Copy link
Collaborator

droogmic commented Feb 8, 2022

Sounds good,

Replace is_road

This still needs to be implemented in osm2lanes right?

The rest seems relatively clear, cutovers are always challenging...

@dabreegster
Copy link
Contributor Author

This still needs to be implemented in osm2lanes right?

I just tried the web viewer... plug in https://www.openstreetmap.org/way/39365402, a building, and it rewrote into a road with 2 lanes. So there's the answer. :) If the input doesn't represent something road-like, we should probably bail out cleanly.

is_road has lots of strange A/B Street logic again; we shouldn't preserve that in osm2lanes

dabreegster added a commit to a-b-street/abstreet that referenced this issue Feb 19, 2022
OSM and other raw input into and store, before later converting to a
Map.

Why?

- build-time performance: while iterating on geometry problems, map_editor in release mode took 33s to build before, 11s now that the crate is split
- better layering: operations on a RawMap are becoming increasingly distinct from later transformations on the bigger map model
- this helps tease apart the dependencies of the intersection polygon algorithm for #846
- this will make it simpler to cutover to osm2lanes for a-b-street/osm2lanes#71

There's further reorganization in raw_map and map_model that'll follow,
but the main work is done here.
@dabreegster
Copy link
Contributor Author

FYI I started this at https://github.com/a-b-street/abstreet/tree/osm2lanes. https://github.com/a-b-street/abstreet/blob/osm2lanes/raw_map/src/lane_specs.rs has the conversion logic. So far, not bad!

Some of the biggest issues jumping out at me:

  1. unimplemented: highway=construction, example https://www.openstreetmap.org/way/663639108
  2. Very high width values. Some config based on highway={service, residential, primary} would go a long way
  3. unsupported: cycleway:right=opposite_lane, example https://www.openstreetmap.org/way/6287847 (I see a separate cycleway tagged there too... who knows)
  4. Loads of errors that occur in practice, like unsupported: more than one bus lanes scheme used, example https://www.openstreetmap.org/way/39817102

I've got enough other things to juggle to help out anytime soon, sorry. :\ But I am working on extracting the logic for making road/intersection polygons into a separate crate, adding unit tests, and taking other steps to make it usable outside of A/B Street

@droogmic
Copy link
Collaborator

unimplemented: highway=construction

do you need any metadata associated with this, or is type=construction alone enough?

https://www.openstreetmap.org/way/6287847

I think that is just bad tagging...
https://wiki.openstreetmap.org/wiki/Key:cycleway:right?uselang=en
and the cycleway is tagged separately...
It should be cycleway:right=separate, and be ignored until we have a way of merging these ways.

https://www.openstreetmap.org/way/39817102

fascinating example, so many random tags just thrown together... I think this is where we can start using the Infer<> values to "build" a best guess, and throw warnings whenever there is something suspicious. The error for now should also have been unimplemented, not unsupported.

@dabreegster
Copy link
Contributor Author

do you need any metadata associated with this, or is type=construction alone enough?

type = construction is nearly enough. I think using construction={residential,cycleway,primary, ...} is useful to feed into the width heuristics though.

I think that is just bad tagging...

100% agree... This is common though. A/B Street needs to do something for broken tagging besides crashing. My approach in the past has been to try to best-guess what the tagger meant, but I think that led to the super messy v1 of the tags_to_lanes implementation. I vote we stay strict in the new osm2lanes. In A/B Street, I can just transform broken roads into something obviously wrong, like a super skinny one-way one lane road, and display some kind of error. If these errors happen frequently in areas I personally need for other work, I'll pre-process tags on my end to "fix" them or attempt to fix upstream in OSM.

So... osm2lanes should stay strict.

I think this is where we can start using the Infer<> values to "build" a best guess

Same as above -- I have no idea what the mappers meant here. :D I'd honestly be happy just quasi-refusing to attempt to render this in A/B Street going forward, to encourage cleaning up the tagging. Handling the best guess in osm2lanes is an option, but

  1. I would vote that complexity stays cleanly separated from the rest of the codebase, something like tags_to_lanes/repair/bus.rs
  2. I wouldn't prioritize working on these inferences over handling more "correct" tagging

@droogmic
Copy link
Collaborator

Do you need an is_road clone,

or can you do:
Highway::from_tags(&tags).is_supported()?

@droogmic
Copy link
Collaborator

my next 2 PRs, in no particular order:

  • move to a RoadBuilder API internally to do full roundtrip.
  • lifecycle support

@dabreegster
Copy link
Contributor Author

I can use Highway::from_tags(&tags).is_supported(). I suspect I'll keep some of the weird filtering and transformation of is_road in the short-term, or move it just after this method.

@droogmic
Copy link
Collaborator

so construction is now in, where do we stand?

@dabreegster
Copy link
Contributor Author

I will revisit after March 23, sorry :( Very high-stakes conference / deadline to finish the LTN work

@droogmic
Copy link
Collaborator

(reminder)

@dabreegster
Copy link
Contributor Author

I did not get less busy yet. :(

But when I revive this, I'll use it as a forcing function to start some kind of locale-specific width config to roughly match areas I've spent a little time tuning before.

@droogmic
Copy link
Collaborator

Ah, width is missing.
I am mainly using this to prioritise my efforts, because at this point the only thing I really see left to do is to iterate through tests cases trying to turn them all on.

@dabreegster
Copy link
Contributor Author

Ah, yeah. Biggest missing thing from my POV is width. And if we wind up with a public API sort of like https://github.com/a-b-street/abstreet/blob/191f1ca265a51563995460f5375913ed30f7895a/raw_map/src/types.rs#L472 (but with locale, highway type, and lane type as input), even better, because then A/B Street's lane editor UI will just automagically make more sense in different places.

@droogmic
Copy link
Collaborator

I think the latest version will improve things, I haven't set up a dev environment for abstreet yet to check myself though, will do that ASAP

@dabreegster
Copy link
Contributor Author

I'll update the draft PR with latest osm2lanes real quick. I don't think I can work on validation for a bit, though.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants