Cache roads per intersection. #93 #98

dabreegster · 2022-09-26T04:04:45Z

Since inserting or removing a road in the graph now requires maintaining state in intersections, we have to be careful and use the new methods that keep everything correct. I hit a few hard-to-debug problems where the order of operations in some transformations matters... you have to delete the road first, then the intersection. Otherwise, remove_road will be upset that one of the intersections doesn't exist.

This is still broken -- two tests still failing for reasons I'm having trouble debugging. The complexity of the code and transformations increases a bit with this change, but I think it's a bit necessary. The previous brute-force roads_per_intersection search was really inefficient.

Once I fix the last bug, I plan to add clockwise ordering using the original OSM center line. When we merge short roads, this'll have some occasional brokenness maybe, but will see

Broken!

BudgieInWA · 2022-09-27T06:11:54Z

osm2streets/src/lib.rs

+    /// All roads connected to this intersection. They may be incoming or outgoing relative to this
+    /// intersection.
+    /// TODO: Guarantee they're sorted clockwise
+    pub roads: Vec<OriginalRoad>,


Is there a good type that has an insert_at to take advantage of the fact that roads is already sorted?

Like https://doc.rust-lang.org/std/vec/struct.Vec.html#method.insert ? When we're adding a new road, we could calculate its position in the list and add it, or just recalculate the ordering for the intersection. It'll depend on the sorting algorithm

Right. Performance doesn't really matter, so Vec works if we want to do it the "find and insert" way.

BudgieInWA · 2022-09-27T06:21:56Z

Great to see! Sorted roads on all intersections will unlock movements in #96, so this progress is exciting.

BudgieInWA · 2022-09-27T06:25:31Z

This is my approach to sorting roads, if it is decipherable:

test fails

No more crashes, but lots of test diffs. From a quick spot check, no visible difference, but will dig in later.

…one use right now, in clip If later transformations need something like this, we could reintroduce. But I suspect we'd always just re-use a real OSM ID from something being merged / transformed.

code

dabreegster · 2022-10-01T12:15:56Z

OK @BudgieInWA, this is ready for review! The changes:

Intersection now stores Vec<OriginalRoad>, sorted in clockwise order
There are new methods like insert_road, remove_road, retain_roads that maintain invariants. They must be used, and carefully. If you follow through the commits in order, you'll see me making some errors with the order things are called, or forgetting to call things, and the resulting bugs were not the easiest to track down.
I rewrote the logic that clips the imported OSM data to the boundary polygon entirely. It was working in a too-crazy way before, because when an intersection is out-of-bounds but connected to multiple roads, we clone the intersection and reassign IDs. The IDs changing was complicated to reason about now that we store derived state in more places. I think the newer logic is easier to follow.
I added a debug layer to show the clockwise sorting in the web UI. We can probably remove it after we're confident sorting works.

There are lots of test diffs. Internal IDs change a bit, especially the negative ones that're created by clipping. Some coordinates also change. I visually diffed all of the cases and they all look identical, except for northgate, where a long disconnected piece of light rail gets clipped a bit differently.

The clockwise sorting is probably imperfect, but all the cases I've looked at are fine:

I wanted to simplify the logic in geometry/algorithm.rs to use the existing clockwise sorting, but I hit a bunch of bugs I don't understand yet. I'll try again and follow up with a smaller change. I think this PR should be enough to unblock the movements work though!

dabreegster · 2022-10-01T12:18:14Z

osm2streets/src/lib.rs

+
+    // Restore the invariant that an intersection's roads are ordered clockwise
+    //
+    // TODO This doesn't handle trim_roads_for_merging


In cases where we remove a short road and merge intersections, sorting around a "center" point gets kind of funny. geometry/algorithm.rs takes care to try and handle that. I didn't port the same logic here yet.

I think the best way to determine the order of roads in those multi-node intersections from scratch is to use the OSM graph structure. First sorting the nodes (that are the Road endpoints) then by bearing of the first OSM segment. #98 (comment)

This would require keeping the OSM graph around in some capacity, at least within multi-node intersections.

I agree with keeping the graph structure around somehow. Preserving turn restrictions when we merge gets very complex, and a possibly simpler approach is to use the original graph and just ask questions about it. Keeping the graph also means we can map back to objects in OSM more directly.

I don't know how this would work, so shelving the idea for the moment, but you've definitely got me thinking...

I like "Asking questions about it". In fact, load and leave the osm graph completely in place. Every road should get it's own copy of the geom (like when you merge similar ways currently) which can be promptly offset to account for placement. Then further tweaked if needed (like sausage links). (Think of an editor, where the graph is literally drawn on the map. One could point at a lane to edit the way.)

