refactor the osm tags helper

abstutil/src/collections.rs

@@ -200,3 +200,42 @@ impl<K: Clone + PartialEq, V> VecMap<K, V> {
         &mut self.inner.last_mut().unwrap().1
     }
 }
+
+// Convenience functions around a string->string map
+#[derive(Clone)]
+pub struct Tags(BTreeMap<String, String>);
+
+impl Tags {
+    pub fn new(map: BTreeMap<String, String>) -> Tags {
+        Tags(map)
+    }
+
+    pub fn get(&self, k: &str) -> Option<&String> {
+        self.0.get(k)
+    }
+
+    pub fn contains_key(&self, k: &str) -> bool {
+        self.0.contains_key(k)
+    }
+
+    pub fn is(&self, k: &str, v: &str) -> bool {
+        self.0.get(k) == Some(&v.to_string())
+    }
+
+    pub fn is_any(&self, k: &str, values: Vec<&str>) -> bool {
+        if let Some(v) = self.0.get(k) {
+            values.contains(&v.as_ref())
+        } else {
+            false
+        }
+    }
+
+    pub fn insert<K: Into<String>, V: Into<String>>(&mut self, k: K, v: V) {
+        self.0.insert(k.into(), v.into());
+    }
+
+    // TODO Maybe store this directly instead.
+    pub fn take(self) -> BTreeMap<String, String> {
+        self.0
+    }
+}

abstutil/src/lib.rs

