Parse `placement` tags and shift roads accordingly #139

BudgieInWA · 2022-11-30T09:12:10Z

I have created the types RoadPosition and Placement, which I believe captures everything that we will need to contend with as we flesh out the support for placement. I see RoadPosition being useful in general, for talking about and working with different road positions. E.g. it might make sense to add LeftEdge and RightEdge etc.

I have renamed the two center line fields in order to see how it feels this way. The idea is that reference_line remains where the osm way is because it might be useful to have easy access to that. The main benefit I see is that the ends will all join up in the same way they do in OSM, which has some nice planar-graph properties, useful for sorting clockwise order, for instance. After shifting the lines, they may cross over each other in confusing ways (especially the nonsense fist/last segments). Though maybe it's better to refer back to the original ways in OsmExtract for cases like that, I don't know yet.

I haven't followed through all the uses of reference_line and center_line yet - I wanted your input on that - to make sure they are using the right one in the right way. We have some decisions to make about this, in particular deciding what transformations should work with/modify, and if we should drop reference_line . Hopefully the concepts and terminology make it clear what should be used at each site.

We will need to calculate the appropriate Placement::Varying for Roads with Placement::Transition after we have determined the lane connectivity. Or maybe make up our own geometry for such roads that join a Connection or Fork, because the reference line often goes outside the road completely.

I haven't inspected or accepted the test diffs yet either, except for the new dedicated placement test, that looks great when it works, and broke one intersection.

BudgieInWA

I've left out propagation of actual errors to keep things as clear as possible for now, and I don't know what error reporting approach we want to build towards.

BudgieInWA · 2022-11-30T09:17:06Z

osm2streets/src/lanes/mod.rs

+
+    /// 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 {


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.

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

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.

BudgieInWA · 2022-11-30T09:18:26Z

osm2streets/src/lanes/mod.rs

+    /// 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.


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.

BudgieInWA · 2022-11-30T09:24:22Z

osm2streets/src/lanes/mod.rs

+    /// 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),


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

BudgieInWA · 2022-11-30T09:31:59Z

osm2streets/src/lanes/mod.rs

+/// Identifies a position within the width of a roadway. Lanes are identified by their left-to-right
+/// position, as per the OSM convention.


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.

BudgieInWA · 2022-11-30T09:33:30Z

osm2streets/src/lanes/mod.rs

+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, PartialOrd, Ord, Serialize, Deserialize)]
+pub enum RoadPosition {
+    /// The center of the carriageway width, ignoring lanes. The default placement of OSM ways.
+    Center,


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?

BudgieInWA · 2022-11-30T09:36:07Z

osm2streets/src/lanes/placement.rs

+    }
+}
+
+impl TryFrom<&Tags> for Placement {


I am very happy with treating the parsing step of the whole scheme independently from seeing if the semantics of the tag values match the road they are tagged on.

Do you think I should back out from implementing the traits though? Or is this the sort of thing you are interested in working towards?

The separate parsing is awesome, and the unit tests show the niceness. I would vote to just change the API to impl Placement { fn parse(tags: &Tags) -> anyhow::Result<Self> } though. I'm not sure how the wider Rust community feels or what's standard in codebases, but the from/into traits don't show up as clearly in rustdocs and they're visually less clear to spot in code. (Something like let x: Placement = some_complicated_thing().try_into()? vs let x = Placement::parse(some_complicated_thing())?)

impl Placement { fn parse(tags: &Tags) -> anyhow::Result<Self> }
Great, I like standardising on using conventional language (parse, like with new) and leaving traits for when a usecase actually arises :)

parse is the right term too. The trait didn't even bring a solution for the errors, so anyhow is a useful suggestion. I was imagining upgrading from () to at least strings, but using anyhow::Result everywhere lets us use ? everywhere, right? I might let that happen later too.

Yep, anyhow or any other error type lets us use ?. anyhow is encouraged for applications and discouraged for libraries, but similar to the thread below about error/warning handling, I don't think increased code complexity is worth it here yet. We can be lazy and use string errors

BudgieInWA · 2022-11-30T09:39:08Z

osm2streets/src/road.rs

+        //TODO use self.left_edge_offset_of(RoadPosition::Center) or something?
+        let mut true_center = self.reference_line.clone();


I hope you can figure out the correct semantics here, depending on how untrimmed_geometry is used.

untrimmed_road_geometry is one of these weird bits of legacy code that I don't remember the original reason for. It just looks for cases where a road has a sidewalk on exactly one side, and shifts over the center line a bit. Most of the uses are in transformations just to check length, and those places could use reference_line.length() today instead. And once we maintain center_line at all times, most/all probably should use that.

