-
Notifications
You must be signed in to change notification settings - Fork 6
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
Use osm2lanes, guarded by experiment flag #85
Conversation
@@ -0,0 +1,355 @@ | |||
use anyhow::Result; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is adapted from a-b-street/abstreet#890. I wrote it in April and have mostly lost context on some of the subtler choices. There's lot in here that's weird/wrong or is stuff that we should fix in osm2lanes proper. But it's a start!
|
||
use crate::{get_lane_specs_ltr, Direction, DrivingSide, MapConfig}; | ||
|
||
// osm2lanes has a more extensive unit test suite, so why does this one exist? This also checks the |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I decided to keep these unit tests long term. They serve a different purpose than the more detailed ones in osm2lanes
@@ -5,86 +5,36 @@ | |||
"coordinates": [ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The diff here is due to updating geom
.
before:
That road polygon is expressed as a multipolygon with two triangles, which is quite silly.
after:
The fix comes from https://github.com/a-b-street/abstreet/issues/951, which is an epic saga to make geom::Polygon
always represent a "good" polygon composed of an exterior ring and optional interior rings.
…ne road polygon properly show up as a polygon, not a multipolygon with two triangles
508d0df
to
f855320
Compare
For a-b-street/osm2lanes#248
screencast.mp4
The web app now has a setting to opt into osm2lanes, and the unit tests here can now also check osm2lanes + translation. This sets us up for iterating rapidly on osm2lanes and its integration here.
CC @droogmic and @BudgieInWA. Feel free to review async, I'll likely merge in the morning, but address feedback whenever is good