Cache roads per intersection. #93

osm2streets-js/src/lib.rs

@@ -96,6 +96,13 @@ impl JsStreetNetwork {
             .map(|x| JsValue::from(JsDebugStreets { inner: x.clone() }))
             .collect()
     }
+
+    #[wasm_bindgen(js_name = debugClockwiseOrderingGeojson)]
+    pub fn debug_clockwise_ordering_geojson(&self) -> String {
+        self.inner
+            .debug_clockwise_ordering_geojson(&mut Timer::throwaway())
+            .unwrap()
+    }
 }
 
 #[wasm_bindgen]

osm2streets/src/lib.rs

@@ -159,28 +159,45 @@ impl StreetNetwork {
         }
     }
 
-    // TODO Might be better to maintain this instead of doing a search everytime.
-    pub fn roads_per_intersection(&self, i: osm::NodeID) -> Vec<OriginalRoad> {
-        let mut results = Vec::new();
-        for id in self.roads.keys() {
-            if id.i1 == i || id.i2 == i {
-                results.push(*id);
-            }
+    pub fn insert_road(&mut self, id: OriginalRoad, road: Road) {
+        self.roads.insert(id, road);
+        for i in [id.i1, id.i2] {
+            self.intersections.get_mut(&i).unwrap().roads.push(id);
+            self.sort_roads(i);
         }
-        results
     }
 
-    pub fn new_osm_node_id(&self, start: i64) -> osm::NodeID {
-        assert!(start < 0);
-        // Slow, but deterministic.
-        let mut osm_node_id = start;
-        loop {
-            if self.intersections.keys().any(|i| i.0 == osm_node_id) {
-                osm_node_id -= 1;
-            } else {
-                return osm::NodeID(osm_node_id);
+    pub fn remove_road(&mut self, id: &OriginalRoad) -> Road {
+        // Since the roads are already sorted, removing doesn't hurt anything
+        self.intersections
+            .get_mut(&id.i1)
+            .unwrap()
+            .roads
+            .retain(|r| r != id);
+        self.intersections
+            .get_mut(&id.i2)
+            .unwrap()
+            .roads
+            .retain(|r| r != id);
+        self.roads.remove(id).unwrap()
+    }
+
+    pub fn retain_roads<F: Fn(&OriginalRoad, &Road) -> bool>(&mut self, f: F) {
+        let mut remove = Vec::new();
+        for (id, r) in &self.roads {
+            if !f(id, r) {
+                remove.push(*id);
             }
         }
+        for id in remove {
+            self.remove_road(&id);
+        }
+    }
+
+    // This always returns roads oriented in clockwise order around the intersection
+    // TODO Consider not cloning. Many callers will have to change
+    pub fn roads_per_intersection(&self, i: osm::NodeID) -> Vec<OriginalRoad> {
+        self.intersections[&i].roads.clone()
     }
 
     /// (Intersection polygon, polygons for roads, list of labeled polygons to debug)
@@ -286,6 +303,60 @@ impl StreetNetwork {
                 .push((self.roads[&r].untrimmed_road_geometry().0, label.into()));
         }
     }
+
+    // Restore the invariant that an intersection's roads are ordered clockwise
+    //
+    // TODO This doesn't handle trim_roads_for_merging
+    fn sort_roads(&mut self, i: osm::NodeID) {
+        let intersection = self.intersections.get_mut(&i).unwrap();
+
+        // (ID, polyline pointing to the intersection, sorting point that's filled out later)
+        let mut road_centers = Vec::new();
+        let mut endpoints_for_center = Vec::new();
+        for r in &intersection.roads {
+            let road = &self.roads[r];
+            // road.center_pts is unadjusted; it doesn't handle unequal widths yet. But that
+            // shouldn't matter for sorting.
+            let center_pl = if r.i1 == i {
+                PolyLine::must_new(road.osm_center_points.clone()).reversed()
+            } else if r.i2 == i {
+                PolyLine::must_new(road.osm_center_points.clone())
+            } else {
+                panic!("Incident road {r} doesn't have an endpoint at {i}");
+            };
+            endpoints_for_center.push(center_pl.last_pt());
+
+            road_centers.push((*r, center_pl, Pt2D::zero()));
+        }
+        // In most cases, this will just be the same point repeated a few times, so Pt2D::center is a
+        // no-op. But when we have pretrimmed roads, this is much closer to the real "center" of the
+        // polygon we're attempting to create.
+        let intersection_center = Pt2D::center(&endpoints_for_center);
+
+        // Sort the road polylines in clockwise order around the center. This is subtle --
+        // https://a-b-street.github.io/docs/tech/map/geometry/index.html#sorting-revisited. When we
+        // get this wrong, the resulting polygon looks like a "bowtie," because the order of the
+        // intersection polygon's points follows this clockwise ordering of roads.
+        //
+        // We could use the point on each road center line farthest from the intersection center. But
+        // when some of the roads bend around, this produces incorrect ordering. Try walking along that
+        // center line a distance equal to the _shortest_ road.
+        let shortest_center = road_centers
+            .iter()
+            .map(|(_, pl, _)| pl.length())
+            .min()
+            .unwrap();
+        for (_, pl, sorting_pt) in &mut road_centers {
+            *sorting_pt = pl.must_dist_along(pl.length() - shortest_center).0;
+        }
+        road_centers.sort_by_key(|(_, _, sorting_pt)| {
+            sorting_pt
+                .angle_to(intersection_center)
+                .normalized_degrees() as i64
+        });
+
+        intersection.roads = road_centers.into_iter().map(|(r, _, _)| r).collect();
+    }
 }
 
 // Mutations and supporting queries
@@ -544,6 +615,10 @@ pub struct Intersection {
     pub control: ControlType,
     pub elevation: Distance,
 
+    /// All roads connected to this intersection. They may be incoming or outgoing relative to this
+    /// intersection. They're ordered clockwise aroundd the intersection.
+    pub roads: Vec<OriginalRoad>,
+
     // true if src_i matches this intersection (or the deleted/consolidated one, whatever)
     pub trim_roads_for_merging: BTreeMap<(osm::WayID, bool), Pt2D>,
 }
@@ -555,6 +630,7 @@ impl Intersection {
             complexity,
             control,
             // Filled out later
+            roads: Vec::new(),
             elevation: Distance::ZERO,
             trim_roads_for_merging: BTreeMap::new(),
         }

osm2streets/src/render.rs

@@ -223,6 +223,35 @@ impl StreetNetwork {
         let output = serde_json::to_string_pretty(&obj)?;
         Ok(output)
     }
+
+    /// For an intersection, show the clockwise ordering of roads around it
+    pub fn debug_clockwise_ordering_geojson(&self, timer: &mut Timer) -> Result<String> {
+        let initial_map = crate::initial::InitialMap::new(self, timer);
+
+        let mut pairs = Vec::new();
+
+        for (i, intersection) in &self.intersections {
+            for (idx, r) in intersection.roads.iter().enumerate() {
+                let pl = &initial_map.roads[r].trimmed_center_pts;
+                let pt = if r.i1 == *i {
+                    pl.first_pt()
+                } else {
+                    pl.last_pt()
+                };
+                pairs.push((
+                    pt.to_geojson(Some(&self.gps_bounds)),
+                    make_props(&[(
+                        "label",
+                        format!("{} / {}", idx + 1, intersection.roads.len()).into(),
+                    )]),
+                ));
+            }
+        }
+
+        let obj = geom::geometries_with_properties_to_geojson(pairs);
+        let output = serde_json::to_string_pretty(&obj)?;
+        Ok(output)
+    }
 }
 
 impl DebugStreets {

osm2streets/src/transform/collapse_intersections.rs

@@ -138,6 +138,7 @@ pub fn collapse_intersection(streets: &mut StreetNetwork, i: NodeID) {
     assert_eq!(roads.len(), 2);
     let mut r1 = roads[0];
     let mut r2 = roads[1];
+    assert_ne!(r1, r2);
 
     // We'll keep r1's way ID, so it's a little more convenient for debugging to guarantee r1 is
     // the longer piece.
@@ -157,11 +158,11 @@ pub fn collapse_intersection(streets: &mut StreetNetwork, i: NodeID) {
     }
 
     info!("Collapsing degenerate {}", i);
-    streets.intersections.remove(&i).unwrap();
     // We could be more careful merging percent_incline and osm_tags, but in practice, it doesn't
     // matter for the short segments we're merging.
-    let mut new_road = streets.roads.remove(&r1).unwrap();
-    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();
 
     // There are 4 cases, easy to understand on paper. Preserve the original direction of r1
     let (new_i1, new_i2) = if r1.i2 == r2.i1 {
@@ -195,7 +196,7 @@ pub fn collapse_intersection(streets: &mut StreetNetwork, i: NodeID) {
         i1: new_i1,
         i2: new_i2,
     };
-    streets.roads.insert(new_r1, new_road);
+    streets.insert_road(new_r1, new_road);
 
     // We may need to fix up turn restrictions. r1 and r2 both become new_r1.
     let rewrite = |x: &OriginalRoad| *x == r1 || *x == r2;
@@ -242,7 +243,7 @@ pub fn trim_deadends(streets: &mut StreetNetwork) {
     }
 
     for r in remove_roads {
-        streets.roads.remove(&r).unwrap();
+        streets.remove_road(&r);
     }
     for i in remove_intersections {
         streets.delete_intersection(i);

osm2streets/src/transform/merge_short_road.rs

@@ -25,7 +25,7 @@ impl StreetNetwork {
         if !self.intersections.contains_key(&short.i1)
             || !self.intersections.contains_key(&short.i2)
         {
-            self.roads.remove(&short).unwrap();
+            self.remove_road(&short);
             bail!(
                 "One endpoint of {} has already been deleted, skipping",
                 short
@@ -95,17 +95,10 @@ impl StreetNetwork {
                 .extend(trim_roads_for_merging);
         }
 
-        self.roads.remove(&short).unwrap();
+        self.remove_road(&short);
 
-        // Arbitrarily keep i1 and destroy i2. If the intersection types differ, upgrade the
-        // surviving interesting.
-        {
-            // Don't use delete_intersection; we're manually fixing up connected roads
-            let i = self.intersections.remove(&i2).unwrap();
-            if i.control == ControlType::TrafficSignal {
-                self.intersections.get_mut(&i1).unwrap().control = ControlType::TrafficSignal;
-            }
-        }
+        // Arbitrarily keep i1 and destroy i2. Don't actually remove the intersection until later;
+        // remove_road needs the intersection to exist
 
         // Fix up all roads connected to i2. Delete them and create a new copy; the ID changes,
         // since one intersection changes.
@@ -115,7 +108,7 @@ impl StreetNetwork {
         let mut new_to_old = BTreeMap::new();
         for r in self.roads_per_intersection(i2) {
             deleted.push(r);
-            let road = self.roads.remove(&r).unwrap();
+            let road = self.remove_road(&r);
             let mut new_id = r;
             if r.i1 == i2 {
                 new_id.i1 = i1;
@@ -133,10 +126,19 @@ impl StreetNetwork {
             old_to_new.insert(r, new_id);
             new_to_old.insert(new_id, r);
 
-            self.roads.insert(new_id, road);
+            self.insert_road(new_id, road);
             created.push(new_id);
         }
 
+        // If the intersection types differ, upgrade the surviving interesting.
+        {
+            // Don't use delete_intersection; we're manually fixing up connected roads
+            let i = self.intersections.remove(&i2).unwrap();
+            if i.control == ControlType::TrafficSignal {
+                self.intersections.get_mut(&i1).unwrap().control = ControlType::TrafficSignal;
+            }
+        }
+
         // If we're deleting the target of a simple restriction somewhere, update it.
         for (from_id, road) in &mut self.roads {
             let mut fix_trs = Vec::new();

osm2streets/src/transform/remove_disconnected.rs

@@ -49,15 +49,15 @@ pub fn remove_disconnected_roads(streets: &mut StreetNetwork) {
     for p in partitions.iter().skip(1) {
         for id in p {
             info!("Removing {} because it's disconnected from most roads", id);
-            streets.roads.remove(id).unwrap();
+            streets.remove_road(id);
             next_roads.remove(id.i1, *id);
             next_roads.remove(id.i2, *id);
         }
     }
 
     // Also remove cul-de-sacs here. TODO Support them properly, but for now, they mess up parking
     // hint matching (loop PolyLine) and pathfinding later.
-    streets.roads.retain(|id, _| id.i1 != id.i2);
+    streets.retain_roads(|id, _| id.i1 != id.i2);
 
     // Remove intersections without any roads
     streets

osm2streets/src/transform/sausage_links.rs

@@ -76,7 +76,7 @@ fn fix(streets: &mut StreetNetwork, id1: OriginalRoad, id2: OriginalRoad) {
     assert!(streets.roads.contains_key(&id2));
 
     // Arbitrarily remove the 2nd
-    let mut road2 = streets.roads.remove(&id2).unwrap();
+    let mut road2 = streets.remove_road(&id2);
     // And modify the 1st
     let road1 = streets.roads.get_mut(&id1).unwrap();
 

osm2streets/src/transform/snappy.rs

@@ -52,7 +52,7 @@ pub fn snap_cycleways(streets: &mut StreetNetwork) {
         snapped_ids.push(cycleway_id);
 
         // Remove the separate cycleway
-        let deleted_cycleway = streets.roads.remove(&cycleway_id).unwrap();
+        let deleted_cycleway = streets.remove_road(&cycleway_id);
 
         // Add it as an attribute to the roads instead
         for (road_id, dir) in roads {

street-explorer/www/js/main.js

@@ -138,7 +138,8 @@ class TestCase {
 
       importButton.innerText = "Importing OSM data...";
 
-      const drivingSide = app.getImportSettings().drivingSideForNewImports || "Right";
+      const drivingSide =
+        app.getImportSettings().drivingSideForNewImports || "Right";
 
       importOSM("Imported area", app, osmInput, drivingSide, true);
       const bounds = app.layers
@@ -196,7 +197,7 @@ function importOSM(groupName, app, osmXML, drivingSide, addOSMLayer) {
       dual_carriageway_experiment: !!importSettings.dualCarriagewayExperiment,
       cycletrack_snapping_experiment:
         !!importSettings.cycletrackSnappingExperiment,
-      inferred_sidewalks: importSettings.sidewalks === 'infer',
+      inferred_sidewalks: importSettings.sidewalks === "infer",
       osm2lanes: !!importSettings.osm2lanes,
     });
     var group = new LayerGroup(groupName, app.map);
@@ -212,6 +213,9 @@ function importOSM(groupName, app, osmXML, drivingSide, addOSMLayer) {
       "Lane markings",
       makeLaneMarkingsLayer(network.toLaneMarkingsGeojson())
     );
+    group.addLazyLayer("Debug road ordering", () =>
+      makeDebugLayer(network.debugClockwiseOrderingGeojson())
+    );
     // TODO Graphviz hits `ReferenceError: can't access lexical declaration 'graph' before initialization`
 
     const numDebugSteps = network.getDebugSteps().length;
@@ -235,6 +239,11 @@ function importOSM(groupName, app, osmXML, drivingSide, addOSMLayer) {
       group.addLazyLayer("Lane markings", () =>
         makeLaneMarkingsLayer(step.getNetwork().toLaneMarkingsGeojson())
       );
+      // TODO Can we disable by default in a group? This one is very noisy, but
+      // could be useful to inspect
+      /*group.addLazyLayer("Debug road ordering", () =>
+        makeDebugLayer(step.getNetwork().debugClockwiseOrderingGeojson())
+      );*/
 
       const debugGeojson = step.toDebugGeojson();
       if (debugGeojson) {