The one "real" use of this is in the very beginning of transform/intersection_geometry.rs. We initialize a road's center_line to this. I think we want to ditch this method entirely and just use the corrected center that's now calculated from placement.

I think I understand what is going on with all of these. It looks like the extra logic here is trying to pretend the line is placed at RoadPosition::Center, so I have implemented it to behave like that. I have touched all the uses of this fn (to add the driving_side param), but mostly left them as is. Ideally we replace the uses with more appropriate lookups as we go.

dabreegster

I'll take another shot at reviewing in the morning, I can't process most of this right now. Responding to some of your comments though

dabreegster · 2022-11-30T19:37:13Z

osm2streets/src/lanes/mod.rs

@@ -355,3 +356,73 @@ impl fmt::Display for Direction {
        }
    }
 }
+
+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, PartialOrd, Ord, Serialize, Deserialize)]
+pub enum LtrLaneNum {


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?

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.

dabreegster · 2022-11-30T19:41:32Z

osm2streets/src/lanes/mod.rs

@@ -355,3 +356,73 @@ impl fmt::Display for Direction {
        }
    }
 }
+
+#[derive(Clone, Copy, Debug, Eq, Hash, PartialEq, PartialOrd, Ord, Serialize, Deserialize)]


nitpick: I'm guessing you copied the derive. Do we need to hash, serialize, order these?

dabreegster · 2022-11-30T19:52:25Z

osm2streets/src/road.rs

@@ -239,6 +278,137 @@ impl Road {
        self.lane_specs_ltr.iter().map(|l| l.width).sum()
    }

+    /// Calculates the number of (forward, both_ways, backward) lanes. The order of the lanes
+    /// doesn't matter.
+    pub fn driving_lane_counts(&self) -> (usize, usize, usize) {


driving_lane makes me think LaneType::Driving, but you're looking for the lanes suffix

You're right, this is more like travel_lane_count. OSM doesn't really provide a clear definition or terminology, and this fn isn't even used yet (I'm iterating from the right instead of doing a subtraction), but this is the sort of thing you'd use to validate if a right_of:12 is in bounds.

dabreegster · 2022-11-30T19:58:15Z

Though maybe it's better to refer back to the original ways in OsmExtract for cases like that, I don't know yet.

I like the new naming. And for now, keeping reference_line here seems fine.

We have some decisions to make about this, in particular deciding what transformations should work with/modify,

It'd probably be better to work off the trimmed and transformed center_line in transformations generally. Things like determining if a road is short or not doesn't really care about untransformed center. Things like estimate_trimmed_geometry are hacks right now because we're not calculating trimmed geometry early yet. I'm hoping to change that in #136, but likely this PR lands first

dabreegster · 2022-11-30T20:10:41Z

I've left out propagation of actual errors to keep things as clear as possible for now, and I don't know what error reporting approach we want to build towards.

We don't have a consistent story here yet. Feel free to sprinkle in warn! logging statements if it's useful.

