Remove the --pathfinding_upfront option. #312 and #258

Originally, all trips in the entire scenario had their paths calculated
at the beginning of a simulation. The sum time is faster than
calculating them individually, because we could use multiple threads.
But a while ago, this was disabled by default to improve the startup
latency. Especially if a player isn't making it through an entire day
anyway, calculating all of the paths upfront is a waste and slows down
their initial experience.

Now I'm hitting all sorts of bugs with live map edits, because the map
change between initial planning and when a trip starts. Previously, the
PathRequest was calculated up-front and resolved when a trip starts. But
even this PathRequest can become stale. So now always calculate it when
a trip actually starts, looking at the current map. To do this sanely,
totally rip out support for --pathfinding_upfront.

If we really want it later for performance, we could add it back in, and
be very careful about detecting stale PathRequests and recomputing. But
for now, there's no use case for this, so it'd needlessly complicate the
code.

book/src/trafficsim/live_edits.md

@@ -22,14 +22,17 @@ more stages.
 
 ## TODO: Recalculating paths
 
-Many of the edits will influence routes. For trips that haven't started yet, as
-long as `pathfinding_upfront` is disabled (currently the default), there's
-nothing to do. For currently active trips, in some cases, rerouting would be
-ideal but not necessary (like if speed limits changed). In other cases -- like
-changing access restrictions, modifying lane types, closing intersections --
-the route must be recomputed. As a simple first attempt, we could just cancel
-all active trips whose path crosses an edited road or intersection. Later, we
-can figure out rerouting.
+Many of the edits will influence routes. For trips that haven't started yet,
+there's nothing to do immediately. Paths are calculated right before the trip
+starts, so slight changes to the start/end of the path due to map edits (like
+where somebody starts biking, for example) are captured naturally.
+
+For currently active trips, in some cases, rerouting would be ideal but not
+necessary (like if speed limits changed). In other cases -- like changing access
+restrictions, modifying lane types, closing intersections -- the route must be
+recomputed. As a simple first attempt, we could just cancel all active trips
+whose path crosses an edited road or intersection. Later, we can figure out
+rerouting.
 
 And actually, the only other case to handle is `ChangeRouteSchedule`, which
 should just be rescheduling the `StartBus` commands.

sim/src/make/spawner.rs

@@ -3,7 +3,7 @@
 
 use serde::{Deserialize, Serialize};
 
-use abstutil::{Parallelism, Timer};
+use abstutil::Timer;
 use geom::{Duration, Time};
 use map_model::{
     BuildingID, BusRouteID, BusStopID, IntersectionID, Map, PathConstraints, PathRequest, Position,
@@ -205,31 +205,8 @@ impl TripSpawner {
         scheduler: &mut Scheduler,
         timer: &mut Timer,
     ) {
-        let pathfinding_upfront = trips.pathfinding_upfront;
-        let paths = timer.parallelize(
-            "calculate paths",
-            Parallelism::Fastest,
-            std::mem::replace(&mut self.trips, Vec::new()),
-            |tuple| {
-                let req = tuple.2.get_pathfinding_request(map);
-                (
-                    tuple,
-                    req.clone(),
-                    if pathfinding_upfront {
-                        req.and_then(|r| map.pathfind(r))
-                    } else {
-                        None
-                    },
-                )
-            },
-        );
-
-        timer.start_iter("spawn trips", paths.len());
-        for (
-            (p, start_time, spec, trip_start, purpose, cancelled, modified),
-            maybe_req,
-            maybe_path,
-        ) in paths
+        timer.start_iter("spawn trips", self.trips.len());
+        for (p, start_time, spec, trip_start, purpose, cancelled, modified) in self.trips.drain(..)
         {
             timer.next();
 
@@ -387,10 +364,7 @@ impl TripSpawner {
                     format!("traffic pattern modifier cancelled this trip"),
                 );
             } else {
-                scheduler.push(
-                    start_time,
-                    Command::StartTrip(trip, spec, maybe_req, maybe_path),
-                );
+                scheduler.push(start_time, Command::StartTrip(trip, spec));
             }
         }
     }

sim/src/scheduler.rs