(Roads and intersections need to have IDs independant of the OSM ids they reference, [and they need to know their own ID, so we don't need to pass pairs eveywhere])

Asking questions about roads is how I'm thinking about movements too. It's like I want to iterate flows of traffic clockwise around the intersection. A way could yield a single (oneway) flow, or commonly two. An intersection provides an iter_cw_flows() -> Flow, where Flow is a view onto a subset of lanes on a Road (that all go in the same direction).

I think that's how the type system helps me out here: There exists a whole lot of data about a whole lot of lanes, laid out in memory in one way or another, I want to slice it up differently, with differently typed structs, for different tasks. It's like the HashMap Entry type.

Keeping an immutable copy of the OSM graph and making the Street graph independent will no doubt become GH Issues to discuss and target soon.

dabreegster · 2022-10-01T12:19:17Z

osm2streets/src/transform/collapse_intersections.rs

-    let mut road2 = streets.roads.remove(&r2).unwrap();
+    let mut new_road = streets.remove_road(&r1);
+    let mut road2 = streets.remove_road(&r2);
+    streets.intersections.remove(&i).unwrap();


Order matters! remove_road cleans up the two intersections, so they need to both exist until the end

BudgieInWA

The results look good. I like the idea of merging this, even if the order isn't correct all of the time. Definitely good enough to start experimenting with.

I've only had a cursory look over the code, and wont have much time for movement calculations this week, so I'm happy to see this go ahead!

dabreegster · 2022-10-02T07:40:17Z

Cool, then I'll merge! Based on approaching timelines for some work stuff, I'm about to start an issue and some experimentation with pedestrian/bike crossings. Not sure yet how things will fit into the movement PR that's started, but we'll find out I guess

… Detected when trying to import larger areas in A/B Street, where turn restrictions happened to cross a boundary

from #98.

WIP: Cache roads per intersection. #93

ef623fb

Broken!

BudgieInWA reviewed Sep 27, 2022

View reviewed changes

dabreegster added 7 commits September 30, 2022 13:16

Fix a bug with caching roads per intersection and clipping. Now just one

e91e94b

test fails

Rewrite the clipping logic entirely to manage road IDs more sanely.

f4b5f67

No more crashes, but lots of test diffs. From a quick spot check, no visible difference, but will dig in later.

Get rid of the over-complicated new_osm_node_id method; there's only …

9cb1679

…one use right now, in clip If later transformations need something like this, we could reintroduce. But I suspect we'd always just re-use a real OSM ID from something being merged / transformed.

Start debugging the road sort order in the UI

b16ca54

Actually sort roads, copying the approach from the intersection polygon

b888748

code

Fix a crash in clipping with the northgate test

84db6c4

Accept changes in tests from #93

e6de03c

dabreegster marked this pull request as ready for review October 1, 2022 12:08

dabreegster commented Oct 1, 2022

View reviewed changes

BudgieInWA self-requested a review October 2, 2022 07:13

BudgieInWA approved these changes Oct 2, 2022

View reviewed changes

dabreegster merged commit 973baa1 into main Oct 2, 2022

dabreegster deleted the roads_per branch October 2, 2022 07:40

dabreegster added a commit that referenced this pull request Oct 3, 2022

Fix bugs when matching turn restrictions to roads, introduced by #98.…

706880f

… Detected when trying to import larger areas in A/B Street, where turn restrictions happened to cross a boundary

dabreegster added a commit that referenced this pull request Oct 4, 2022

Fix a slight bug when clipping long light rail roads to the boundary,

93567f8

from #98.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cache roads per intersection. #93 #98

Cache roads per intersection. #93 #98

dabreegster commented Sep 26, 2022

BudgieInWA Sep 27, 2022

dabreegster Sep 27, 2022

BudgieInWA Sep 27, 2022

BudgieInWA commented Sep 27, 2022

BudgieInWA commented Sep 27, 2022

dabreegster commented Oct 1, 2022

dabreegster Oct 1, 2022

BudgieInWA Oct 2, 2022

dabreegster Oct 2, 2022

BudgieInWA Oct 6, 2022

dabreegster Oct 1, 2022

BudgieInWA left a comment

dabreegster commented Oct 2, 2022

Cache roads per intersection. #93 #98

Cache roads per intersection. #93 #98

Conversation

dabreegster commented Sep 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BudgieInWA commented Sep 27, 2022

BudgieInWA commented Sep 27, 2022

dabreegster commented Oct 1, 2022

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 left a comment

Choose a reason for hiding this comment

dabreegster commented Oct 2, 2022