-
Notifications
You must be signed in to change notification settings - Fork 12
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
Brain-dump how osm2streets currently works #117
Conversation
And after writing this (basically a quick code review of everything), I'm extremely convinced switching to opaque IDs from OSM ones will dramatically simplify a bunch of code. Whenever we merge / delete / modify something, we won't need to do this crazy dance of fixing up IDs for turn restrictions and connecting objects |
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.
Great! The underlying reasoning and intentions listed here are very helpful.
|
||
At its heart, a graph of roads and intersections. (Roads lead between exactly two intersections -- "road segments" might be more precise.) Roads have their lanes listed from left-to-right, with a type, width, and direction. Note osm2streets doesn't model bidirectional lanes yet -- sidewalks and shared center turn lanes are either forwards or backwards right now, and something downstream interprets them in a special way. (osm2lanes has more nuance, but isn't used in osm2streets yet.) | ||
|
||
Intersections have a `ControlType` -- stop signs, traffic signals, uncontrolled, etc. This is orthogonal to `IntersectionComplexity` and `ConflictType`... TODO, narrow down valid combinations and give examples. MultiConnection vs Merge, please! |
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.
Yes, I'll go ahead with the IntersectionComplexity
to IntersectionType
change I mentioned elsewhere.
docs/how_it_works.md
Outdated
|
||
### Merging a short road | ||
|
||
`merge_short_road` removes a road, then combines the two intersections into one. |
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 should be collapse_short_road
or merge_intersection
!
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.
Oof yeah, you're right. To me, the first is more descriptive -- "merge intersections" isn't clear about how many. I'll do a quick renaming...
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.
docs/how_it_works.md
Outdated
|
||
`merge_short_road` removes a road, then combines the two intersections into one. | ||
|
||
It first calculates trimmed intersection geometry for the two intersections. On each connected road (besides the short one being merged, of course), we store the trimming distance in `trim_roads_for_merging`, so that later the intersection geometry algorithm can follow a special case for the single merged intersection. |
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 mentioned briefly in our recent meeting that I think the trim distances and multi-node intersections are fundamental to the data model we want. I hope these ideas can be incorporated into the data types over time.
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.
trim_roads_for_merging
is capturing start/end trim distances before collapsing a short road. I agree storing this on every road could make things much more clear. Still not sure how multi-node intersections should work.
If I'm thinking about the hydrated concept, maybe it becomes less weird. As a first pass, we run the intersection geometry algorithm on the original network. Later we remove a short road, but leave the trim distances alone:
Still feels like it's a bit special-cased; that algorithm needs to know to use the trim distances that've been calculated before...
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.
...that algorithm needs to know to use the trim distances that've been calculated before...
This is the kind of reason I think trim distance needs to be a fundamental part of the Road
model. Editing a bunch of OSM highways teaches you that there are many scenarios where the geometry near the end of a way is nonsense - usually the physical feature that the way represents has ended - but it is clear why the way continues: because it needs to connect the graph properly. Slip lanes and sausage links are the weirder ones, where you commonly end up making aesthetic decisions about where to put the connecting geometry, but every traffic intersection has the same thing.
I think a fundamental trim distance field should exist on Road
that represents where the physical feature actually ends, or at least where the Road
ends and the Intersection
takes over. I'm not sure quite how it will all shake out, but I feel like a sort of "minimum" trim distance can be calculated very early based on where the roads overlap each other - much like it seems trim_roads_for_merging
is doing. Maybe stop lines and other context clues can be incorporated right from the start, leading to more accurate Intersection
area calculations, or maybe the come much later, staying out of the way of the early transformations and being considered near the end for largely aesthetic reasons.
The big complication is when a road continues uninterrupted through an intersection (like when a side road with a stop sign joins a major road, or the slip lane case). In these cases, some of the roads do physically continue right to the end of their geometry.
I think the best way to think about this for now, is that the Road
s all get trimmed back to the overlapped area, and the Intersection
takes responsibility for the whole overlapping area. The Intersection
can figure out which roads continue through and use its own internal Road
instances or something to draw the section of the road that continues through the Intersection
area. The Intersection
is already responsible for handling the individual lane turn paths, so this is pretty natural.
I think we can consider allowing the uninterrupted ("major") roads to remain untrimmed (with the "minor" roads getting trimmed to avoid any overlap) later in development when we have a better understanding. I think that would unlock some more sophisticated and nuanced outcomes, but we need to get a handle on the simpler cases first.
Editing complex intersections in OSM also teaches you that inside an intersection area (where the lane marking have ceased), the position of any ways doesn't really matter, only the connectivity of the ways. So the data is authored with the assumption that the whole intersection is being considered together. Therefore, our Intersection
model should treat multi-node intersections as first class. Perhaps as the other case but not as special case.
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.
All of this makes sense to me! An attempt to extract some next steps...
I think a fundamental trim distance field should exist on Road that represents where the physical feature actually ends
Then as you suggested earlier, a trim_distance_start
and trim_distance_end
field would make sense. The more I look at
// true if src_i matches this intersection (or the deleted/consolidated one, whatever)
pub trim_roads_for_merging: BTreeMap<(osm::WayID, bool), Pt2D>,
the more I'm wondering why I chose this convoluted representation originally. It's equivalent to start and end trim distance per road, except it has an indirect way of referring to a road. I think once we have opaque IDs and the process of removing short roads from the graph doesn't have to reassign IDs everywhere, this BTreeMap will be even sillier.
a sort of "minimum" trim distance can be calculated very early based on where the roads overlap each other
Right, so that'd pretty much mean running transform/intersection_geometry.rs
upfront, and maybe incrementally recalculating trim distances and intersection polygons as we do later transformations. This makes the hydration idea even more relevant.
Maybe stop lines and other context clues can be incorporated
I've never even looked at these in OSM in practice, it'd be great to at least start extracting them and displaying as an optional extra layer! We could see how well the current intersection trimming algorithm matches up to the stop lines, as a very basic start
I think we can consider allowing the uninterrupted ("major") roads to remain untrimmed (with the "minor" roads getting trimmed to avoid any overlap)
One possible consequence of this could be how we define movements then. If the main road is uninterrupted, the movement geometry there may not exist (or be just a point). This would break A/B Street's current definition of turn conflicts for figuring out that a side road turning onto a main road. But the new clockwise-overlapping definition would sort that out.
Also, looks like the case of a side road confused me long ago: https://a-b-street.github.io/docs/tech/map/geometry/index.html#a-radically-simpler-approach. I found this representation problematic, because I wanted the intersection polygon to physically exist and be that entire rectangle. The more nuanced idea of an "intersection" without geometry of its own never occurred!
Therefore, our Intersection model should treat multi-node intersections as first class. Perhaps as the other case but not as special case.
I'm nearly at the point where Intersection
will have original_ids: Vec<osm::NodeID>
, so we can start to directly represent this. We could also store all of the internal OriginalRoads
if that winds up proving useful.
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 more I'm wondering why I chose this convoluted [trim distance] representation originally.
Seems appropriate if it were in fact an edge case relating just to this transformation. I had the benefit of seeing the trim distance idea in the JOSM Lanes plugin, so I had it as part of my mental model right from the start.
Right, so that'd pretty much mean running
transform/intersection_geometry.rs
upfront, and maybe incrementally recalculating trim distances and intersection polygons as we do later transformations. This makes the hydration idea even more relevant.
"Hydration" is a term I came up with while writing that comment to describe how I'm conceptualising some of the properties as derived data which is a more useful representation of the state. (The term is not entirely perfect, but "the hydration idea" is very useful).
It would by awesome if the intersection geometry/trim distance calculation can produce good results at every stage so that we can treat the geom and trim distances as purely derived state and recalculate them in the straight forward way as you describe. There is a chance that the trim distance needs to be actual state that we guess initially but otherwise need to maintain through transformations, but I'd like to avoid that if possible.
I've never even looked at [stop lines] in OSM in practice, it'd be great to at least start extracting them and displaying as an optional extra layer! We could see how well the current intersection trimming algorithm matches up to the stop lines, as a very basic start.
Yes, that would be cool. In my experience, they are not commonly present in the data, but I think that's because they aren't used anywhere that I've seen. Making them move visible would be a good outcome in its own right.
I think we can consider allowing the uninterrupted ("major") roads to remain untrimmed (with the "minor" roads getting trimmed to avoid any overlap)
One possible consequence of this could be how we define movements then. If the main road is uninterrupted, the movement geometry there may not exist (or be just a point). This would break A/B Street's current definition of turn conflicts for figuring out that a side road turning onto a main road. But the new clockwise-overlapping definition would sort that out.
These are exactly the sorts of complications that I was coming up with, and why I think we shouldn't pursue it yet.
Also, looks like the case of a side road confused me long ago: https://a-b-street.github.io/docs/tech/map/geometry/index.html#a-radically-simpler-approach. I found this representation problematic, because I wanted the intersection polygon to physically exist and be that entire rectangle. The more nuanced idea of an "intersection" without geometry of its own never occurred!
The piece that might be worthwhile earlier on is supporting Connection
intersections whose geometry is a line with no area (like where the vertical roads meet in the linked diagram, if the side road didn't exist). That would require "angled end caps" #95 and probably a non-intersection solution to drawing the beginning/end of new lanes (like where turn lanes appear from nothing). But for now I think those situations can all be treated as the same kind of thing: movements through large Intersection
areas, some of which are marked as normal lanes.
|
||
## The transformations | ||
|
||
TODO: before/after pictures |
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 don't think it's worth getting so carried away as to add screenshots here. I think this is valuable as a jumping off point that should get shorter and shorter as the structure of the code gets clearer, and as the docstrings improve.
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.
Fair enough. At some point for explaining purposes, it'd be nice to have a "gallery" of all the transformations available. Maybe if we plumb more info into the street-explorer URLs, we could eventually express "run with debug steps, and show step 5"
3dd9321
to
55ba354
Compare
This is quite messy, but more or less covers everything