Add turn movements by BudgieInWA · Pull Request #207 · a-b-street/osm2streets

BudgieInWA · 2023-03-05T11:01:02Z

We don't need all of this before merging, but here's the plan so far:

Rename Movement -> Turn (road level), add TurnMovement (lane level)
Calculate TurnMovements for degenerate (2 road) intersections
Calculate movements for IntersectionKind::Connections with more than two roads.
Do some default allocations for arbitrarily complex intersections.
Add TurnKind to determine what kind of lane markings the TurnMovements get.
Render "turn guide" lane markings for "turn" turns.
Render ordinary lane markings for "continuation" turns (ideally, reusing the normal logic).

This is making some good progress towards lane level movements. In fact, some movements are calculated and drawn! I added the movement centrelines in post, the left edge of the movements are being drawn:

We need angled road ends, #95, to draw these markings properly.

Tests simply rename "movements" list to "turns"

…d offset

dabreegster

Really cool to see this!

Not that I'm likely to play with the JOSM side soon, but is https://github.com/BudgieInWA/JOSM2Streets/ still up-to-date, or are there new rendering goodies corresponding to this?

dabreegster · 2023-03-05T11:09:45Z

+
+        match intersection.kind {
+            IntersectionKind::Connection => {
+                for turn in intersection.turns.iter_mut() {


Suggested change

for turn in intersection.turns.iter_mut() {

for turn in &mut intersection.turns {

Optional, not really sure what style is preferable. If we later need to do anything else with the iterator, like .enumerate(), then how you have it is nicer (because we'd switch back to it)

dabreegster · 2023-03-05T11:12:34Z

+
+/// A specific path that a vehicle can take through a turn.
+///
+/// Lane numbers are counted from the perspective of the direction of the turn.


This stumped me for a few minutes; can we add an example or clarify the comment? LaneIDs normally index into lanes_ltr. Normally whatever direction we're thinking about a road, the lane ID is "absolute" -- 0 means the left side of the road based on its OSM-defined orientation. It feels like here we have a sudden explosion of possibilities, if we point from the right side of one road to the left side of another in the backwards direction, for instance...

And actually now I'm wondering if this refers to LtrLaneNum and not LaneID and am more confused :\

D'oh, this is an outdated comment. You're right, it was referring to LtrLaneNum, trying to clarify about Forward/Backward variants.

dabreegster · 2023-03-05T11:17:05Z

+}
+
+/// Limitations: ignores both ways lanes
+pub fn default(


Getting tests of some form here is important IMHO. We could maybe make unit tests if we can succinctly express input and output, or maybe it's better to do our usual snapshot+diff testing approach, and get more rendering into Street Explorer

I think unit tests is a great idea here, and covering at least some of these code paths in our snapshot tests is important too.

I keep finding myself wanting to create "unit test" like snapshot tests (with very simple, contrived inputs), instead of the large "integration tests" that our current snapshot tests are. Maybe that will be worthwhile at some point, but unit testing these kinds of functions is an easier way to to address my want!

Hmm... nothing would stop us from drawing something synthetic in JOSM and adding the .osm to tests/ like we normally do

dabreegster · 2023-03-05T11:17:50Z

+            // Lanes through intersections
+            for inter in self.intersections.values() {
+                for Turn { movements, .. } in &inter.turns {
+                    let Some(movements) = movements else { continue };


OK I actually hadn't understood the point of let-else till this; very cool!

Yep, a bunch of nesting can be replaced with early returns! The else block can run any code, as long as it diverges (doesn't run to the end).

dabreegster · 2023-03-05T11:18:54Z

        areas
    }
+
+    fn lane(&self, lane: LaneID) -> Option<&LaneSpec> {


Could we move to road.rs in the impl StreetNetwork block?

dabreegster · 2023-03-05T11:20:32Z

                pairs.push((polygon.to_geojson(Some(&self.gps_bounds)), make_props(&[])));
            }
        }
+        if pairs.is_empty() {


In general, when should we expect turns to exist, but not movements? Right now update_i does both. Is this temporary before we have movements defined in all cases?

Yes, that was a temporary measure. I don't know where it will land though. I think we will be able to calculate movements any time we calculate turns, so we will be able to remove the Option.

I was playing with using an Option in places where data is only present after some step of the algorithm, instead of using *::dummy() kind of values.

dabreegster · 2023-03-05T11:21:51Z

        Ok((left, right))
    }

+    pub fn intersection_at(&self, end: RoadEnd) -> IntersectionID {


These new helpers are nice; there's probably many places throughout the code where they can come in handy!

Using RoadEnd has been indispensable while dealing with the possibility space explosion in crate::movements, and these helpers remove a lot of noise from those implementation sites that make it possible to see the (still very complicated) logic there.

BudgieInWA · 2023-03-06T00:48:59Z

+
+/// A specific path that a vehicle can take through a turn.
+///
+/// Lane numbers are counted from the perspective of the direction of the turn.


D'oh, this is an outdated comment. You're right, it was referring to LtrLaneNum, trying to clarify about Forward/Backward variants.

BudgieInWA · 2023-03-06T00:59:20Z

+}
+
+/// Limitations: ignores both ways lanes
+pub fn default(


I think unit tests is a great idea here, and covering at least some of these code paths in our snapshot tests is important too.

I keep finding myself wanting to create "unit test" like snapshot tests (with very simple, contrived inputs), instead of the large "integration tests" that our current snapshot tests are. Maybe that will be worthwhile at some point, but unit testing these kinds of functions is an easier way to to address my want!

BudgieInWA · 2023-03-06T01:02:48Z

+            // Lanes through intersections
+            for inter in self.intersections.values() {
+                for Turn { movements, .. } in &inter.turns {
+                    let Some(movements) = movements else { continue };


Yep, a bunch of nesting can be replaced with early returns! The else block can run any code, as long as it diverges (doesn't run to the end).

BudgieInWA · 2023-03-06T01:05:56Z

-                        "movements",
+                        "turns",
                        serde_json::Value::Array(
                            intersection
-                                .movements
+                                .turns
                                .iter()
-                                .map(|(a, b)| format!("{a} -> {b}").into())
+                                .map(|Turn { from, to, .. }| format!("{from} -> {to}").into())


I need to add TurnMovements to the test output to observe these changes.

BudgieInWA · 2023-03-06T01:10:56Z

                pairs.push((polygon.to_geojson(Some(&self.gps_bounds)), make_props(&[])));
            }
        }
+        if pairs.is_empty() {


Yes, that was a temporary measure. I don't know where it will land though. I think we will be able to calculate movements any time we calculate turns, so we will be able to remove the Option.

I was playing with using an Option in places where data is only present after some step of the algorithm, instead of using *::dummy() kind of values.

BudgieInWA · 2023-03-06T01:14:36Z

        Ok((left, right))
    }

+    pub fn intersection_at(&self, end: RoadEnd) -> IntersectionID {


Using RoadEnd has been indispensable while dealing with the possibility space explosion in crate::movements, and these helpers remove a lot of noise from those implementation sites that make it possible to see the (still very complicated) logic there.

BudgieInWA · 2023-03-06T01:19:39Z

+            path: PolyLine::new(vec![src_pt, dst_pt])
+                .unwrap_or_else(|_| PolyLine::must_new(vec![src_pt, Pt2D::new(0.0, 0.0), dst_pt])),


This is where I would want to create a nice curved path, instead of just jamming two points in.

After working with it, it might be better to remove the geometry aspect from here and calculate it later, or on the fly. That matches up better with where geometry of other kinds is calculated.

Also, the or_else polyline sometimes crashes when the first one does, but not always!

On-the-fly geom could be nice, yeah, and would have reduce file size requirements maybe. (But it's not a clear-cut tradeoff; lazily calculating in a UI later can be sluggish.)

Also, the or_else polyline sometimes crashes when the first one does, but not always!

I actually didn't look at this carefully; inserting (0, 0) in the middle is an attempt to make the total line length larger? In what kind of cases is the above PolyLine::new(vec![src_pt, dst_pt]) failing -- when the two points are very close together? What should we do in those cases -- maybe it's valid to just have no movement geometry?

Yes, when the points are close together. Given our current approach, we shouldn't have any zero length movements, but I think it happens coincidently sometimes.

I'm going to experiment with removing path from TurnMovement so that turns and movement are only concerned with connectivity for now (and don't have to be recalculated if trim distances change, or something like that). I want to think about storing lane centerlines and movement paths in the same way.

BudgieInWA · 2023-03-06T10:01:25Z

Really cool to see this!

Not that I'm likely to play with the JOSM side soon, but is BudgieInWA/JOSM2Streets still up-to-date, or are there new rendering goodies corresponding to this?

The JOSM2Streets build embeds StreetNetwork.jar, which in turn embeds the rust lib. If the osm2streets java classes haven't changed their API, then you can build your own StreetNetwork.jar and replace the copy in the JOSM2Streets repo before building. You'd need to do that for these changes, I haven't pushed up the new JAR yet.

oneil512 · 2024-06-09T06:23:12Z

Hi, is this still being considered?

dabreegster · 2024-06-09T08:24:54Z

I don't think Ben or I have had time to work on this, so not anytime soon. Are you interested in picking it up?

paaspaas00 · 2025-10-01T23:06:05Z

Hi @BudgieInWA is this being worked on?

BudgieInWA added 4 commits February 14, 2023 19:48

New terminology: "Turn" for road to road, "Movement" for lane to lane

52b754b

Tests simply rename "movements" list to "turns"

Generate default lane-level movements for IntersectionKind::Connections

e39c237

Draw turn guides for any calculated movements through intersections

cc4e54d

Use placement for movements by implement matching lanes with the foun…

0d26746

…d offset

dabreegster reviewed Mar 5, 2023

View reviewed changes

BudgieInWA commented Mar 6, 2023

View reviewed changes

dabreegster mentioned this pull request May 25, 2023

Add an optional layer showing existing turn restrictions. a-b-street/abstreet#1090

Merged

	for turn in intersection.turns.iter_mut() {
	for turn in &mut intersection.turns {

		path: PolyLine::new(vec![src_pt, dst_pt])
		.unwrap_or_else(\|_\| PolyLine::must_new(vec![src_pt, Pt2D::new(0.0, 0.0), dst_pt])),

Conversation

BudgieInWA commented Mar 5, 2023

Uh oh!

dabreegster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

BudgieInWA commented Mar 6, 2023

Uh oh!

oneil512 commented Jun 9, 2024

Uh oh!

dabreegster commented Jun 9, 2024

Uh oh!

paaspaas00 commented Oct 1, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants