From d2ecab44a7033ed97b579448c4c2b2e5a4042f50 Mon Sep 17 00:00:00 2001
From: Dustin Carlino <dabreegster@gmail.com>
Date: Wed, 2 Jun 2021 11:40:54 -0700
Subject: [PATCH] Be sure to validate signals before committing the results,
 since GMNS imports aren't validated upfront. #626

---
 game/src/edit/traffic_signals/mod.rs     | 28 ++++++++++++++++++++++++
 map_model/src/objects/traffic_signals.rs |  2 +-
 2 files changed, 29 insertions(+), 1 deletion(-)

diff --git a/game/src/edit/traffic_signals/mod.rs b/game/src/edit/traffic_signals/mod.rs
index 9d5e3f20ac..1a8f675a6e 100644
--- a/game/src/edit/traffic_signals/mod.rs
+++ b/game/src/edit/traffic_signals/mod.rs
@@ -1,5 +1,7 @@
 use std::collections::{BTreeSet, VecDeque};
 
+use anyhow::Result;
+
 use abstutil::Timer;
 use geom::{Distance, Line, Polygon, Pt2D};
 use map_gui::options::TrafficSignalStyle;
@@ -175,6 +177,14 @@ impl TrafficSignalEditor {
         self.draw_current = ctx.upload(batch);
         self.movements = movements;
     }
+
+    // We may have imported the signal configuration without validating it.
+    fn validate_all_members(&self, app: &App) -> Result<()> {
+        for i in &self.members {
+            app.primary.map.get_traffic_signal(*i).validate()?;
+        }
+        Ok(())
+    }
 }
 
 impl State<App> for TrafficSignalEditor {
@@ -299,6 +309,15 @@ impl State<App> for TrafficSignalEditor {
                                 changes to include them.",
                             ],
                         ));
+                    } else if let Err(err) = self.validate_all_members(app) {
+                        // TODO There's some crash between usvg and lyon trying to tesellate the
+                        // error text!
+                        error!("{}", err);
+                        return Transition::Push(PopupMsg::new_state(
+                            ctx,
+                            "Error",
+                            vec!["This signal configuration is somehow invalid; check the console logs"]
+                        ));
                     } else {
                         let changes = BundleEdits::get_current(app, &self.members);
                         self.original.apply(app);
@@ -320,6 +339,15 @@ impl State<App> for TrafficSignalEditor {
                     ));
                 }
                 "Edit multiple signals" => {
+                    if let Err(err) = self.validate_all_members(app) {
+                        error!("{}", err);
+                        return Transition::Push(PopupMsg::new_state(
+                            ctx,
+                            "Error",
+                            vec!["This signal configuration is somehow invalid; check the console logs"]
+                        ));
+                    }
+
                     // First commit the current changes, so we enter SignalPicker with clean state.
                     // This UX flow is a little unintuitive.
                     let changes = check_for_missing_turns(app, &self.members)
diff --git a/map_model/src/objects/traffic_signals.rs b/map_model/src/objects/traffic_signals.rs
index e78d01aa1b..7d33763b2d 100644
--- a/map_model/src/objects/traffic_signals.rs
+++ b/map_model/src/objects/traffic_signals.rs
@@ -104,7 +104,7 @@ impl ControlTrafficSignal {
         Duration::seconds(time.inner_seconds().ceil())
     }
 
-    pub(crate) fn validate(&self) -> Result<()> {
+    pub fn validate(&self) -> Result<()> {
         // Does the assignment cover the correct set of movements?
         let expected_movements: BTreeSet<MovementID> = self.movements.keys().cloned().collect();
         let mut actual_movements: BTreeSet<MovementID> = BTreeSet::new();