@@ -17,7 +17,7 @@ pub enum Command {
     /// If true, retry when there's no room to spawn somewhere
     SpawnCar(CreateCar, bool),
     SpawnPed(CreatePedestrian),
-    StartTrip(TripID, TripSpec, Option<PathRequest>, Option<Path>),
+    StartTrip(TripID, TripSpec),
     UpdateCar(CarID),
     /// Distinguish this from UpdateCar to avoid confusing things
     UpdateLaggyHead(CarID),
@@ -43,7 +43,7 @@ impl Command {
         match self {
             Command::SpawnCar(ref create, _) => CommandType::Car(create.vehicle.id),
             Command::SpawnPed(ref create) => CommandType::Ped(create.id),
-            Command::StartTrip(id, _, _, _) => CommandType::StartTrip(*id),
+            Command::StartTrip(id, _) => CommandType::StartTrip(*id),
             Command::UpdateCar(id) => CommandType::Car(*id),
             Command::UpdateLaggyHead(id) => CommandType::CarLaggyHead(*id),
             Command::UpdatePed(id) => CommandType::Ped(*id),
@@ -59,7 +59,7 @@ impl Command {
         match self {
             Command::SpawnCar(_, _) => SimpleCommandType::Car,
             Command::SpawnPed(_) => SimpleCommandType::Ped,
-            Command::StartTrip(_, _, _, _) => SimpleCommandType::StartTrip,
+            Command::StartTrip(_, _) => SimpleCommandType::StartTrip,
             Command::UpdateCar(_) => SimpleCommandType::Car,
             Command::UpdateLaggyHead(_) => SimpleCommandType::CarLaggyHead,
             Command::UpdatePed(_) => SimpleCommandType::Ped,

sim/src/sim/mod.rs

@@ -95,9 +95,6 @@ pub struct SimOptions {
     pub enable_pandemic_model: Option<XorShiftRng>,
     /// When a warning is encountered during simulation, specifies how to respond.
     pub alerts: AlertHandler,
-    /// At the beginning of the simulation, precompute the route for all trips for the entire
-    /// scenario.
-    pub pathfinding_upfront: bool,
     /// Ignore parking data in the map and instead treat every building as if it has unlimited
     /// capacity for vehicles.
     pub infinite_parking: bool,
@@ -144,7 +141,6 @@ impl SimOptions {
                     _ => panic!("Bad --alerts={}. Must be print|block|silence", x),
                 })
                 .unwrap_or(AlertHandler::Print),
-            pathfinding_upfront: args.enabled("--pathfinding_upfront"),
             infinite_parking: args.enabled("--infinite_parking"),
             disable_turn_conflicts: args.enabled("--disable_turn_conflicts"),
             cancel_drivers_delay_threshold: args
@@ -181,7 +177,6 @@ impl SimOptions {
             handle_uber_turns: true,
             enable_pandemic_model: None,
             alerts: AlertHandler::Print,
-            pathfinding_upfront: false,
             infinite_parking: false,
             disable_turn_conflicts: false,
             cancel_drivers_delay_threshold: None,
@@ -201,7 +196,7 @@ impl Sim {
             intersections: IntersectionSimState::new(map, &mut scheduler, &opts),
             transit: TransitSimState::new(map),
             cap: CapSimState::new(map, &opts),
-            trips: TripManager::new(opts.pathfinding_upfront),
+            trips: TripManager::new(),
             pandemic: if let Some(rng) = opts.enable_pandemic_model {
                 Some(PandemicModel::new(rng))
             } else {
@@ -436,9 +431,8 @@ impl Sim {
         };
 
         match cmd {
-            Command::StartTrip(id, trip_spec, maybe_req, maybe_path) => {
-                self.trips
-                    .start_trip(self.time, id, trip_spec, maybe_req, maybe_path, &mut ctx);
+            Command::StartTrip(id, trip_spec) => {
+                self.trips.start_trip(self.time, id, trip_spec, &mut ctx);
             }
             Command::SpawnCar(create_car, retry_if_no_room) => {
                 // create_car contains a Path, which is expensive to clone. We need different parts

sim/src/trips.rs

@@ -5,8 +5,7 @@ use serde::{Deserialize, Serialize};
 use abstutil::{deserialize_btreemap, serialize_btreemap, Counter};
 use geom::{Duration, Speed, Time};
 use map_model::{
-    BuildingID, BusRouteID, BusStopID, IntersectionID, Map, Path, PathConstraints, PathRequest,
-    Position,
+    BuildingID, BusRouteID, BusStopID, IntersectionID, Map, PathConstraints, PathRequest, Position,
 };
 
 use crate::sim::Ctx;
@@ -35,7 +34,6 @@ pub struct TripManager {
     )]
     active_trip_mode: BTreeMap<AgentID, TripID>,
     unfinished_trips: usize,
-    pub pathfinding_upfront: bool,
 
     car_id_counter: usize,
 
@@ -43,7 +41,7 @@ pub struct TripManager {
 }
 
 impl TripManager {
-    pub fn new(pathfinding_upfront: bool) -> TripManager {
+    pub fn new() -> TripManager {
         TripManager {
             trips: Vec::new(),
             people: Vec::new(),
@@ -51,7 +49,6 @@ impl TripManager {
             unfinished_trips: 0,
             car_id_counter: 0,
             events: Vec::new(),
-            pathfinding_upfront,
         }
     }
 
@@ -1018,7 +1015,7 @@ impl TripManager {
         if person.delayed_trips.is_empty() {
             return;
         }
-        let (trip, spec, maybe_req, maybe_path) = person.delayed_trips.remove(0);
+        let (trip, spec) = person.delayed_trips.remove(0);
         if false {
             self.events.push(Event::Alert(
                 AlertLocation::Person(person.id),
@@ -1028,22 +1025,11 @@ impl TripManager {
                 ),
             ));
         }
-        self.start_trip(now, trip, spec, maybe_req, maybe_path, ctx);
+        self.start_trip(now, trip, spec, ctx);
     }
 
-    pub fn start_trip(
-        &mut self,
-        now: Time,
-        trip: TripID,
-        spec: TripSpec,
-        maybe_req: Option<PathRequest>,
-        mut maybe_path: Option<Path>,
-        ctx: &mut Ctx,
-    ) {
+    pub fn start_trip(&mut self, now: Time, trip: TripID, spec: TripSpec, ctx: &mut Ctx) {
         assert!(self.trips[trip.0].info.cancellation_reason.is_none());
-        if !self.pathfinding_upfront && maybe_path.is_none() && maybe_req.is_some() {
-            maybe_path = ctx.map.pathfind(maybe_req.clone().unwrap());
-        }
 
         let person = &mut self.people[self.trips[trip.0].person.0];
         if let PersonState::Trip(_) = person.state {
@@ -1057,9 +1043,7 @@ impl TripManager {
                     ),
                 ));
             }
-            person
-                .delayed_trips
-                .push((trip, spec, maybe_req, maybe_path));
+            person.delayed_trips.push((trip, spec));
             self.events.push(Event::TripPhaseStarting(
                 trip,
                 person.id,
@@ -1070,6 +1054,10 @@ impl TripManager {
         }
         self.trips[trip.0].started = true;
 
+        // Defer calculating the path until now, to handle live map edits.
+        let maybe_req = spec.get_pathfinding_request(ctx.map);
+        let maybe_path = maybe_req.clone().and_then(|req| ctx.map.pathfind(req));
+
         match spec {
             TripSpec::VehicleAppearing {
                 start_pos,
@@ -1147,9 +1135,6 @@ impl TripManager {
                 assert_eq!(person.state, PersonState::Inside(start_bldg));
                 person.state = PersonState::Trip(trip);
 
-                // TODO For now, use the car we decided to statically. That makes sense in most
-                // cases.
-
                 if let Some(parked_car) = ctx.parking.lookup_parked_car(car).cloned() {
                     let start = SidewalkSpot::building(start_bldg, ctx.map);
                     let walking_goal =
@@ -1709,7 +1694,7 @@ pub struct Person {
     /// Both cars and bikes
     pub vehicles: Vec<Vehicle>,
 
-    delayed_trips: Vec<(TripID, TripSpec, Option<PathRequest>, Option<Path>)>,
+    delayed_trips: Vec<(TripID, TripSpec)>,
     on_bus: Option<CarID>,
 }