A few traffic signal cleanups: (#512)

- Stop alerting when slow pedestrians can't make it through the minimum
  crosswalk time
- Simpler iteration style in lagging_green.rs
- Totally delete the old brute force signal config code; it never worked
  well, and the improved heuristics eliminate the need for it anyway
- Make a Duration::max function and use it in one case

game/src/debug/mod.rs

@@ -729,13 +729,12 @@ impl ContextualActions for Actions {
 }
 
 fn find_bad_signals(app: &App) {
-    println!("Bad traffic signals:");
+    error!("Bad traffic signals:");
     for i in app.primary.map.all_intersections() {
         if i.is_traffic_signal() {
             let first = &ControlTrafficSignal::get_possible_policies(&app.primary.map, i.id)[0].0;
             if first == "stage per road" || first == "arbitrary assignment" {
-                println!("- {}", i.id);
+                error!("- {}", i.id);
-                ControlTrafficSignal::brute_force(&app.primary.map, i.id);
             }
         }
     }

geom/src/duration.rs

@@ -222,6 +222,24 @@ impl Duration {
         }
         s
     }
+
+    /// Returns the largest of the two inputs.
+    pub fn max(self, other: Duration) -> Duration {
+        if self >= other {
+            self
+        } else {
+            other
+        }
+    }
+
+    /// Returns the smallest of the two inputs.
+    pub fn min(self, other: Duration) -> Duration {
+        if self <= other {
+            self
+        } else {
+            other
+        }
+    }
 }
 
 impl std::fmt::Display for Duration {

map_model/src/make/traffic_signals/brute_force.rs

@@ -1,39 +0,0 @@
-use super::*;
-
-/// Temporary experiment to group all movements into the smallest number of stages.
-pub fn make_traffic_signal(map: &Map, i: IntersectionID) {
-    let movements: Vec<Movement> = Movement::for_i(i, map)
-        .unwrap()
-        .into_iter()
-        .filter_map(|(id, m)| if id.crosswalk { None } else { Some(m) })
-        .collect();
-    let indices: Vec<usize> = (0..movements.len()).collect();
-    for num_stages in 1..=movements.len() {
-        println!(
-            "For {} turn movements, looking for solution with {} stages",
-            movements.len(),
-            num_stages
-        );
-        for partition in helper(&indices, num_stages) {
-            if okay_partition(movements.iter().collect(), partition) {
-                return;
-            }
-        }
-    }
-    unreachable!()
-}
-
-fn okay_partition(movements: Vec<&Movement>, partition: Partition) -> bool {
-    for stage in partition.0 {
-        let mut protected: Vec<&Movement> = Vec::new();
-        for idx in stage {
-            let m = movements[idx];
-            if protected.iter().any(|other| m.conflicts_with(other)) {
-                return false;
-            }
-            protected.push(m);
-        }
-    }
-    println!("found one that works! :O");
-    true
-}

map_model/src/make/traffic_signals/lagging_green.rs

@@ -1,6 +1,5 @@
 use super::*;
 
-///
 /// Create a traffic signal which has a stage that is: protected straight, protected right,
 /// unprotected left, unprotected right on red. Followed by a variable stage that has protected
 /// left, unprotected right on red. With a last stage that is all-walk and variable.
@@ -57,15 +56,11 @@ fn optimize(mut ts: ControlTrafficSignal) -> Option<ControlTrafficSignal> {
 fn make_crosswalk_variable(ts: &mut ControlTrafficSignal) {
     const MIN_CROSSWALK_TIME: Duration = Duration::const_seconds(15.0);
     for mut s in ts.stages.iter_mut() {
-        if let Some(mut duration) = s.max_crosswalk_time(&ts.movements) {
+        if let Some(duration) = s.max_crosswalk_time(&ts.movements) {
             match s.stage_type {
                 StageType::Fixed(_) => {
-                    // get the minimum duration, but never less than 15 seconds
-                    if duration < MIN_CROSSWALK_TIME {
-                        duration = MIN_CROSSWALK_TIME;
-                    }
                     s.stage_type = StageType::Variable(
-                        duration,
+                        duration.max(MIN_CROSSWALK_TIME),
                         Duration::const_seconds(1.0),
                         Duration::const_seconds(1.0),
                     )
@@ -157,20 +152,20 @@ fn multi_way_stages(map: &Map, id: IntersectionID) -> Option<ControlTrafficSigna
         // Insert the straight movements, followed by the right and then the left.
         stage1.protected_movements.insert(m1.clone());
         stage1.protected_movements.insert(m2.clone());
-        turns(&m1.from.id, &right).iter().for_each(|t| {
+        stage1
-            stage1.protected_movements.insert(*t);
+            .protected_movements
-        });
+            .extend(turns(&m1.from.id, &right));
-        turns(&m2.from.id, &right).iter().for_each(|t| {
+        stage1
-            stage1.protected_movements.insert(*t);
+            .protected_movements
-        });
+            .extend(turns(&m2.from.id, &right));
-        turns(&m1.from.id, &left).iter().for_each(|t| {
+        for t in turns(&m1.from.id, &left) {
-            stage1.yield_movements.insert(*t);
+            stage1.yield_movements.insert(t);
-            stage2.protected_movements.insert(*t);
+            stage2.protected_movements.insert(t);
-        });
+        }
-        turns(&m2.from.id, &left).iter().for_each(|t| {
+        for t in turns(&m2.from.id, &left) {
-            stage1.yield_movements.insert(*t);
+            stage1.yield_movements.insert(t);
-            stage2.protected_movements.insert(*t);
+            stage2.protected_movements.insert(t);
-        });
+        }
         add_stage(&mut ts, stage1);
         if let Some((m1, m2)) = is_conflict(&ts, &stage2) {
             // We've hit the case where oncoming left turns can't both be protected.
@@ -203,33 +198,25 @@ fn multi_way_stages(map: &Map, id: IntersectionID) -> Option<ControlTrafficSigna
             }
 
             // add right turns
-            turns(&r1, &right).iter().for_each(|t| {
+            stage1.protected_movements.extend(turns(&r1, &right));
-                stage1.protected_movements.insert(*t);
+            stage1.protected_movements.extend(turns(&r2, &right));
-            });
-            turns(&r2, &right).iter().for_each(|t| {
-                stage1.protected_movements.insert(*t);
-            });
 
             // add left turns
-            turns(&r1, &left).iter().for_each(|t| {
+            for t in turns(&r1, &left) {
-                stage1.yield_movements.insert(*t);
+                stage1.yield_movements.insert(t);
-                stage2.protected_movements.insert(*t);
+                stage2.protected_movements.insert(t);
-            });
+            }
-            turns(&r2, &left).iter().for_each(|t| {
+            for t in turns(&r2, &left) {
-                stage1.yield_movements.insert(*t);
+                stage1.yield_movements.insert(t);
-                stage2.protected_movements.insert(*t);
+                stage2.protected_movements.insert(t);
-            });
+            }
             // add the stages
             add_stage(&mut ts, stage1);
             add_stage(&mut ts, stage2);
         } else {
             // single stage without lagging left turns
-            turns(&r1, &right).iter().for_each(|t| {
+            stage1.protected_movements.extend(turns(&r1, &right));
-                stage1.protected_movements.insert(*t);
+            stage1.protected_movements.extend(turns(&r1, &left));
-            });
-            turns(&r1, &left).iter().for_each(|t| {
-                stage1.protected_movements.insert(*t);
-            });
             add_stage(&mut ts, stage1);
         }
     }

map_model/src/make/traffic_signals/mod.rs

@@ -13,7 +13,6 @@ use crate::{
 };
 use geom::Duration;
 
-pub mod brute_force;
 mod lagging_green;
 
 /// Applies a bunch of heuristics to a single intersection, returning the valid results in
@@ -446,41 +445,6 @@ fn make_stages_filtered(
     }
 }
 
-// Technically, a set of sets; order doesn't matter
-#[derive(Clone)]
-struct Partition(Vec<Vec<usize>>);
-
-// Extremely hasty port of https://stackoverflow.com/a/30903689
-fn helper(items: &[usize], max_size: usize) -> Vec<Partition> {
-    if items.len() < max_size || max_size == 0 {
-        return Vec::new();
-    }
-
-    if max_size == 1 {
-        return vec![Partition(vec![items.to_vec()])];
-    }
-
-    let mut results = Vec::new();
-    let prev1 = helper(&items[0..items.len() - 1], max_size);
-    for i in 0..prev1.len() {
-        for j in 0..prev1[i].0.len() {
-            let mut partition: Vec<Vec<usize>> = Vec::new();
-            for inner in &prev1[i].0 {
-                partition.push(inner.clone());
-            }
-            partition[j].push(*items.last().unwrap());
-            results.push(Partition(partition));
-        }
-    }
-
-    let set = vec![*items.last().unwrap()];
-    for mut partition in helper(&items[0..items.len() - 1], max_size - 1) {
-        partition.0.push(set.clone());
-        results.push(partition);
-    }
-    results
-}
-
 /// Simple second-pass after generating all signals. Find pairs of traffic signals very close to
 /// each other with 2 stages each, see if the primary movement of the first stages lead to each
 /// other, and flip the order of stages if not. This is often wrong when the most common movement is

map_model/src/objects/traffic_signals.rs

@@ -7,7 +7,7 @@ use serde::{Deserialize, Serialize};
 use abstutil::{deserialize_btreemap, retain_btreeset, serialize_btreemap};
 use geom::{Distance, Duration, Speed};
 
-use crate::make::traffic_signals::{brute_force, get_possible_policies};
+use crate::make::traffic_signals::get_possible_policies;
 use crate::raw::OriginalRoad;
 use crate::{
     osm, CompressedMovementID, DirectedRoadID, Direction, IntersectionID, Map, Movement,
@@ -89,10 +89,6 @@ impl ControlTrafficSignal {
         // signal config.
         get_possible_policies(map, id, false)
     }
-    // TODO tmp
-    pub fn brute_force(map: &Map, id: IntersectionID) {
-        brute_force::make_traffic_signal(map, id)
-    }
 
     pub fn get_min_crossing_time(&self, idx: usize) -> Duration {
         let mut max_distance = Distance::meters(0.0);

sim/src/mechanics/intersection.rs

@@ -839,16 +839,10 @@ impl IntersectionSimState {
         // it wrong, that's fine -- block the box a bit.
         let time_to_cross = turn.geom.length() / speed;
         if time_to_cross > remaining_stage_time {
-            // Actually, we might have bigger problems...
+            // Signals enforce a minimum crosswalk time, but some pedestrians are configured to
-            if time_to_cross > full_stage_duration {
+            // walk very slowly. In that case, allow them to go anyway and wind up in the crosswalk
-                self.events.push(Event::Alert(
+            // during a red. This matches reality reasonably.
-                    AlertLocation::Intersection(req.turn.parent),
+            if time_to_cross <= full_stage_duration {
-                    format!(
-                        "{:?} is impossible to fit into stage duration of {}",
-                        req, full_stage_duration
-                    ),
-                ));
-            } else {
                 return false;
             }
         }