Start calculating Movements and Conflicts #96

BudgieInWA · 2022-09-22T02:07:28Z

still faulty due to road sort order and missing travel direction checks.

See #67. We should be able to see on and off ramps, sausage links, etc. pretty clearly in the result when its working.

street_network/src/initial.rs

street_network/src/render.rs

street_network/src/transform/classify_intersections.rs

street_network/src/types.rs

still faulty due to road sort order and missing travel direction checks. We should be able to see on and off ramps, sausage links, etc. pretty clearly in the result when its working.

…ections cross or not

The movements become incorrect (and lead to crashes) when the number of roads in an intersection changes. The movements should just be recalculated (which might be easy, but I haven't reasoned it out yet).

BudgieInWA · 2022-11-07T13:55:54Z

osm2streets/src/render.rs

+            for (a, b) in intersection.movements.iter() {
+                if *a != *b {
+                    pairs.push((
+                        Line::must_new(road_points[*a], road_points[*b])


Oh, this can fail if the points are too close together... I find it surprising that a line type can't even represent lines segments of length zero... I guess that would mean that it has no direction, and so a whole host of operations on it would need to be fallible...

I'll use the constructor that returns a result instead.

BudgieInWA · 2022-11-07T14:58:23Z

@dabreegster I finally found the spare energy to get this functioning at a draft level. Movements are drawn crudely as arrows between the Road endpoints on the new "Debug movements" layer, and some merges vs diverges are detected.

Still to do:

actually look into the results and new test output to see if it's working right
add a heuristic to identify movements that are U-turns (for the suasage-link case). Tagging convention assumes they are easily identified, so we should be able to find prior work.
filter out roads that don't have car traffic at all (they should be considered in a separate pass)
recalculate movements when roads are added/removed
offset the arrow endpoints for bidi roads closer to the center of the indicated direction
document ConflictType
address a TODO or two
tidy up some use statements

dabreegster · 2022-11-07T15:13:56Z

Awesome! I'm having a look now. Do you want high- or low-level feedback now, or after those TODOs done?

BudgieInWA · 2022-11-07T16:45:15Z

Any high level thoughts now. I'll request a review when I'm ready for it to go in.

dabreegster

I haven't dove into the logic yet, but just reporting a bug in arizona_highways:

osm2streets/src/transform/classify_intersections.rs

… classifications I have spot checked the results, which look largely correct. I did find a false-positive crossing detection with a one way merging into a bidi road in northgate_dual_carriageway, which I have edited into the output, so the test still fails.

complexity and conflict_level are nonsense for bike intersections

# Conflicts: # osm2streets/src/initial.rs # osm2streets/src/lib.rs

…indexes The indexes we had were indexing into a filtered road list. Road ids seems better anyway.

…removing a road

BudgieInWA

@dabreegster I'm ready for you to take a look at the implementation, and consider merging this in.

The complexity and conflict_level fields themselves might end up not actually being used in any calculations / rendering decisions, but they are useful now for debugging and for the tests. It's more likely to be the Major/Minor movement distinction, which I'm starting to work through in my head atm.

osm2streets/src/transform/classify_intersections.rs

BudgieInWA · 2022-11-14T09:23:40Z

osm2streets/src/render.rs

+
+        for (i, intersection) in &self.intersections {
+            // Find the points where the arrows should (leave, enter) the roads.
+            let road_points: BTreeMap<_, _> = intersection


I wanted to implement a method on Road that could calculate these offset endpoints (for a given end and direction). Because a Road doesn't know it's own DrivingSide or dividing line (!!), it's non-trivial, so I'm happy with this currently.

I want to figure out how a Road should figure out it's own driving side, dividing line and HalfRoads pretty soon. Maybe annotating contraflow lanes within LaneSpec would make it easy enough to calculate on demand from lanes_lrt, or maybe the driving side and dividing lane should be stored on the Road as part of the OSM parsing process (where we have the most context).

I agree it's annoying how often DrivingSide has to be plumbed around. Just so I understand, by diving line, you mean the point where the direction of travel changes (or the left/right edge on one-way roads)? Looking at pairs of lanes_ltr and direction should work for that, right? And that process exposes cases where there isn't one obvious dividing lane (bidi cycletracks, shared center turn lanes)

Maybe annotating contraflow lanes within LaneSpec

What do you mean by this? They have Direction already

Yes, that is what I mean by "dividing line". If contraflow lanes were annotated as "contraflow" from the beginning, then we wouldn't get confused when trying to find the dividing line. Knowing the driving side might be enough to come up with a consistent definition / detection algorithm though.

I'm open to trying either way. This is sounding like a good thing to unit test individually -- it's probably just a few weird edge cases I'm worrying about

BudgieInWA · 2022-11-14T09:40:27Z

osm2streets/src/lib.rs

@@ -86,21 +87,36 @@ impl StreetNetwork {
        for i in [id.i1, id.i2] {
            self.intersections.get_mut(&i).unwrap().roads.push(id);
            self.sort_roads(i);
+            // Recalculate movements and complexity.
+            let (complexity, conflict_level, movements) =
+                crate::transform::classify_intersections::guess_complexity(self, &i);


This sort of thing challenges my understanding of what should be a "transformation step" and what is an inherent part of the data types. Or at least which transformations are responsible for calculating data (that therefore need to be rerun when later transformations want to change things, such as merging intersections).

The "calculate change first, then commit them all" pattern that is forced upon us by the borrow checker in our current transformation implementations might actually be a better idea than calling this directly whenever a road is added. That is, maybe the responsibility for recalculating the movements etc. should be higher up than insert_road. At the level of merge_intersections(a, b) or higher.

Anyway, I've got a lot to understand yet about how the transformations fit together...

This sort of thing challenges my understanding of what should be a "transformation step" and what is an inherent part of the data types

I don't have clear ideas here either. :( I'm going to try to draw the flow chart of how everything gets created / set currently. The big hammer is to treat this as a big dataflow-type problem and consider automatically recalculating derived state when something changes. Or not do that automatically, but at least have a principled way to organize / reason about this. The problem is that changes percolate to related objects. If you do anything to a road, you need to recalculate geometry for two intersections. That calculation might trim back other roads one hop away. If there are transformations that look at trimmed road length, would we want to always go trigger them again? It gets confusing very quickly...

maybe the responsibility for recalculating the movements etc. should be higher up than insert_road. At the level of merge_intersections(a, b) or higher.

I like this way of thinking. Sometimes a very low-level, "surgical" set of mutations can happen. Or we use this "medium level" of abstraction. But another transformation may just want to say "merge short road" and automatically recalculate a bunch of derived state.

Or not do that automatically, but at least have a principled way to organize / reason about this [big dataflow-type problem].

This seems the likely best approach to me. At least for the code that takes the raw input to the naively "filled out" street representation.

My idea is that when you call StreetNetwork::new(osm_stuff) you might expect to get back Roads and Intersections that are complete, but blindly representing the raw data. There would be a handful of low-level conditions that are met ("all the geometry connects up properly to each other..."), and the other higher level conditions haven't been addressed yet ("... but some of it overlaps"). These are the bits of code that don't feel like "transformations" to me, more like "hydrating" the abstract raw descriptions ("this centerline with this placement and these lanes") into a useful concrete representation ("these lanes covering these areas").

Because the raw data is criminally under-specified we are expected to infer a bunch of stuff; under-represented so we want to guess/fill in a bunch of stuff; and imprecise so we want to correct a bunch of stuff. For example, if all intersection-internal ways were tagged junction=intersection (or whatever the tag is) then we wouldn't need a transformation to merge intersections together, we'd simply collect all the connected intersection nodes together before creating the Intersection.

These inferences, guesses and corrections are easier to make with the hydrated representation, so it makes sense to hydrate the raw data, calculate any inferences, propagate them back into the raw data then hydrate again. This is how I see "transformations" fitting in conceptually.

Actually modifying the osm data as the result of transformations sounds like a painful idea, but it feels like there's something there: StreetNetwork/Road/Intersection are like primitives, with some source-of-truth data (placement adjusted centerlines, connectivity, stop line positions) and some straightforwardly derived data (geometry, etc.). If StreetNetwork provided a rich enough set of operations (like collapsing a road (merging intersections), collapsing an intersection (merging roads), zipping/snapping adjacent roads, adding turn restrictions, moving centerlines/stop line positions, that sort of thing) then it could be used to manually draw beautifully accurate street networks (or a horrible mess). The problem of getting a beautiful street network out of raw osm data then becomes one of naively translating the osm spec into StreetNetwork primitives, then doing bunch of tweaking operations on them using osm-specific inferences/assumptions.

The StreetNetwork primitive operations could be efficiently implemented, maybe even in a way that allows batching (perform a bunch of operations and keep track of what is invalidated, then recalculate all the invalidated data). Then we could reason about the order of transformations, if they need repeating, even of multiple transformations can be committed at the same time.

I'm gonna let that marinate in my brain for a little while and look to talk with you about it later.

I just finished writing up #117 and this sounds so much better. The dataflow "framework" doesn't have to make the entire codebase complex; this batching executor could isolate it. I really agree at least two of the current transformations are something much more fundamental / less optional

dabreegster

I'm fine merging this in and making smaller incremental steps. We have enough ideas for refactors and new abstractions, and rebasing a long-lived PR gets tough. As long as nothing external using osm2streets currently gets affected, I prefer checking in experiments and leaving the results unused or flagging them off

Awesome leap forward!

osm2streets/src/lib.rs

dabreegster · 2022-11-14T17:14:42Z

osm2streets/src/lib.rs

@@ -86,21 +87,36 @@ impl StreetNetwork {
        for i in [id.i1, id.i2] {
            self.intersections.get_mut(&i).unwrap().roads.push(id);
            self.sort_roads(i);
+            // Recalculate movements and complexity.
+            let (complexity, conflict_level, movements) =
+                crate::transform::classify_intersections::guess_complexity(self, &i);


This sort of thing challenges my understanding of what should be a "transformation step" and what is an inherent part of the data types

I don't have clear ideas here either. :( I'm going to try to draw the flow chart of how everything gets created / set currently. The big hammer is to treat this as a big dataflow-type problem and consider automatically recalculating derived state when something changes. Or not do that automatically, but at least have a principled way to organize / reason about this. The problem is that changes percolate to related objects. If you do anything to a road, you need to recalculate geometry for two intersections. That calculation might trim back other roads one hop away. If there are transformations that look at trimmed road length, would we want to always go trigger them again? It gets confusing very quickly...

maybe the responsibility for recalculating the movements etc. should be higher up than insert_road. At the level of merge_intersections(a, b) or higher.

I like this way of thinking. Sometimes a very low-level, "surgical" set of mutations can happen. Or we use this "medium level" of abstraction. But another transformation may just want to say "merge short road" and automatically recalculate a bunch of derived state.

osm2streets/src/render.rs

dabreegster · 2022-11-14T17:35:37Z

osm2streets/src/transform/classify_intersections.rs

+    // detect if they diverge, merge, or cross.
+    // assert!(connections is sorted) so small_con large_con makes sense.
+    let mut each_con = connections.iter();
+    while let Some(small_con) = each_con.next() {


Just so I understand: conflict is symmetric, so we only need to calculate "half the matrix" of pairs, right? What do you mean by sorted / small and large connections? Literally the tuples of movement indices, ie, (1, 3) < (2, 1)?

Yes, spot on. I'll clarify this in the "every pair of connections" comment.

In fact, the logic doesn't depend on the sort order of small_con/large_con (it may have at one point). We also aren't doing anything with the full set of conflicts, so I'll omit storing them all here.

osm2streets/src/transform/classify_intersections.rs

…ted logic and document it

… in comment

dabreegster reviewed Sep 22, 2022

View reviewed changes

BudgieInWA mentioned this pull request Sep 27, 2022

Cache roads per intersection. #93 #98

Merged

BudgieInWA force-pushed the movement-conflicts branch from a27d99f to 0dc5a0b Compare October 20, 2022 08:53

BudgieInWA added 2 commits October 20, 2022 17:21

Start calculating movements and conflicts

e7919f6

still faulty due to road sort order and missing travel direction checks. We should be able to see on and off ramps, sausage links, etc. pretty clearly in the result when its working.

Store movements in Intersections and add a debug layer showing them

b5110af

BudgieInWA force-pushed the movement-conflicts branch from 0dc5a0b to b5110af Compare October 20, 2022 09:28

BudgieInWA added 5 commits November 5, 2022 13:49

untested: Calculate if movements using the same road in different dir…

fcdb296

…ections cross or not

Take into account the driving direction of the roads

76d32e0

Clear the list of movements when modifying an intersection

fa037f3

The movements become incorrect (and lead to crashes) when the number of roads in an intersection changes. The movements should just be recalculated (which might be easy, but I haven't reasoned it out yet).

Use terser syntax

93742f4

Improve comment about using indexes to refer to roads

670a890

BudgieInWA commented Nov 7, 2022

View reviewed changes

dabreegster reviewed Nov 7, 2022

View reviewed changes

osm2streets/src/transform/classify_intersections.rs Show resolved Hide resolved

BudgieInWA added 14 commits November 8, 2022 14:06

Merge branch 'main' into movement-conflicts

b8b9f21

Merge branch 'main' into movement-conflicts

aee6298

Extract the "can drive into/out of" logic into a fn

01347e6

Calculate movements for Connections (instead of assuming)

d872777

Fix an Uncontested case with bidi roads in calc_conflict

b75b2f2

Offset arrow start/end points from the center of bidi roads

156c79a

Document ConflictType and conflict_level

eaf8713

Pass around (&OriginalRoad, &Road) tuples instead of two lists

e9cbad0

Pre-filter non-drivable roads.

86432b4

complexity and conflict_level are nonsense for bike intersections

tmp: Add todos

e58eaa9

Merge branch 'main' into movement-conflicts

2c13409

# Conflicts: # osm2streets/src/initial.rs # osm2streets/src/lib.rs

Fix debug_movements_geojson now that InitialMap has gone away

24845f3

Make use of embedded road ids instead of passing around (id, road) pairs

ad8e7e8

BudgieInWA added 3 commits November 14, 2022 16:02

Represent movements stored in Intersection as roads ids instead or …

d46babd

…indexes The indexes we had were indexing into a filtered road list. Road ids seems better anyway.

Recalculate intersection movements and complexity after inserting or …

427aaea

…removing a road

cleanup use statements and old FIXME

479d636

BudgieInWA commented Nov 14, 2022

View reviewed changes

BudgieInWA requested a review from dabreegster November 14, 2022 09:46

dabreegster approved these changes Nov 14, 2022

View reviewed changes

BudgieInWA added 6 commits November 15, 2022 17:18

Introduce StreetNetwork::recalculate_movements(self, i) to hold repea…

19ab938

…ted logic and document it

Document IntersectionComplexity and note its awkwardness

34eecc2

Comment arrow offset variable

1bca406

Don't bother calculating all conflicts, and remove unneeded assertion…

beca662

… in comment

Fix comment typos

05def90

rust style improvements

ffe0e9d

BudgieInWA marked this pull request as ready for review November 15, 2022 10:16

BudgieInWA merged commit 323799c into main Nov 15, 2022

BudgieInWA deleted the movement-conflicts branch November 15, 2022 10:17

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start calculating Movements and Conflicts #96

Start calculating Movements and Conflicts #96

BudgieInWA commented Sep 22, 2022

BudgieInWA Nov 7, 2022

BudgieInWA commented Nov 7, 2022 •

edited

Loading

dabreegster commented Nov 7, 2022

BudgieInWA commented Nov 7, 2022

dabreegster left a comment

BudgieInWA left a comment

BudgieInWA Nov 14, 2022

dabreegster Nov 14, 2022

BudgieInWA Nov 15, 2022

dabreegster Nov 15, 2022

BudgieInWA Nov 14, 2022

dabreegster Nov 14, 2022

BudgieInWA Nov 15, 2022

dabreegster Nov 15, 2022

dabreegster left a comment

dabreegster Nov 14, 2022

dabreegster Nov 14, 2022

BudgieInWA Nov 15, 2022

BudgieInWA Nov 15, 2022

Start calculating Movements and Conflicts #96

Start calculating Movements and Conflicts #96

Conversation

BudgieInWA commented Sep 22, 2022

Choose a reason for hiding this comment

BudgieInWA commented Nov 7, 2022 • edited Loading

dabreegster commented Nov 7, 2022

BudgieInWA commented Nov 7, 2022

dabreegster left a comment

Choose a reason for hiding this comment

BudgieInWA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabreegster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BudgieInWA commented Nov 7, 2022 •

edited

Loading