From a5cf34ff59bd07534b12622e9109e35e42bdb7d3 Mon Sep 17 00:00:00 2001 From: Dustin Carlino Date: Tue, 14 Jul 2020 09:03:37 -0700 Subject: [PATCH] refactor the osm tags helper --- abstutil/src/collections.rs | 39 +++++ abstutil/src/lib.rs | 2 +- convert_osm/src/osm_reader.rs | 181 +++++++++++------------ map_model/src/make/buildings.rs | 29 +--- map_model/src/make/initial/lane_specs.rs | 64 +++----- map_model/src/osm.rs | 2 - 6 files changed, 160 insertions(+), 157 deletions(-) diff --git a/abstutil/src/collections.rs b/abstutil/src/collections.rs index 699192e756..6c6959fb0f 100644 --- a/abstutil/src/collections.rs +++ b/abstutil/src/collections.rs @@ -200,3 +200,42 @@ impl VecMap { &mut self.inner.last_mut().unwrap().1 } } + +// Convenience functions around a string->string map +#[derive(Clone)] +pub struct Tags(BTreeMap); + +impl Tags { + pub fn new(map: BTreeMap) -> 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, V: Into>(&mut self, k: K, v: V) { + self.0.insert(k.into(), v.into()); + } + + // TODO Maybe store this directly instead. + pub fn take(self) -> BTreeMap { + self.0 + } +} diff --git a/abstutil/src/lib.rs b/abstutil/src/lib.rs index c462204ce0..5670fd7479 100644 --- a/abstutil/src/lib.rs +++ b/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::{ diff --git a/convert_osm/src/osm_reader.rs b/convert_osm/src/osm_reader.rs index 5a5008114b..a8aed2fe49 100644 --- a/convert_osm/src/osm_reader.rs +++ b/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 = None; let mut via_node_id: Option = None; let mut via_way_id: Option = 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,22 +326,24 @@ pub fn extract_osm( ) } -fn tags_to_map(raw_tags: &[osm_xml::Tag]) -> BTreeMap { - raw_tags - .iter() - .filter_map(|tag| { - // Toss out really useless metadata. - if tag.key.starts_with("tiger:") || tag.key.starts_with("old_name:") { - None - } else { - Some((tag.key.clone(), tag.val.clone())) - } - }) - .collect() +fn tags_to_map(raw_tags: &[osm_xml::Tag]) -> Tags { + Tags::new( + raw_tags + .iter() + .filter_map(|tag| { + // Toss out really useless metadata. + if tag.key.starts_with("tiger:") || tag.key.starts_with("old_name:") { + None + } else { + Some((tag.key.clone(), tag.val.clone())) + } + }) + .collect(), + ) } -fn is_road(tags: &mut BTreeMap) -> bool { - if tags.get("railway") == Some(&"light_rail".to_string()) { +fn is_road(tags: &mut Tags) -> bool { + if tags.is("railway", "light_rail") { return true; } @@ -351,34 +353,35 @@ fn is_road(tags: &mut BTreeMap) -> bool { // https://github.com/Project-OSRM/osrm-backend/blob/master/profiles/car.lua is another // potential reference - for value in &[ - // 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", - "living_street", - "pedestrian", - "track", - "bus_guideway", - "escape", - "raceway", - "bridleway", - "steps", - "path", - "cycleway", - "proposed", - // This one's debatable. Includes alleys. - "service", - // more discovered manually - "abandoned", - "elevator", - "planned", - "razed", - "corridor", - "junction", - ] { - if tags.get(osm::HIGHWAY) == Some(&value.to_string()) { - return false; - } + 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", + "living_street", + "pedestrian", + "track", + "bus_guideway", + "escape", + "raceway", + "bridleway", + "steps", + "path", + "cycleway", + "proposed", + // This one's debatable. Includes alleys. + "service", + // more discovered manually + "abandoned", + "elevator", + "planned", + "razed", + "corridor", + "junction", + ], + ) { + return false; } // If there's no parking data in OSM already, then assume no parking and mark that it's @@ -386,42 +389,41 @@ fn is_road(tags: &mut BTreeMap) -> bool { 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.get(osm::HIGHWAY) != Some(&"motorway_link".to_string()) - && tags.get("junction") != Some(&"roundabout".to_string()) + && !tags.is(osm::HIGHWAY, "motorway") + && !tags.is(osm::HIGHWAY, "motorway_link") + && !tags.is("junction", "roundabout") { - tags.insert(osm::PARKING_BOTH.to_string(), "no_parking".to_string()); - tags.insert(osm::INFERRED_PARKING.to_string(), "true".to_string()); + tags.insert(osm::PARKING_BOTH, "no_parking"); + 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()); - if tags.get(osm::HIGHWAY) == Some(&"motorway".to_string()) - || tags.get(osm::HIGHWAY) == Some(&"motorway_link".to_string()) - || tags.get("junction") == Some(&"roundabout".to_string()) + tags.insert(osm::INFERRED_SIDEWALKS, "true"); + if tags.is_any(osm::HIGHWAY, vec!["motorway", "motorway_link"]) + || tags.is("junction", "roundabout") { - tags.insert(osm::SIDEWALK.to_string(), "none".to_string()); - } else if tags.get("oneway") == Some(&"yes".to_string()) { - tags.insert(osm::SIDEWALK.to_string(), "right".to_string()); - if tags.get(osm::HIGHWAY) == Some(&"residential".to_string()) { - tags.insert(osm::SIDEWALK.to_string(), "both".to_string()); + tags.insert(osm::SIDEWALK, "none"); + } else if tags.is("oneway", "yes") { + tags.insert(osm::SIDEWALK, "right"); + if tags.is(osm::HIGHWAY, "residential") { + 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) -> 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) -> 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) -> BTreeSet<(String, Stri amenities } -fn get_area_type(tags: &BTreeMap) -> Option { - if tags.get("leisure") == Some(&"park".to_string()) { +fn get_area_type(tags: &Tags) -> Option { + 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.get("natural") == Some(&"water".to_string()) - || tags.get("waterway") == Some(&"riverbank".to_string()) - { + + if tags.is("natural", "water") || tags.is("waterway", "riverbank") { 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()) - && tags.get("area") == Some(&"yes".to_string()) - { + if tags.is("highway", "pedestrian") && tags.is("area", "yes") { return Some(AreaType::PedestrianIsland); } } + None } @@ -605,7 +604,7 @@ fn glue_to_boundary(result_pl: PolyLine, boundary: &Ring) -> Option { } fn extract_route( - master_tags: &BTreeMap, + master_tags: &Tags, master_rel: &osm_xml::Relation, doc: &osm_xml::OSM, id_to_way: &HashMap>, diff --git a/map_model/src/make/buildings.rs b/map_model/src/make/buildings.rs index 4c4a586cdc..76022aaf0e 100644 --- a/map_model/src/make/buildings.rs +++ b/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, + 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::().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); -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 - } - } -} diff --git a/map_model/src/make/initial/lane_specs.rs b/map_model/src/make/initial/lane_specs.rs index 32ffdef9e7..593d04ff8f 100644 --- a/map_model/src/make/initial/lane_specs.rs +++ b/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) -> (Vec, Vec) { - 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) -> (Vec, Ve } // TODO Reversible roads should be handled differently? - let oneway = tags.is("oneway", "yes") - || tags.is("oneway", "reversible") - || tags.is("junction", "roundabout"); + let oneway = + tags.is_any("oneway", vec!["yes", "reversible"]) || 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::().ok()) { n - } else if let Some(n) = osm_tags - .get("lanes") - .and_then(|num| num.parse::().ok()) - { + } else if let Some(n) = tags.get("lanes").and_then(|num| num.parse::().ok()) { if oneway { n } else if n % 2 == 0 { @@ -51,15 +48,12 @@ pub fn get_lane_types(osm_tags: &BTreeMap) -> (Vec, 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::().ok()) { n - } else if let Some(n) = osm_tags - .get("lanes") - .and_then(|num| num.parse::().ok()) - { + } else if let Some(n) = tags.get("lanes").and_then(|num| num.parse::().ok()) { if oneway { 0 } else if n % 2 == 0 { @@ -80,7 +74,7 @@ pub fn get_lane_types(osm_tags: &BTreeMap) -> (Vec, 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) -> (Vec, 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) -> (Vec, 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) -> (Vec, Ve if tags.is("cycleway:right", "lane") { fwd_side.push(LaneType::Biking); } - if tags.is("cycleway:left", "lane") - || tags.is("cycleway:left", "opposite_lane") + if tags.is_any("cycleway:left", vec!["lane", "opposite_lane"]) || tags.is("cycleway", "opposite_lane") { back_side.push(LaneType::Biking); @@ -175,15 +168,11 @@ pub fn get_lane_types(osm_tags: &BTreeMap) -> (Vec, Ve } if driving_lane == LaneType::Driving { - fn has_parking(value: Option<&String>) -> bool { - value == Some(&"parallel".to_string()) - || value == Some(&"diagonal".to_string()) - || value == Some(&"perpendicular".to_string()) - } - 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)); + let has_parking = vec!["parallel", "diagonal", "perpendicular"]; + let parking_lane_fwd = tags.is_any(osm::PARKING_RIGHT, has_parking.clone()) + || tags.is_any(osm::PARKING_BOTH, has_parking.clone()); + let parking_lane_back = tags.is_any(osm::PARKING_LEFT, has_parking.clone()) + || tags.is_any(osm::PARKING_BOTH, has_parking); if parking_lane_fwd { fwd_side.push(LaneType::Parking); } @@ -210,15 +199,6 @@ pub fn get_lane_types(osm_tags: &BTreeMap) -> (Vec, 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); -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 { diff --git a/map_model/src/osm.rs b/map_model/src/osm.rs index 6a6e792a9c..a24c56aaac 100644 --- a/map_model/src/osm.rs +++ b/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";