Now that we've got an OSM lane editor actually wired up, I'm starting to think about this. I feel that osm2lanes went a bit overboard in creating types to precisely capture all possible things that can go wrong, but the end result is hard to use in the code, the final error messages can wind up nonsense (the tags.subset thing often doesn't actually plumb through the context needed to understand an error), and the immediate way we'd want to use these is probably just as a string message anyway. For something like get_lane_specs_ltr, I feel like just plumbing back a list of string warnings could be useful to display. In the broader osm2streets, maybe we could have a streets.record_warning(...) API build up a list of string errors and maybe display somewhere.

BudgieInWA · 2022-12-01T01:09:37Z

I'm hoping to calculating trimmed geometry early in #136, but likely this PR lands first

Yep, bit by bit!

It'd probably be better to work off the trimmed and transformed center_line in transformations generally.

That's the idea. We just need to figure out the bare minimum sensible behaviour for existing uses, probably sprinkle some update_center_line calls around.

Also, new screenshots in the opening comment!

dabreegster · 2022-12-01T09:52:15Z

osm2streets/src/road.rs

+                warn!("varying placement not yet supported, using placement:start");
+                p
+            }
+            Placement::Transition => {


Finally reading the Wiki...

Instead of the value transition, one can use the suffixes :start and :end to provide information about the placement at the start resp. end of the OSM.way. This is usually not necessary if such information can be retrieved from the previous resp. following OSM way.

To later support this correctly, we have to look at the previous and next OSM ways (and decide which one is "the same road" -- probably just by name?) and interpret explicit or implicit placement tags there?

We look at the traffic flow lane by lane to determine the lane connections. With Connections and Forks you can examine movements, do lane math and divvy up the lanes easily. Make some assumptions if its not trivial, as the mapper can tag the lane connectivity explicitly too.

I think we are supposed to tag placement=transition on the nonsense segments of motorway exits (the ones that go way outside the lanes that they represent, and so couldn't be tagged with any sensible placement:start anyway). We probably just assume them to be at the start of the applicable roads automatically, because we need to do it all over the place anyway.

When both Roads have their placement tagged (as non-transition), that actually tells us the lane connectivity at that node. Try mapping a segment where a right turn lane appears for a short distance and the ways stay straight with right_of:1 for the 2 lane and 3 lane segments.

dabreegster

I'm starting to look at some of the visual diffs. https://wiki.openstreetmap.org/wiki/Proposed_features/placement#Exemplary_tags:_one-way_road has some nice synthetic examples. I wonder if we could/should use Overpass and look for clear places demonstrating some of these? Or does fremantle_placement test things adequately do you think? It looks like it only uses right_of (with one transition)

dabreegster · 2022-12-01T10:03:07Z

osm2streets/src/transform/intersection_geometry.rs

@@ -4,12 +4,6 @@ use geom::{Circle, Distance};
 use crate::{IntersectionControl, IntersectionKind, StreetNetwork};

 pub fn generate(streets: &mut StreetNetwork, timer: &mut Timer) {
-    // Initialize trimmed_center_line to the corrected center


I'm still puzzling through how it happens, but take a look at the diff in aurora_sausage_link. It looks like we lose clipping to the map edge. In Road::new we're immediately doing update_center_line, so this should be set...
OHHH no it's simple. At the end of streets_reader/src/clip.rs, we need to call update_center_line. #127 would make this simpler possibly

The approach in this PR is to treat center_line as derived state and update it as soon as its invalidated (like kind). 4b03cdc actually follows through on that and adds the required update_center_line calls (after the initial one in Road::new), which fixes the clip problem, and the one in my original screenshot.

BudgieInWA · 2022-12-04T10:11:29Z

I'm ready for this to merge and to fix things in follow ups, if that works for you within your geom refactor workflow.

I have looked through most of the tests and things look good. Some intersections have different shapes, which I guess comes from a different untrimmed_road_geometry and get_untrimmed_sides implementation. I think the new ones are more accurate (unless they are buggy, I haven't tested them very thoroughly).

Running StreetExplorer with new code but letting it load old test geom files from the dir gives a good way to diff the output: The "original test geometry" layer can be turned back on after generating lane markings, to see where the shapes have changed! So I haven't commited any tests yet. Feel free to do that if you are ready to merge.

One big question is the reference_line/center_line source of truth divide, which is an open question for now, which suggests merging is appropriate.

Another piece that we want soon, but probably after this PR merges, is to improve the implementation of PolyLine::shift so that a different start and end offset can be provided. The amount to shift each point should simply be a linear interpolation between start_offset and end_offset (based on distance along the line), with the remaining logic remaining as is, I think.

dabreegster · 2022-12-04T10:35:51Z

osm2streets/src/lanes/placement.rs

@@ -23,66 +22,60 @@ impl TryFrom<&str> for RoadPosition {
                            "left_of" => Ok(LeftOf(LtrLaneNum::Forward(lane))),
                            "middle_of" => Ok(MiddleOf(LtrLaneNum::Forward(lane))),
                            "right_of" => Ok(RightOf(LtrLaneNum::Forward(lane))),
-                            _ => Err(()),
+                            _ => Err(anyhow!("unknown lane position specifier: {kind}")),


You may also be able to use the bail! macro here, which does return Err(anyhow!(...))

dabreegster · 2022-12-04T10:41:13Z

Awesome, this is a huge step forwards! I will:

Use the bail! macro in the parse routines if it helps readability
Fix merge conflicts
Run the tests and commit all the results if they look good
Merge

in the next hour or two, if that's the plan.

Running StreetExplorer with new code but letting it load old test geom files from the dir gives a good way to diff the output

Oh yeah, I completely forgot we had that! Much easier than switching between browser tabs

improve the implementation of PolyLine::shift so that a different start and end offset can be provided

Ooh, this sounds exciting! Like everything else, the implementation there is on the scary-legacy-code side. We can definitely have a go at cleaning it up. And georust/geo#935 is in-progress to implement this logic in georust in a much more rigorous and well-tested way.

RoadPosition describes the positions in the width of the road like "the middle of lane 1" or "the separation line". Placement describes the placement of a line over a segment of road, the whole thing conveyed by the `placement` tag and its subtags.

…ter_line appropriately.

…d lanes, give `RoadPosition::FullWidthCenter` instead Centering on the highest tier of lane might be better in the long term...

…oad position `untrimmed_road_geometry` returns a line along `RoadPosition::Center`, which is what the `true_center` calculation was doing (when it found the right conditions). `get_untrimmed_sides` uses the actual `reference_line` offset instead of assuming it is `RoadPosition::FullWidthCenter`. Ideally, more of the usages could be switched out for something using `center_line` instead.

Don't know if I've found all such cases yet.

dabreegster · 2022-12-04T12:07:59Z

Test changes are mixed. Perth stretched lights and peanut get much better; almost-parallel roads that were previously overlapping are now set apart a bit. There are some cases where the intersection geometry no longer matches up with the road, and I'm not entirely sure why yet... borough, bristol sausage link, and service road loop. Some examples:

And a few intersections get much worse, possibly due to the problem above. Northgate, quad_intersection, and the 2 Tempe tests:

I have a suspicion we're initializing from the wrong reference line when calculating geometry that I'll check into briefly, but I'm happy merging this and iterating

dabreegster · 2022-12-04T12:19:02Z

osm2streets/src/transform/separate_cycletracks.rs

@@ -83,7 +83,7 @@ fn find_cycleways(streets: &StreetNetwork) -> Vec<Cycleway> {
                {
                    cycleways.push(Cycleway {
                        cycleway: cycleway_road.id,
-                        cycleway_center: cycleway_road.untrimmed_road_geometry().0,
+                        cycleway_center: cycleway_road.center_line.clone(),


This is fine here, and is the kind of thing we'll want to do almost everywhere. untrimmed_road_geometry should almost never be used once we maintain center_line as we go

dabreegster · 2022-12-04T12:29:10Z

I'm happy merging and iterating in smaller steps

BudgieInWA · 2022-12-04T13:40:54Z

And a few intersections get much worse... I have a suspicion we're initializing from the wrong reference line when calculating geometry

I'd guess one of the update_center_lines I added in ce540b1 happens after a trim_for_merging or something was applied. I've been practically blind to the trim distance handling, and update_center_lines doesn't incorporate it yet.

BudgieInWA requested a review from dabreegster November 30, 2022 09:12

BudgieInWA commented Nov 30, 2022

View reviewed changes

dabreegster reviewed Nov 30, 2022

View reviewed changes

dabreegster reviewed Dec 1, 2022

View reviewed changes

dabreegster reviewed Dec 4, 2022

View reviewed changes

BudgieInWA and others added 10 commits December 4, 2022 11:35

Add a test case with lots of placement tags

128aa44

Rename (un)trimmed_center_line to reference_line and center_line

79eef65

Add Road::left_edge_offset_of(RoadPosition) and use it to shift cen…

94147c7

…ter_line appropriately.

When asking for offset of RoadPosition::Center and there are no roa…

1adb096

…d lanes, give `RoadPosition::FullWidthCenter` instead Centering on the highest tier of lane might be better in the long term...

Update center_line after modifying reference_line in transformations

717ad95

Don't know if I've found all such cases yet.

Tweak docstrings, names, derives

b8fee5b

Simplify try_from to parse with anyhow

bfcdf55

Fix rebase

8d20e2d

dabreegster force-pushed the placement branch from 04ea83d to 8d20e2d Compare December 4, 2022 11:47

dabreegster added 2 commits December 4, 2022 11:48

Use bail! macro to clean up code very very slightly

5f59cc7

Accept test changes after placement PR

eb0d3dd

Fix tiny mistakes in the rebase

2e28f38

dabreegster reviewed Dec 4, 2022

View reviewed changes

dabreegster merged commit 4de2a7d into main Dec 4, 2022

dabreegster deleted the placement branch December 4, 2022 12:29

dabreegster mentioned this pull request Dec 30, 2022

Mark geometry lane based on placement= tags #6

Open

		/// Identifies a position within the width of a roadway. Lanes are identified by their left-to-right
		/// position, as per the OSM convention.

		//TODO use self.left_edge_offset_of(RoadPosition::Center) or something?
		let mut true_center = self.reference_line.clone();

Parse placement tags and shift roads accordingly #139

Parse placement tags and shift roads accordingly #139

Conversation

BudgieInWA commented Nov 30, 2022 • edited Loading

BudgieInWA left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabreegster left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabreegster commented Nov 30, 2022

dabreegster commented Nov 30, 2022

BudgieInWA commented Dec 1, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dabreegster left a comment

Choose a reason for hiding this comment

dabreegster Dec 1, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

BudgieInWA commented Dec 4, 2022

Choose a reason for hiding this comment

dabreegster commented Dec 4, 2022

dabreegster commented Dec 4, 2022

Choose a reason for hiding this comment

dabreegster commented Dec 4, 2022

BudgieInWA commented Dec 4, 2022

Parse `placement` tags and shift roads accordingly #139

Parse `placement` tags and shift roads accordingly #139

BudgieInWA commented Nov 30, 2022 •

edited

Loading

dabreegster Dec 1, 2022 •

edited

Loading