@@ -8,7 +8,7 @@ mod time;
 pub use crate::cli::CmdArgs;
 pub use crate::clone::Cloneable;
 pub use crate::collections::{
-    contains_duplicates, retain_btreemap, retain_btreeset, wraparound_get, Counter, MultiMap,
+    contains_duplicates, retain_btreemap, retain_btreeset, wraparound_get, Counter, MultiMap, Tags,
     VecMap,
 };
 pub use crate::io::{

convert_osm/src/osm_reader.rs

@@ -1,4 +1,4 @@
-use abstutil::{FileWithProgress, Timer};
+use abstutil::{FileWithProgress, Tags, Timer};
 use geom::{GPSBounds, HashablePt2D, LonLat, PolyLine, Polygon, Pt2D, Ring};
 use map_model::raw::{
     OriginalBuilding, RawArea, RawBuilding, RawBusRoute, RawBusStop, RawMap, RawParkingLot,
@@ -72,7 +72,7 @@ pub fn extract_osm(
         osm_node_ids.insert(pt.to_hashable(), node.id);
 
         let tags = tags_to_map(&node.tags);
-        if tags.get(osm::HIGHWAY) == Some(&"traffic_signals".to_string()) {
+        if tags.is(osm::HIGHWAY, "traffic_signals") {
             traffic_signals.insert(pt.to_hashable());
         }
         if let Some(amenity) = tags.get("amenity") {
@@ -118,21 +118,21 @@ pub fn extract_osm(
         }
         let pts = map.gps_bounds.convert(&gps_pts);
         let mut tags = tags_to_map(&way.tags);
-        tags.insert(osm::OSM_WAY_ID.to_string(), way.id.to_string());
+        tags.insert(osm::OSM_WAY_ID, way.id.to_string());
 
         if is_road(&mut tags) {
             // TODO Hardcoding these overrides. OSM is correct, these don't have
             // sidewalks; there's a crosswalk mapped. But until we can snap sidewalks properly, do
             // this to prevent the sidewalks from being disconnected.
             if way.id == 332060260 || way.id == 332060236 {
-                tags.insert(osm::SIDEWALK.to_string(), "right".to_string());
+                tags.insert(osm::SIDEWALK, "right");
             }
 
             roads.push((
                 way.id,
                 RawRoad {
                     center_points: pts,
-                    osm_tags: tags,
+                    osm_tags: tags.take(),
                     turn_restrictions: Vec::new(),
                     complicated_turn_restrictions: Vec::new(),
                 },
@@ -151,7 +151,7 @@ pub fn extract_osm(
                     public_garage_name: None,
                     num_parking_spots: 0,
                     amenities: get_bldg_amenities(&tags),
-                    osm_tags: tags,
+                    osm_tags: tags.take(),
                 },
             );
         } else if let Some(at) = get_area_type(&tags) {
@@ -162,17 +162,17 @@ pub fn extract_osm(
                 area_type: at,
                 osm_id: way.id,
                 polygon: Polygon::new(&pts),
-                osm_tags: tags,
+                osm_tags: tags.take(),
             });
-        } else if tags.get("natural") == Some(&"coastline".to_string()) {
+        } else if tags.is("natural", "coastline") {
             coastline_groups.push((way.id, pts));
-        } else if tags.get("amenity") == Some(&"parking".to_string()) {
+        } else if tags.is("amenity", "parking") {
             // TODO Verify parking = surface or handle other cases?
             map.parking_lots.push(RawParkingLot {
                 polygon: Polygon::new(&pts),
                 osm_id: way.id,
             });
-        } else if tags.get("highway") == Some(&"service".to_string()) {
+        } else if tags.is("highway", "service") {
             map.parking_aisles.push(pts);
         } else {
             // The way might be part of a relation later.
@@ -188,22 +188,22 @@ pub fn extract_osm(
     for rel in doc.relations.values() {
         timer.next();
         let mut tags = tags_to_map(&rel.tags);
-        tags.insert(osm::OSM_REL_ID.to_string(), rel.id.to_string());
+        tags.insert(osm::OSM_REL_ID, rel.id.to_string());
 
         if let Some(area_type) = get_area_type(&tags) {
-            if tags.get("type") == Some(&"multipolygon".to_string()) {
+            if tags.is("type", "multipolygon") {
                 if let Some(pts_per_way) = get_multipolygon_members(rel, &id_to_way) {
                     for polygon in glue_multipolygon(rel.id, pts_per_way, &boundary) {
                         map.areas.push(RawArea {
                             area_type,
                             osm_id: rel.id,
                             polygon,
-                            osm_tags: tags.clone(),
+                            osm_tags: tags.clone().take(),
                         });
                     }
                 }
             }
-        } else if tags.get("type") == Some(&"restriction".to_string()) {
+        } else if tags.is("type", "restriction") {
             let mut from_way_id: Option<i64> = None;
             let mut via_node_id: Option<i64> = None;
             let mut via_way_id: Option<i64> = None;
@@ -280,11 +280,11 @@ pub fn extract_osm(
                         public_garage_name: None,
                         num_parking_spots: 0,
                         amenities: get_bldg_amenities(&tags),
-                        osm_tags: tags,
+                        osm_tags: tags.take(),
                     },
                 );
             }
-        } else if tags.get("type") == Some(&"route_master".to_string()) {
+        } else if tags.is("type", "route_master") {
             map.bus_routes
                 .extend(extract_route(&tags, rel, &doc, &id_to_way, &map.gps_bounds));
         }
@@ -326,7 +326,8 @@ pub fn extract_osm(
     )
 }
 
-fn tags_to_map(raw_tags: &[osm_xml::Tag]) -> BTreeMap<String, String> {
+fn tags_to_map(raw_tags: &[osm_xml::Tag]) -> Tags {
+    Tags::new(
         raw_tags
             .iter()
             .filter_map(|tag| {
@@ -337,11 +338,12 @@ fn tags_to_map(raw_tags: &[osm_xml::Tag]) -> BTreeMap<String, String> {
                     Some((tag.key.clone(), tag.val.clone()))
                 }
             })
-        .collect()
+            .collect(),
+    )
 }
 
-fn is_road(tags: &mut BTreeMap<String, String>) -> bool {
+fn is_road(tags: &mut Tags) -> bool {
-    if tags.get("railway") == Some(&"light_rail".to_string()) {
+    if tags.is("railway", "light_rail") {
         return true;
     }
 
@@ -351,7 +353,9 @@ fn is_road(tags: &mut BTreeMap<String, String>) -> bool {
 
     // https://github.com/Project-OSRM/osrm-backend/blob/master/profiles/car.lua is another
     // potential reference
-    for value in &[
+    if tags.is_any(
+        osm::HIGHWAY,
+        vec![
             // List of non-car types from https://wiki.openstreetmap.org/wiki/Key:highway
             // TODO Footways are very useful, but they need more work to associate with main roads
             "footway",
@@ -375,53 +379,51 @@ fn is_road(tags: &mut BTreeMap<String, String>) -> bool {
             "razed",
             "corridor",
             "junction",
-    ] {
+        ],
-        if tags.get(osm::HIGHWAY) == Some(&value.to_string()) {
+    ) {
         return false;
     }
-    }
 
     // If there's no parking data in OSM already, then assume no parking and mark that it's
     // inferred.
     if !tags.contains_key(osm::PARKING_LEFT)
         && !tags.contains_key(osm::PARKING_RIGHT)
         && !tags.contains_key(osm::PARKING_BOTH)
-        && tags.get(osm::HIGHWAY) != Some(&"motorway".to_string())
+        && !tags.is(osm::HIGHWAY, "motorway")
-        && tags.get(osm::HIGHWAY) != Some(&"motorway_link".to_string())
+        && !tags.is(osm::HIGHWAY, "motorway_link")
-        && tags.get("junction") != Some(&"roundabout".to_string())
+        && !tags.is("junction", "roundabout")
     {
-        tags.insert(osm::PARKING_BOTH.to_string(), "no_parking".to_string());
+        tags.insert(osm::PARKING_BOTH, "no_parking");
-        tags.insert(osm::INFERRED_PARKING.to_string(), "true".to_string());
+        tags.insert(osm::INFERRED_PARKING, "true");
     }
 
     // If there's no sidewalk data in OSM already, then make an assumption and mark that
     // it's inferred.
     if !tags.contains_key(osm::SIDEWALK) {
-        tags.insert(osm::INFERRED_SIDEWALKS.to_string(), "true".to_string());
+        tags.insert(osm::INFERRED_SIDEWALKS, "true");
-        if tags.get(osm::HIGHWAY) == Some(&"motorway".to_string())
+        if tags.is_any(osm::HIGHWAY, vec!["motorway", "motorway_link"])
-            || tags.get(osm::HIGHWAY) == Some(&"motorway_link".to_string())
+            || tags.is("junction", "roundabout")
-            || tags.get("junction") == Some(&"roundabout".to_string())
         {
-            tags.insert(osm::SIDEWALK.to_string(), "none".to_string());
+            tags.insert(osm::SIDEWALK, "none");
-        } else if tags.get("oneway") == Some(&"yes".to_string()) {
+        } else if tags.is("oneway", "yes") {
-            tags.insert(osm::SIDEWALK.to_string(), "right".to_string());
+            tags.insert(osm::SIDEWALK, "right");
-            if tags.get(osm::HIGHWAY) == Some(&"residential".to_string()) {
+            if tags.is(osm::HIGHWAY, "residential") {
-                tags.insert(osm::SIDEWALK.to_string(), "both".to_string());
+                tags.insert(osm::SIDEWALK, "both");
             }
         } else {
-            tags.insert(osm::SIDEWALK.to_string(), "both".to_string());
+            tags.insert(osm::SIDEWALK, "both");
         }
     }
 
     true
 }
 
-fn is_bldg(tags: &BTreeMap<String, String>) -> bool {
+fn is_bldg(tags: &Tags) -> bool {
     // Sorry, the towers at Gasworks don't count. :)
     tags.contains_key("building") && !tags.contains_key("abandoned:man_made")
 }
 
-fn get_bldg_amenities(tags: &BTreeMap<String, String>) -> BTreeSet<(String, String)> {
+fn get_bldg_amenities(tags: &Tags) -> BTreeSet<(String, String)> {
     let mut amenities = BTreeSet::new();
     if let Some(amenity) = tags.get("amenity") {
         amenities.insert((
@@ -442,38 +444,35 @@ fn get_bldg_amenities(tags: &BTreeMap<String, String>) -> BTreeSet<(String, Stri
     amenities
 }
 
-fn get_area_type(tags: &BTreeMap<String, String>) -> Option<AreaType> {
+fn get_area_type(tags: &Tags) -> Option<AreaType> {
-    if tags.get("leisure") == Some(&"park".to_string()) {
+    if tags.is_any("leisure", vec!["park", "golf_course"]) {
         return Some(AreaType::Park);
     }
-    if tags.get("leisure") == Some(&"golf_course".to_string()) {
+    if tags.is("natural", "wood") {
         return Some(AreaType::Park);
     }
-    if tags.get("natural") == Some(&"wood".to_string()) {
+    if tags.is("landuse", "cemetery") {
         return Some(AreaType::Park);
     }
-    if tags.get("landuse") == Some(&"cemetery".to_string()) {
+
-        return Some(AreaType::Park);
+    if tags.is("natural", "water") || tags.is("waterway", "riverbank") {
-    }
-    if tags.get("natural") == Some(&"water".to_string())
-        || tags.get("waterway") == Some(&"riverbank".to_string())
-    {
         return Some(AreaType::Water);
     }
-    if tags.get("place") == Some(&"island".to_string()) {
+
+    if tags.is("place", "island") {
         return Some(AreaType::Island);
     }
+
     // TODO These just cover up poorly inferred road geometry now. Figure out how to use these.
     if false {
-        if tags.get("traffic_calming") == Some(&"island".to_string()) {
+        if tags.is("traffic_calming", "island") {
             return Some(AreaType::PedestrianIsland);
         }
-        if tags.get("highway") == Some(&"pedestrian".to_string())
+        if tags.is("highway", "pedestrian") && tags.is("area", "yes") {
-            && tags.get("area") == Some(&"yes".to_string())
-        {
             return Some(AreaType::PedestrianIsland);
         }
     }
+
     None
 }
 
@@ -605,7 +604,7 @@ fn glue_to_boundary(result_pl: PolyLine, boundary: &Ring) -> Option<Polygon> {
 }
 
 fn extract_route(
-    master_tags: &BTreeMap<String, String>,
+    master_tags: &Tags,
     master_rel: &osm_xml::Relation,
     doc: &osm_xml::OSM,
     id_to_way: &HashMap<i64, Vec<Pt2D>>,

map_model/src/make/buildings.rs

@@ -4,7 +4,7 @@ use crate::{
     osm, Building, BuildingID, BuildingType, FrontPath, LaneID, LaneType, Map, OffstreetParking,
     ParkingLot, ParkingLotID, Position, NORMAL_LANE_THICKNESS, PARKING_LOT_SPOT_LENGTH,
 };
-use abstutil::Timer;
+use abstutil::{Tags, Timer};
 use geom::{Angle, Distance, FindClosest, HashablePt2D, Line, PolyLine, Polygon, Pt2D, Ring};
 use rand::{Rng, SeedableRng};
 use rand_xorshift::XorShiftRng;
@@ -73,7 +73,12 @@ pub fn make_all_buildings(
                 amenities: b.amenities.clone(),
                 parking: None,
                 label_center: b.polygon.polylabel(),
-                bldg_type: classify_bldg(&b.osm_tags, &b.amenities, b.polygon.area(), &mut rng),
+                bldg_type: classify_bldg(
+                    Tags::new(b.osm_tags.clone()),
+                    &b.amenities,
+                    b.polygon.area(),
+                    &mut rng,
+                ),
             };
 
             // Can this building have a driveway? If it's not next to a driving lane, then no.
@@ -360,13 +365,12 @@ fn line_valid(
 }
 
 fn classify_bldg(
-    tags: &BTreeMap<String, String>,
+    tags: Tags,
     amenities: &BTreeSet<(String, String)>,
     area_sq_meters: f64,
     rng: &mut rand_xorshift::XorShiftRng,
 ) -> BuildingType {
     // used: top values from https://taginfo.openstreetmap.org/keys/building#values (>100k uses)
-    let tags = Tags(tags);
 
     let mut commercial = false;
     let workers;
@@ -426,7 +430,6 @@ fn classify_bldg(
         workers = rng.gen_range(0, 2);
     } else if tags.is_any("building", vec!["apartments", "terrace", "residential"]) {
         let levels = tags
-            .0
             .get("building:levels")
             .and_then(|x| x.parse::<usize>().ok())
             .unwrap_or(1);
@@ -445,19 +448,3 @@ fn classify_bldg(
     }
     return BuildingType::Residential(workers);
 }
-
-// TODO Refactor with lane_specs
-struct Tags<'a>(&'a BTreeMap<String, String>);
-impl<'a> Tags<'a> {
-    fn is(&self, k: &str, v: &str) -> bool {
-        self.0.get(k) == Some(&v.to_string())
-    }
-
-    fn is_any(&self, k: &str, values: Vec<&str>) -> bool {
-        if let Some(v) = self.0.get(k) {
-            values.contains(&v.as_ref())
-        } else {
-            false
-        }
-    }
-}

map_model/src/make/initial/lane_specs.rs

@@ -1,4 +1,5 @@
 use crate::{osm, LaneType};
+use abstutil::Tags;
 use serde::{Deserialize, Serialize};
 use std::collections::BTreeMap;
 use std::{fmt, iter};
@@ -6,9 +7,9 @@ use std::{fmt, iter};
 // TODO This is ripe for unit testing.
 // (original direction, reversed direction)
 pub fn get_lane_types(osm_tags: &BTreeMap<String, String>) -> (Vec<LaneType>, Vec<LaneType>) {
-    let tags = Tags(osm_tags);
+    let tags = Tags::new(osm_tags.clone());
 
-    if let Some(s) = osm_tags.get(osm::SYNTHETIC_LANES) {
+    if let Some(s) = tags.get(osm::SYNTHETIC_LANES) {
         if let Some(spec) = RoadSpec::parse(s.to_string()) {
             return (spec.fwd, spec.back);
         } else {
@@ -25,20 +26,16 @@ pub fn get_lane_types(osm_tags: &BTreeMap<String, String>) -> (Vec<LaneType>, Ve
     }
 
     // TODO Reversible roads should be handled differently?
-    let oneway = tags.is("oneway", "yes")
+    let oneway =
-        || tags.is("oneway", "reversible")
+        tags.is_any("oneway", vec!["yes", "reversible"]) || tags.is("junction", "roundabout");
-        || tags.is("junction", "roundabout");
 
     // How many driving lanes in each direction?
-    let num_driving_fwd = if let Some(n) = osm_tags
+    let num_driving_fwd = if let Some(n) = tags
         .get("lanes:forward")
         .and_then(|num| num.parse::<usize>().ok())
     {
         n
-    } else if let Some(n) = osm_tags
+    } else if let Some(n) = tags.get("lanes").and_then(|num| num.parse::<usize>().ok()) {
-        .get("lanes")
-        .and_then(|num| num.parse::<usize>().ok())
-    {
         if oneway {
             n
         } else if n % 2 == 0 {
@@ -51,15 +48,12 @@ pub fn get_lane_types(osm_tags: &BTreeMap<String, String>) -> (Vec<LaneType>, Ve
         // TODO Grrr.
         1
     };
-    let num_driving_back = if let Some(n) = osm_tags
+    let num_driving_back = if let Some(n) = tags
         .get("lanes:backward")
         .and_then(|num| num.parse::<usize>().ok())
     {
         n
-    } else if let Some(n) = osm_tags
+    } else if let Some(n) = tags.get("lanes").and_then(|num| num.parse::<usize>().ok()) {
-        .get("lanes")
-        .and_then(|num| num.parse::<usize>().ok())
-    {
         if oneway {
             0
         } else if n % 2 == 0 {
@@ -80,7 +74,7 @@ pub fn get_lane_types(osm_tags: &BTreeMap<String, String>) -> (Vec<LaneType>, Ve
     let driving_lane = if tags.is("access", "no") && tags.is("bus", "yes") {
         // Sup West Seattle
         LaneType::Bus
-    } else if osm_tags
+    } else if tags
         .get("motor_vehicle:conditional")
         .map(|x| x.starts_with("no"))
         .unwrap_or(false)
@@ -105,14 +99,14 @@ pub fn get_lane_types(osm_tags: &BTreeMap<String, String>) -> (Vec<LaneType>, Ve
         return (fwd_side, back_side);
     }
 
-    let fwd_bus_spec = if let Some(s) = osm_tags.get("bus:lanes:forward") {
+    let fwd_bus_spec = if let Some(s) = tags.get("bus:lanes:forward") {
         s
-    } else if let Some(s) = osm_tags.get("psv:lanes:forward") {
+    } else if let Some(s) = tags.get("psv:lanes:forward") {
         s
     } else if oneway {
-        if let Some(s) = osm_tags.get("bus:lanes") {
+        if let Some(s) = tags.get("bus:lanes") {
             s
-        } else if let Some(s) = osm_tags.get("psv:lanes") {
+        } else if let Some(s) = tags.get("psv:lanes") {
             s
         } else {
             ""
@@ -135,9 +129,9 @@ pub fn get_lane_types(osm_tags: &BTreeMap<String, String>) -> (Vec<LaneType>, Ve
             }
         }
     }
-    if let Some(spec) = osm_tags
+    if let Some(spec) = tags
         .get("bus:lanes:backward")
-        .or_else(|| osm_tags.get("psv:lanes:backward"))
+        .or_else(|| tags.get("psv:lanes:backward"))
     {
         let parts: Vec<&str> = spec.split("|").collect();
         if parts.len() == back_side.len() {
@@ -161,8 +155,7 @@ pub fn get_lane_types(osm_tags: &BTreeMap<String, String>) -> (Vec<LaneType>, Ve
         if tags.is("cycleway:right", "lane") {
             fwd_side.push(LaneType::Biking);
         }
-        if tags.is("cycleway:left", "lane")
+        if tags.is_any("cycleway:left", vec!["lane", "opposite_lane"])
-            || tags.is("cycleway:left", "opposite_lane")
             || tags.is("cycleway", "opposite_lane")
         {
             back_side.push(LaneType::Biking);
@@ -175,15 +168,11 @@ pub fn get_lane_types(osm_tags: &BTreeMap<String, String>) -> (Vec<LaneType>, Ve
     }
 
     if driving_lane == LaneType::Driving {
-        fn has_parking(value: Option<&String>) -> bool {
+        let has_parking = vec!["parallel", "diagonal", "perpendicular"];
-            value == Some(&"parallel".to_string())
+        let parking_lane_fwd = tags.is_any(osm::PARKING_RIGHT, has_parking.clone())
-                || value == Some(&"diagonal".to_string())
+            || tags.is_any(osm::PARKING_BOTH, has_parking.clone());
-                || value == Some(&"perpendicular".to_string())
+        let parking_lane_back = tags.is_any(osm::PARKING_LEFT, has_parking.clone())
-        }
+            || tags.is_any(osm::PARKING_BOTH, has_parking);
-        let parking_lane_fwd = has_parking(osm_tags.get(osm::PARKING_RIGHT))
-            || has_parking(osm_tags.get(osm::PARKING_BOTH));
-        let parking_lane_back = has_parking(osm_tags.get(osm::PARKING_LEFT))
-            || has_parking(osm_tags.get(osm::PARKING_BOTH));
         if parking_lane_fwd {
             fwd_side.push(LaneType::Parking);
         }
@@ -210,15 +199,6 @@ pub fn get_lane_types(osm_tags: &BTreeMap<String, String>) -> (Vec<LaneType>, Ve
     (fwd_side, back_side)
 }
 
-// TODO Figure out Rust strings; there's maybe a less verbose way than the old
-// osm_tags.get("cycleway:right") == Some(&"lane".to_string()) mess.
-struct Tags<'a>(&'a BTreeMap<String, String>);
-impl<'a> Tags<'a> {
-    fn is(&self, k: &str, v: &str) -> bool {
-        self.0.get(k) == Some(&v.to_string())
-    }
-}
-
 // This is a convenient way for map_editor to plumb instructions here.
 #[derive(Serialize, Deserialize)]
 pub struct RoadSpec {

map_model/src/osm.rs

@@ -13,8 +13,6 @@ pub const SIDEWALK: &str = "sidewalk";
 // The rest of these are all inserted by A/B Street to plumb data between different stages of map
 // construction. They could be plumbed another way, but this is the most convenient.
 
-// TODO Comparing to Some(&"true".to_string()) is annoying
-
 // Just a copy of OSM IDs, so that things displaying/searching tags will also pick these up.
 pub const OSM_WAY_ID: &str = "abst:osm_way_id";
 pub const OSM_REL_ID: &str = "abst:osm_rel_id";