Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Parse placement tags and shift roads accordingly #139

Merged
merged 13 commits into from
Dec 4, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion osm2streets-java/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@ impl StreetNetwork {

let clip_pts = None;
let mut timer = Timer::throwaway();
let mut network =
let (mut network, _) =
streets_reader::osm_to_street_network(&osm_xml_input, clip_pts, cfg, &mut timer)
.unwrap();
let transformations = Transformation::standard_for_clipped_areas();
Expand Down
4 changes: 2 additions & 2 deletions osm2streets/src/intersection.rs
Original file line number Diff line number Diff line change
Expand Up @@ -143,9 +143,9 @@ impl StreetNetwork {
// road.center_pts is unadjusted; it doesn't handle unequal widths yet. But that
// shouldn't matter for sorting.
let center_pl = if road.src_i == i {
road.untrimmed_center_line.reversed()
road.reference_line.reversed()
} else if road.dst_i == i {
road.untrimmed_center_line.clone()
road.reference_line.clone()
} else {
panic!("Incident road {r} doesn't have an endpoint at {i}");
};
Expand Down
125 changes: 125 additions & 0 deletions osm2streets/src/lanes/mod.rs
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
mod classic;
mod osm2lanes;
mod placement;
#[cfg(test)]
mod tests;

Expand Down Expand Up @@ -87,6 +88,47 @@ impl LaneType {
}
}

/// Determines if the lane is a travel lane that is represented in OSM `*:lanes` tags.
/// Note that the `lanes` tag counts car driving lanes, excluding bike lanes, whereas the
/// `:lanes` suffix specifies that each lane, including bike lanes, should have a value between
/// `|`s. This function identifies the latter kind.
Comment on lines +92 to +94
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know if you were aware of this, it's not very well presented on the wiki and has big implications in osm2lanes parsing.

pub fn is_tagged_by_lanes_suffix(&self) -> bool {
match self {
LaneType::Driving => true,
LaneType::Biking => true, // FIXME depends on lane vs track
LaneType::Bus => true,
LaneType::Parking => false,
LaneType::Sidewalk => false,
LaneType::Shoulder => false,
LaneType::SharedLeftTurn => true,
LaneType::Construction => true,
LaneType::LightRail => false,
LaneType::Buffer(_) => false,
LaneType::Footway => false,
LaneType::SharedUse => false,
}
}

/// Determines if the lane is part of the roadway, the contiguous sealed surface that OSM
/// mappers consider the "road".
pub fn is_roadway(&self) -> bool {
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This function (and is_tagged_by_lanes_suffix above) might need to be moved to LaneSpec, with some extra info stored to answer them properly. Or this gets clearer when we improve LaneType to tease apart the multiple different properties it is trying to represent.

The is_roadway concept that I use here might need to be a field on LaneSpec that is determined at parsing time.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Adding to LaneSpec and/or refining LaneType sounds fine to me. I still would like to avoid the full generality of osm2lanes though.. https://github.com/a-b-street/osm2lanes/blob/main/osm2lanes/src/road/lane.rs can express nonsensical things like a parking lane for Foot.

Rephrasing... if we start to refactor LaneType dramatically, let's please do it in a separate PR and a bit slowly, so I can keep downstream A/B Street code abreast of changes. #91 is an example that would probably be great to do in this repo, but I'd like time to support it everywhere

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

separate PRs and a bit slowly.

Absolutely, I'm enjoying the standard set by supporting A/B Street at each iteration.

I still would like to avoid the full generality of osm2lanes though.

This is an opportunity to see what a useful osm2lanes API would look like, so we can carve osm2lanes down into the support for this lib. RoadPosition/Placement is an example of the kind of complexity we end up needing (i think).

I think it's ok that the types (aka vocabulary) let you say semantically nonsense things like "pedestrian parking". What if someone tagged it as park of crowd/queue management at some arena or something? We shouldn't crash. Parsing tags into fields seems simpler than trying to classify into an enum of distinct types right at the beginning. If parking can be tagged with access restrictions, then it's up to the renderer to scratch its head when it tries to find lane markings for foot parking.

match self {
LaneType::Driving => true,
LaneType::Biking => true, // FIXME depends on lane vs track
LaneType::Bus => true,
LaneType::Parking => true, // FIXME depends on on-street vs street-side
LaneType::Sidewalk => false,
LaneType::Shoulder => true,
LaneType::SharedLeftTurn => true,
LaneType::Construction => true,
LaneType::LightRail => true, // FIXME only for trams
LaneType::Buffer(BufferType::Curb) => false,
LaneType::Buffer(_) => true,
LaneType::Footway => false,
LaneType::SharedUse => false,
}
}

pub fn is_walkable(self) -> bool {
matches!(
self,
Expand Down Expand Up @@ -355,3 +397,86 @@ impl fmt::Display for Direction {
}
}
}

/// Refers to a lane by its left-to-right position among all lanes in that direction. Backward
/// lanes are counted left-to-right from the backwards direction.
///
/// e.g. The left-most forward lane is `LtrLaneNum::Forward(1)` and the backward lane furthest to
/// the road-right is `LtrLaneNum::Backward(1)`, because of the backward perspective.
#[derive(Clone, Copy, Debug, PartialEq, Serialize, Deserialize)]
pub enum LtrLaneNum {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Before I read the below comment, I was confused what this meant. Always counting from the left, its the 0th, 1st, 2nd, etc lane that's forwards or backwards, regardless of contraflow. What about lanes:both_ways?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pretty much that (but starting at 1). Contraflow lanes are not mentioned anywhere, but a strict reading of "the forward lanes in LtR order" can be followed by mappers and consumers equally well. Does placement:backward=left_of:3 look like V|^^VV?

Like tagging turn:lanes[:forward]=left|left;through|through, three lanes, 1, 2, 3.

This is where i need to summarise the idea in comments! And link to the wiki quotes.

Forward(usize),
Backward(usize),
}

impl LtrLaneNum {
pub fn direction(&self) -> Direction {
match self {
Self::Forward(_) => Direction::Fwd,
Self::Backward(_) => Direction::Back,
}
}

pub fn number(&self) -> usize {
match self {
Self::Forward(num) | Self::Backward(num) => *num,
}
}

/// Converts to the same numbered lane in the opposite direction.
pub fn reverse(&self) -> Self {
use LtrLaneNum::*;
match self {
Forward(n) => Backward(*n),
Backward(n) => Forward(*n),
}
}
}

/// Identifies a position within the width of a roadway. Lanes are identified by their left-to-right
/// position, as per the OSM convention.
Comment on lines +436 to +437
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Early on I was tempted to represent this using Inside/OutsideOf(i32) naming lanes relative to the separation (with negative indicating lanes on the backward side) and adding an invariant to Road that it stores its separation. After working with the minutia of calculating things from the left edge using lanes_ltr I'm more comfortable with it - in big part because my understanding is that in the presence of contraflow lanes, *:lanes:forward tags, and thus placement:forward, refer to the forward lanes in lrt order, regardless of their position.

///
/// Most commonly seen as a value of the placement tag, e.g.
/// `placement=right_of:1` means that the OSM way is drawn along the right edge of lane 1.
#[derive(Clone, Copy, Debug, PartialEq, Serialize, Deserialize)]
pub enum RoadPosition {
/// The center of the carriageway width, ignoring lanes. The default placement of OSM ways.
Center,
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seems awkward that this type uses "center" and "middle" to mean the same thing, but "center" matches with the convention in this repo, and "middle" matches the placement tag. Opinion?

/// The center of the full width of a `Road`, including verges and footpaths.
FullWidthCenter,
/// The center of the separation between both directions of traffic, i.e. the dividing line,
/// median, or shared turning lane. For a oneway road, this is the "inside" edge of the road,
/// i.e. the right side of LHT and the left side of RHT.
Separation,
/// On the left edge of the named lane (from the direction of the named lane).
LeftOf(LtrLaneNum),
/// In the middle of the named lane.
MiddleOf(LtrLaneNum),
/// On the right edge of the named lane (from the direction of the named lane).
RightOf(LtrLaneNum),
Comment on lines +451 to +456
Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The other way to do this could be ForwardLane(usize, LanePosition) and BackwardLane with enum LanePosition { Left, Right, Middle }.

}

impl RoadPosition {
/// Converts to the same placement interpreted from the other direction. That is, only the
/// wrapped LtrLaneNum is reversed.
pub fn reverse(self) -> Self {
use RoadPosition::*;
match self {
Center | FullWidthCenter | Separation => self,
LeftOf(n) => LeftOf(n.reverse()),
MiddleOf(n) => MiddleOf(n.reverse()),
RightOf(n) => RightOf(n.reverse()),
}
}
}

/// Describes the placement of a line (such as the OSM Way) along a road.
#[derive(Clone, Copy, Debug, PartialEq, Serialize, Deserialize)]
pub enum Placement {
/// Along the specified position down the entire length.
Consistent(RoadPosition),
/// Varying linearly from a specified position at the start, to a different one at the end.
Varying(RoadPosition, RoadPosition),
/// Varying linearly from some unspecified position at the start, to a different one at the end.
Transition,
}
170 changes: 170 additions & 0 deletions osm2streets/src/lanes/placement.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,170 @@
use abstutil::Tags;
use anyhow::Result;

use super::{LtrLaneNum, Placement, RoadPosition};

use Placement::*;
use RoadPosition::*;

impl RoadPosition {
/// Tries to parse a road position from an osm tag value as per the `placement` scheme.
/// See https://wiki.openstreetmap.org/wiki/Proposed_features/placement#Tagging
///
/// The direction is treated as forward, use `reverse()` on the result if the context is backward.
pub fn parse(value: &str) -> Result<Self> {
match value {
"" => Ok(Center),
"separation" => Ok(Separation),
_ => {
if let Some((kind, lane_str)) = value.split_once(':') {
if let Ok(lane) = lane_str.parse::<usize>() {
match kind {
"left_of" => Ok(LeftOf(LtrLaneNum::Forward(lane))),
"middle_of" => Ok(MiddleOf(LtrLaneNum::Forward(lane))),
"right_of" => Ok(RightOf(LtrLaneNum::Forward(lane))),
_ => bail!("unknown lane position specifier: {kind}"),
}
} else {
bail!("bad lane number: {lane_str}")
}
} else {
bail!("unknown placement value: {value}")
}
}
}
}
}

impl Placement {
/// Tries to parse a placement from a set of OSM tags according to the `placement` scheme.
/// See https://wiki.openstreetmap.org/wiki/Proposed_features/placement#Tagging
///
/// Limitations:
/// - Doesn't validate tag combos, just returns the first interpretation it finds.
/// - Doesn't allow `:end` and `:start` to mix `:forward` and `:back`. Maybe it should?
pub fn parse(tags: &Tags) -> Result<Self> {
if let Some(transition_or_pos) = tags.get("placement") {
if transition_or_pos == "transition" {
Ok(Transition)
} else {
Ok(Consistent(RoadPosition::parse(transition_or_pos.as_str())?))
}
} else if tags.has_any(vec!["placement:start", "placement:end"]) {
Ok(Varying(
RoadPosition::parse(tags.get("placement:start").map_or("", |s| s.as_str()))?,
RoadPosition::parse(tags.get("placement:end").map_or("", |s| s.as_str()))?,
))
} else if let Some(pos) = tags.get("placement:forward") {
Ok(Consistent(RoadPosition::parse(pos.as_str())?))
} else if tags.has_any(vec!["placement:forward:start", "placement:forward:end"]) {
Ok(Varying(
RoadPosition::parse(
tags.get("placement:forward:start")
.map_or("", |s| s.as_str()),
)?,
RoadPosition::parse(tags.get("placement:forward:end").map_or("", |s| s.as_str()))?,
))
} else if let Some(backwards_pos) = tags.get("placement:backward") {
Ok(Consistent(
RoadPosition::parse(backwards_pos.as_str())?.reverse(),
))
} else if tags.has_any(vec!["placement:backward:start", "placement:backward:end"]) {
Ok(Varying(
RoadPosition::parse(
tags.get("placement:backward:start")
.map_or("", |s| s.as_str()),
)?
.reverse(),
RoadPosition::parse(
tags.get("placement:backward:end")
.map_or("", |s| s.as_str()),
)?
.reverse(),
))
} else {
Ok(Consistent(Center)) // The default when not tagged.
}
}
}

#[cfg(tests)]
mod tests {
use super::*;
use std::collections::BTreeMap;
use LtrLaneNum::*;

#[test]
fn road_position_parses() {
assert_eq!(RoadPosition::parse("").unwrap(), Center);
assert_eq!(RoadPosition::parse("separation").unwrap(), Separation);
assert_eq!(
RoadPosition::parse("left_of:1").unwrap(),
LeftOf(Forward(1))
);
assert_eq!(
RoadPosition::parse("middle_of:1").unwrap(),
MiddleOf(Forward(1))
);
assert_eq!(
RoadPosition::parse("right_of:1").unwrap(),
RightOf(Forward(1))
);
}

#[test]
fn placement_parses() {
assert_eq!(
Placement::parse(&Tags::new(BTreeMap::from([(
"placement".into(),
"transition".into()
)])))
.unwrap(),
Transition
);

assert_eq!(
Placement::parse(&Tags::new(BTreeMap::from([(
"placement".into(),
"right_of:1".into()
)])))
.unwrap(),
Consistent(RightOf(Forward(1)))
);

assert_eq!(
Placement::parse(&Tags::new(BTreeMap::from([(
"placement:forward".into(),
"right_of:1".into()
)])))
.unwrap(),
Consistent(RightOf(Forward(1)))
);

assert_eq!(
Placement::parse(&Tags::new(BTreeMap::from([(
"placement:backward".into(),
"right_of:1".into()
)])))
.unwrap(),
Consistent(RightOf(Backward(1)))
);

assert_eq!(
Placement::parse(&Tags::new(BTreeMap::from([
("placement:start".into(), "right_of:1".into()),
("placement:end".into(), "right_of:2".into())
])))
.unwrap(),
Varying(RightOf(Forward(1)), RightOf(Forward(2)))
);

assert_eq!(
Placement::parse(&Tags::new(BTreeMap::from([
("placement:backward:start".into(), "right_of:1".into()),
("placement:backward:end".into(), "right_of:2".into())
])))
.unwrap(),
Varying(RightOf(Backward(1)), RightOf(Backward(2)))
);
}
}
6 changes: 3 additions & 3 deletions osm2streets/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -156,7 +156,7 @@ impl StreetNetwork {
for road in self.roads_per_intersection(endpts[0]) {
// trimmed_center_line hasn't been initialized yet, so override this
let mut input = road.to_input_road();
input.center_pts = road.untrimmed_road_geometry().0;
input.center_pts = road.untrimmed_road_geometry(self.config.driving_side).0;
input_roads.push(input);
}
let mut results = intersection_polygon(
Expand All @@ -176,7 +176,7 @@ impl StreetNetwork {
if road.id == road_id {
input.center_pts = trimmed_center_pts.clone();
} else {
input.center_pts = road.untrimmed_road_geometry().0;
input.center_pts = road.untrimmed_road_geometry(self.config.driving_side).0;
}
input_roads.push(input);
}
Expand Down Expand Up @@ -228,7 +228,7 @@ impl StreetNetwork {
pub(crate) fn debug_road<I: Into<String>>(&self, r: RoadID, label: I) {
if let Some(step) = self.debug_steps.borrow_mut().last_mut() {
step.polylines
.push((self.roads[&r].untrimmed_road_geometry().0, label.into()));
.push((self.roads[&r].center_line.clone(), label.into()));
}
}
}
Expand Down
Loading