Save LTN proposals in a more future-proof format (#894)

Cargo.lock

@@ -2095,6 +2095,7 @@ dependencies = [
  "abstutil",
  "anyhow",
  "contour",
+ "flate2",
  "fs-err",
  "geo",
  "geojson",
@@ -2107,6 +2108,7 @@ dependencies = [
  "map_model",
  "maplit",
  "raw_map",
+ "regex",
  "serde",
  "serde_json",
  "structopt",

abstio/src/abst_paths.rs

@@ -398,7 +398,7 @@ pub fn path_all_edits(name: &MapName) -> String {
 
 pub fn path_ltn_proposals(name: &MapName, proposal_name: &str) -> String {
     path(format!(
-        "player/ltn_proposals/{}/{}/{}/{}.bin",
+        "player/ltn_proposals/{}/{}/{}/{}.json.gz",
         name.city.country, name.city.city, name.map, proposal_name
     ))
 }

abstio/src/io_native.rs

@@ -95,6 +95,12 @@ pub fn write_binary<T: Serialize>(path: String, obj: &T) {
     info!("Wrote {}", path);
 }
 
+pub fn write_raw(path: String, bytes: &[u8]) -> Result<()> {
+    let mut file = BufWriter::new(File::create(path)?);
+    file.write_all(bytes)?;
+    Ok(())
+}
+
 /// Idempotent
 pub fn delete_file<I: AsRef<str>>(path: I) {
     let path = path.as_ref();

abstio/src/io_web.rs

@@ -82,17 +82,18 @@ pub fn list_dir(dir: String) -> Vec<String> {
 
 pub fn slurp_file<I: AsRef<str>>(path: I) -> Result<Vec<u8>> {
     let path = path.as_ref();
-    debug!(
-        "slurping file: {}, trimmed_path: {}",
-        path,
-        path.trim_start_matches("../data/system/")
-    );
 
     if let Some(raw) = SYSTEM_DATA.get_file(path.trim_start_matches("../data/system/")) {
         Ok(raw.contents().to_vec())
     } else if path.starts_with(&path_player("")) {
         let string = read_local_storage(path)?;
-        Ok(string.into_bytes())
+        // TODO Hack: if it probably wasn't written with write_json, do the base64 decoding. This
+        // may not always be appropriate...
+        if path.ends_with(".json") {
+            Ok(string.into_bytes())
+        } else {
+            base64::decode(string).map_err(|err| err.into())
+        }
     } else {
         bail!("Can't slurp_file {}, it doesn't exist", path)
     }
@@ -122,17 +123,23 @@ pub fn write_json<T: Serialize>(path: String, obj: &T) {
 }
 
 pub fn write_binary<T: Serialize>(path: String, obj: &T) {
+    write_raw(path, &abstutil::to_binary(obj)).unwrap();
+}
+
+pub fn write_raw(path: String, bytes: &[u8]) -> Result<()> {
     // Only save for data/player, for now
     if !path.starts_with(&path_player("")) {
-        warn!("Not saving {}", path);
-        return;
+        bail!("Not saving {}", path);
     }
 
     let window = web_sys::window().unwrap();
     let storage = window.local_storage().unwrap().unwrap();
     // Local storage only supports strings, so base64 encoding needed
-    let encoded = base64::encode(abstutil::to_binary(obj));
-    storage.set_item(&path, &encoded).unwrap();
+    let encoded = base64::encode(bytes);
+    storage
+        .set_item(&path, &encoded)
+        .map_err(|err| anyhow!(err.as_string().unwrap_or("set_item failed".to_string())))?;
+    Ok(())
 }
 
 pub fn delete_file<I: AsRef<str>>(path: I) {

apps/ltn/Cargo.toml

@@ -16,6 +16,7 @@ abstio = { path = "../../abstio" }
 abstutil = { path = "../../abstutil" }
 anyhow = "1.0.38"
 contour = "0.4.0"
+flate2 = "1.0.20"
 fs-err = "2.6.0"
 geo = "0.19"
 geojson = { version = "0.22.2", features = ["geo-types"] }
@@ -28,6 +29,7 @@ maplit = "1.0.2"
 map_gui = { path = "../../map_gui" }
 map_model = { path = "../../map_model" }
 raw_map = { path = "../../raw_map" }
+regex = "1.5.4"
 serde = "1.0.123"
 serde_json = "1.0.61"
 synthpop = { path = "../../synthpop" }

apps/ltn/src/filters/mod.rs

@@ -5,6 +5,7 @@ use std::collections::{BTreeMap, BTreeSet};
 
 use serde::{Deserialize, Serialize};
 
+use abstutil::{deserialize_btreemap, serialize_btreemap};
 use geom::{Circle, Distance, Line};
 use map_model::{IntersectionID, Map, RoadID, RoutingParams, TurnID};
 use widgetry::mapspace::{DrawUnzoomedShapes, ToggleZoomed};
@@ -16,8 +17,17 @@ use crate::{after_edit, colors, App};
 /// Stored in App session state. Before making any changes, call `before_edit`.
 #[derive(Clone, Default, Serialize, Deserialize)]
 pub struct ModalFilters {
+    // We use serialize_btreemap so that save::perma can detect and transform IDs
     /// For filters placed along a road, where is the filter located?
+    #[serde(
+        serialize_with = "serialize_btreemap",
+        deserialize_with = "deserialize_btreemap"
+    )]
     pub roads: BTreeMap<RoadID, Distance>,
+    #[serde(
+        serialize_with = "serialize_btreemap",
+        deserialize_with = "deserialize_btreemap"
+    )]
     pub intersections: BTreeMap<IntersectionID, DiagonalFilter>,
 
     /// Edit history is preserved recursively

apps/ltn/src/save/mod.rs

@@ -1,8 +1,10 @@
+mod perma;
+
 use anyhow::Result;
 use serde::{Deserialize, Serialize};
 
 use abstio::MapName;
-use abstutil::{Counter, Timer};
+use abstutil::Counter;
 use map_gui::tools::{ChooseSomething, PromptInput};
 use map_model::PathRequest;
 use widgetry::tools::PopupMsg;
@@ -11,11 +13,8 @@ use widgetry::{Choice, EventCtx, Key, Line, State, Widget};
 use crate::partition::BlockID;
 use crate::{App, BrowseNeighborhoods, ModalFilters, Partitioning, Transition};
 
-/// Captures all of the edits somebody makes to a map in the LTN tool. Note this separate from
+/// Captures all of the edits somebody makes to a map in the LTN tool. Note this is separate from
 /// `map_model::MapEdits`.
-///
-/// TODO Note this format isn't future-proof at all. Changes to the LTN blockfinding algorithm or
-/// map data (like RoadIDs) will probably break someone's edits.
 #[derive(Serialize, Deserialize)]
 pub struct Proposal {
     pub map: MapName,
@@ -51,22 +50,27 @@ impl Proposal {
 
     /// Try to load a proposal. If it fails, returns a popup message state.
     pub fn load(ctx: &mut EventCtx, app: &mut App, name: &str) -> Option<Box<dyn State<App>>> {
-        ctx.loading_screen(
-            "load existing proposal",
-            |ctx, mut timer| match Self::inner_load(ctx, app, name, &mut timer) {
-                Ok(()) => None,
-                Err(err) => Some(PopupMsg::new_state(
-                    ctx,
-                    "Error",
-                    vec![format!("Couldn't load proposal {}", name), err.to_string()],
-                )),
-            },
-        )
+        match Self::inner_load(ctx, app, name) {
+            Ok(()) => None,
+            Err(err) => Some(PopupMsg::new_state(
+                ctx,
+                "Error",
+                vec![
+                    format!("Couldn't load proposal {}", name),
+                    err.to_string(),
+                    "The format of saved proposals recently changed.".to_string(),
+                    "Contact dabreegster@gmail.com if you need help restoring a file.".to_string(),
+                ],
+            )),
+        }
     }
 
-    fn inner_load(ctx: &mut EventCtx, app: &mut App, name: &str, timer: &mut Timer) -> Result<()> {
-        let proposal: Proposal =
-            abstio::maybe_read_binary(abstio::path_ltn_proposals(app.map.get_name(), name), timer)?;
+    fn inner_load(ctx: &mut EventCtx, app: &mut App, name: &str) -> Result<()> {
+        let bytes = abstio::slurp_file(abstio::path_ltn_proposals(app.map.get_name(), name))?;
+        let decoder = flate2::read::GzDecoder::new(&bytes[..]);
+        let value = serde_json::from_reader(decoder)?;
+        let proposal = perma::from_permanent(&app.map, value)?;
+
         // TODO We could try to detect if the file's partitioning (road IDs and such) still matches
         // this version of the map or not
 
@@ -125,16 +129,35 @@ fn save_ui(ctx: &mut EventCtx, app: &App, preserve_state: PreserveState) -> Box<
             // and file state are not synchronized / auto-saved.
             app.session.proposal_name = Some(name.clone());
 
-            let path = abstio::path_ltn_proposals(app.map.get_name(), &name);
-            let proposal = Proposal::from_app(app);
-            abstio::write_binary(path, &proposal);
-
-            // If we changed the name, we'll want to recreate the panel
-            preserve_state.switch_to_state(ctx, app)
+            match inner_save(app) {
+                // If we changed the name, we'll want to recreate the panel
+                Ok(()) => preserve_state.switch_to_state(ctx, app),
+                Err(err) => Transition::Multi(vec![
+                    preserve_state.switch_to_state(ctx, app),
+                    Transition::Push(PopupMsg::new_state(
+                        ctx,
+                        "Error",
+                        vec![format!("Couldn't save proposal: {}", err)],
+                    )),
+                ]),
+            }
         }),
     )
 }
 
+fn inner_save(app: &App) -> Result<()> {
+    let proposal = Proposal::from_app(app);
+    let path = abstio::path_ltn_proposals(app.map.get_name(), &proposal.name);
+
+    let json_value = perma::to_permanent(&app.map, &proposal)?;
+    let mut output_buffer = Vec::new();
+    let mut encoder =
+        flate2::write::GzEncoder::new(&mut output_buffer, flate2::Compression::best());
+    serde_json::to_writer(&mut encoder, &json_value)?;
+    encoder.finish()?;
+    abstio::write_raw(path, &output_buffer)
+}
+
 fn load_picker_ui(
     ctx: &mut EventCtx,
     app: &App,
@@ -145,9 +168,15 @@ fn load_picker_ui(
     ChooseSomething::new_state(
         ctx,
         "Load which proposal?",
-        Choice::strings(abstio::list_all_objects(abstio::path_all_ltn_proposals(
-            app.map.get_name(),
-        ))),
+        // basename (and thus list_all_objects) turn "foo.json.gz" into "foo.json", so further
+        // strip out the extension.
+        // TODO Fix basename, but make sure nothing downstream breaks
+        Choice::strings(
+            abstio::list_all_objects(abstio::path_all_ltn_proposals(app.map.get_name()))
+                .into_iter()
+                .map(abstutil::basename)
+                .collect(),
+        ),
         Box::new(|name, ctx, app| match Proposal::load(ctx, app, &name) {
             Some(err_state) => Transition::Replace(err_state),
             None => preserve_state.switch_to_state(ctx, app),

apps/ltn/src/save/perma.rs

@@ -0,0 +1,128 @@
+//! The Proposal struct references IntersectionIDs and RoadIDs, which won't survive OSM updates.
+//! Similar to the MapEdits <-> PermanentMapEdits strategy, transform those IDs before saving.
+//!
+//! Unlike PermanentMapEdits, we don't define a PermanentProposal struct, because to do so for
+//! everything it includes would be a nightmare. In particular, Partitioning includes Blocks, which
+//! nest RoadIDs deep inside. Instead, play a "runtime reflection" trick:
+//!
+//! 1) Serialize the Proposal with RoadIDs to JSON
+//! 2) Dynamically walk the JSON
+//! 3) When the path of a value matches the hardcoded list of patterns in is_road_id and
+//!    is_intersection_id, transform to a permanent ID
+//! 4) Save the proposal as JSON with that ID instead
+//! 5) Do the inverse to later load
+//!
+//! In practice, this attempt to keep proposals compatible with future basemap updates might be
+//! futile. We're embedding loads of details about the partitioning, but not checking that they
+//! remain valid after loading. Even splitting one road in two anywhere in the map would likely
+//! break things kind of silently. Absolute worst case, we also record an abst_version field so we
+//! could manually load the proposal in the correct version, and do something to manually recover
+//! an old proposal.
+//!
+//! Also, the JSON blobs are massive because of the partitioning, so compress everything.
+
+use anyhow::Result;
+use lazy_static::lazy_static;
+use regex::Regex;
+use serde_json::Value;
+
+use map_model::{IntersectionID, Map, RoadID};
+use raw_map::osm::NodeID;
+use raw_map::OriginalRoad;
+
+use super::Proposal;
+
+pub fn to_permanent(map: &Map, proposal: &Proposal) -> Result<Value> {
+    let mut proposal_value = serde_json::to_value(proposal)?;
+    walk("", &mut proposal_value, &|path, value| {
+        if is_road_id(path) {
+            let replace_with = map.get_r(RoadID(value.as_u64().unwrap() as usize)).orig_id;
+            *value = serde_json::to_value(&replace_with)?;
+        } else if is_intersection_id(path) {
+            let replace_with = map
+                .get_i(IntersectionID(value.as_u64().unwrap() as usize))
+                .orig_id;
+            *value = serde_json::to_value(&replace_with)?;
+        }
+        Ok(())
+    })?;
+    Ok(proposal_value)
+}
+
+pub fn from_permanent(map: &Map, mut proposal_value: Value) -> Result<Proposal> {
+    walk("", &mut proposal_value, &|path, value| {
+        if is_road_id(path) {
+            let orig_id: OriginalRoad = serde_json::from_value(value.clone())?;
+            let replace_with = map.find_r_by_osm_id(orig_id)?;
+            *value = serde_json::to_value(&replace_with)?;
+        } else if is_intersection_id(path) {
+            let orig_id: NodeID = serde_json::from_value(value.clone())?;
+            let replace_with = map.find_i_by_osm_id(orig_id)?;
+            *value = serde_json::to_value(&replace_with)?;
+        }
+        Ok(())
+    })?;
+    let result = serde_json::from_value(proposal_value)?;
+    Ok(result)
+}
+
+fn is_road_id(path: &str) -> bool {
+    lazy_static! {
+        static ref PATTERNS: Vec<Regex> = vec![
+            Regex::new(r"^/modal_filters/roads/\d+/0$").unwrap(),
+            Regex::new(r"^/modal_filters/intersections/\d+/1/r1$").unwrap(),
+            Regex::new(r"^/modal_filters/intersections/\d+/1/r2$").unwrap(),
+            Regex::new(r"^/modal_filters/intersections/\d+/1/group1/y$").unwrap(),
+            Regex::new(r"^/modal_filters/intersections/\d+/1/group2/y$").unwrap(),
+            // First place a Block is stored
+            Regex::new(r"^/partitioning/single_blocks/\d+/perimeter/interior/\d+$").unwrap(),
+            Regex::new(r"^/partitioning/single_blocks/\d+/perimeter/roads/\d+/road$").unwrap(),
+            // The other
+            Regex::new(r"^/partitioning/neighborhoods/\d+/0/perimeter/interior/\d+$").unwrap(),
+            Regex::new(r"^/partitioning/neighborhoods/\d+/0/perimeter/roads/\d+/road$").unwrap(),
+        ];
+    }
+
+    PATTERNS.iter().any(|re| re.is_match(path))
+}
+
+fn is_intersection_id(path: &str) -> bool {
+    lazy_static! {
+        static ref PATTERNS: Vec<Regex> = vec![
+            Regex::new(r"^/modal_filters/intersections/\d+/0$").unwrap(),
+            Regex::new(r"^/modal_filters/intersections/\d+/1/i$").unwrap(),
+        ];
+    }
+
+    PATTERNS.iter().any(|re| re.is_match(path))
+}
+
+// Note there's no chance to transform keys in a map. So use serialize_btreemap elsewhere to force
+// into a list of pairs
+fn walk<F: Fn(&str, &mut Value) -> Result<()>>(
+    path: &str,
+    value: &mut Value,
+    transform: &F,
+) -> Result<()> {
+    match value {
+        Value::Array(list) => {
+            for (idx, x) in list.into_iter().enumerate() {
+                walk(&format!("{}/{}", path, idx), x, transform)?;
+            }
+            transform(path, value)?;
+        }
+        Value::Object(map) => {
+            for (key, val) in map {
+                walk(&format!("{}/{}", path, key), val, transform)?;
+            }
+            // After recursing, possibly transform this. We turn a number into an object, so to
+            // reverse that...
+            transform(path, value)?;
+        }
+        _ => {
+            transform(path, value)?;
+            // The value may have been transformed into an array or object, but don't walk it.
+        }
+    }
+    Ok(())
+}

map_model/src/edits/mod.rs

@@ -225,8 +225,7 @@ impl MapEdits {
             Err(_) => {
                 // The JSON format may have changed, so attempt backwards compatibility.
                 let bytes = abstio::slurp_file(path)?;
-                let contents = std::str::from_utf8(&bytes)?;
-                let value = serde_json::from_str(contents)?;
+                let value = serde_json::from_slice(&bytes)?;
                 compat::upgrade(value, map)?
             }
         };

tests/src/main.rs

@@ -354,7 +354,11 @@ fn bus_route_test() -> Result<()> {
         ));
         let mut f = File::create(path)?;
         for tr in map.all_transit_routes() {
-            writeln!(f, "{} ({}) from {} to {:?}", tr.gtfs_id, tr.short_name, tr.start, tr.end_border)?;
+            writeln!(
+                f,
+                "{} ({}) from {} to {:?}",
+                tr.gtfs_id, tr.short_name, tr.start, tr.end_border
+            )?;
             for ts in &tr.stops {
                 let ts = map.get_ts(*ts);
                 writeln!(