Add an optional layer showing existing turn restrictions. by andrewphilipsmith · Pull Request #1090 · a-b-street/abstreet

andrewphilipsmith · 2023-05-10T13:41:53Z

Draft for now

andrewphilipsmith · 2023-05-25T08:36:10Z

Here’s a screenshot of some progress. Basically, showing the original icons (now in purple) alongside svg icons deliberately offset 10m to the east.

In this screenshot, I’m using the icons from JOSM, but I won’t commit these to ABStreet without checking licensing/permissions etc.

Todo:

Decide on whether to use JOSM icons or not:
Handle cases where turn geometry does not match turn restriction tagging
Handle false-positive U-turn labelling

Details

Decide on whether to use JOSM icons or not:

Check JOSM icon licences/permissions/attribution requirements etc prior to including them.
If no to JOSM, find/make alternative icon

OSM tagging is applied to the legal turn, not the prohibited turn:

There are examples where turn restriction tags are applied to the legal turn, not the prohibited turn:

https://www.openstreetmap.org/relation/7710682

Because we’re using “the intersection geometry + the fact there is a banned turn at this intersection”, then this gets mapped the wrong way round (this should be a no-left turn):

I got lost in a bit of an OSM-wiki-rabbit-hole, but can’t find anything that says you shouldn’t map turn restrictions like this. I can kinda see why someone might think that way, and hence it might happen elsewhere too.

Hence I’m not sure where to put this down to incorrect OSM tagging or a situation we should handle.

False positives for U-turn

There are a few examples where the threshold for U-turns results in false positives. eg:

~~I could change the threshold easily enough but I wanted to check if there would be unexpected consequences.~~

EDIT: Whilst writing these notes, I found make_vehicle_turns, which has more detailed logic to avoid false positives. I’ll do a little be of refactoring so that we can reuse that logic here too.

dabreegster · 2023-05-25T08:55:24Z

OSM tagging is applied to the legal turn, not the prohibited turn:

Looking at the example, I'm confused in two ways. The second part of the relation is the north piece of Clayton Street, and so the relation is geometrically referring to a right turn, but it uses "no_left_turn." The left turn from Pink Lane would go against the one-way, so I think the relation is itself redundant; otherwise OSM would be littered with these redundant relations for every one-way road around.

I suspect this is incorrectly tagged. Should I start a thread in the osmus slack, a great place for these kinds of questions? I would say as a first cut, render what's in OSM to surface errors like this, and manually fix up problems we find. We could try to auto-detect certain classes of problems later and maybe hide them from being shown, but that feels less important (and maybe bad, because it stops bad OSM data from being dealt with).

Whilst writing these notes, I found make_vehicle_turns, which has more detailed logic to avoid false positives.

Refactoring sounds great! All of the turn generation code is old and probably convoluted, please ask questions early and often about weird things there! Maybe some of the refactoring effort here can get "upstreamed" into a-b-street/osm2streets#207 a little later.

andrewphilipsmith · 2023-06-23T21:12:50Z

I've made some more progress on this.

The goldenfiles now contain a section on the turn types and a section on turn restriction.
The tests programmatically compare the test result with the content of the goldenfiles on each run (rather than relying on overwriting them and noticing the changes in the version control).
I’ve wrapped the tests up in a #[cfg(test)]. This has been convenient to allow me to easily select individual tests from the command line. However, I strongly suspect that this is not the idiomatic thing to do. The Test Organization page in the Rust Book, doesn’t really cover integration tests spanning multiple crates and I’ve not looked beyond that, so I’ve left this as is for now.
I’ve made some minor changes to the docs that detail the integration tests. I’ll create a PR for this soon.

Remaining Todo (or at least make as separate issues):

For now I’ve stuck with the logic of adjusting the size of the icons to match the width of the road. I’m not convinced by this because in some cases the icons are very small. In the LTN mapping, the modal filters are fixed size. Visually the modal filters are more important than the turn restrictions. But perhaps it would be worth trying fix size turn signs, that as smaller than the model filter signs?
More important are cases where there are more than one turn restriction centred on the same node. Currently, this means that turn signs overlap and only the last-drawn sign is visible. (Here is an example https://www.openstreetmap.org/node/11134462)
An optional extra feature (though a possible partial solution to the overlapping sign problem above) - it would be nice to have a mouse-over feature. When you move your mouse over the turn-restriction sign, it highlights the relevant road segments. I’ve thought of a couple of ways I could do this, but it might depend on the work you’re doing on osm2streets.
I made some functions public for testing purposes. It would be appropriate to check that this is still required and revert to private wherever possible.

I have some thoughts about RestrictionType, TurnType, Turn and how they relate to each other, though I’ve no idea how feasible they are. Happy to talk about this at some point.

dabreegster

Awesome to see progress! If it's easier, feel free to split out some of the changes into a separate PR. The updates to the tests could be one candidate -- comparing goldenfiles programatically and expressing with cfg(test).

This has been convenient to allow me to easily select individual tests from the command line

There's an ancient crate buried in git history somewhere, but the reason the integration tests are written in a strange way is related to logging. The Rust test runner (as of ~2019, maybe it's gotten better) made it tough to stream test output and separate by test back when I was writing some of these originally, so I just wrote a regular binary. When I want to run one test, I usually temporarily comment stuff out. But if the Rust test runner has been working fine for you, we could convert everything to regular cfg(test). Also glancing through the tests, some look like they could be moved into more specific crates; there's no great reason to group integration tests like they are now.

Feel free to decouple this cleanup with this PR; after we decide what to do, we could do it separately before or after this PR.

it would be nice to have a mouse-over feature. When you move your mouse over the turn-restriction sign, it highlights the relevant road segments.

This sounds very useful! widgetry::mapspace::World should make it reasonably easy to write. There's a way to specify a custom Drawable for hover state, but in larger maps it might eat a lot of GPU memory to precalculate all of those. There's examples of lazily drawing stuff based on World hover state somewhere; I can dig it up if useful.

I’ve thought of a couple of ways I could do this, but it might depend on the work you’re doing on osm2streets.

So far no work there yet, and it might still be some time, so I'd go with the idea you have for now

More important are cases where there are more than one turn restriction centred on the same node

Also related to above. The next step to this work will be letting the user modify turn restrictions (deleting existing ones, and adding new ones). We should chat through the UX of this. Maybe for deleting, just clicking an icon could work. And for adding, perhaps clicking the source and target road? Based on what we might try and do next, we can think through how to handle multiple icons overlapping.

I have some thoughts about RestrictionType, TurnType, Turn and how they relate to each other

Let's sync about it this week!

dabreegster · 2023-06-24T15:52:43Z

    // TODO Also return distance along self of the hit
    pub fn intersection(&self, other: &PolyLine) -> Option<(Pt2D, Angle)> {
-        assert_ne!(self, other);
+        if self == other {


If you sync and rebase against main, this diff should go away -- it was added in 6c44da8

dabreegster · 2023-06-24T15:55:52Z

+
+// TODO: make private
+// Public for now, purely for testing purposes
+// TODO: would it be more appropriate to have this function live in `map_model/src/make/turns.rs`?


I think it belongs best in map_model/src/objects/turn.rs, maybe as a method on Map. make is meant for code that transforms a RawMap into a full serializable Map.

Often I first add code to something like the ltn crate, keeping compilation times low. Once it's stable, moving it to a lower level is nice. But if you edit map_model and test through an app, you'll unfortunately hit worse compile times

dabreegster · 2023-06-24T15:57:27Z

+        ));
+        let turn_types = all_turn_info_as_string(&map);
+        // TODO - I'm sure there is an more idiomatic way to do this.
+        assert!(match compare_with_goldenfile(turn_types, path_types) {


How about something like

if let Err(err) = compare_with_goldenfile(turn_types, path_types) { panic!("Error in compare_with_goldenfile: {err}"); }

The separate print is followed by an assert anyway, so just panicking is fine

Wait, if this returns Result, you can just call unwrap, or to add extra context with the error, expect

dabreegster · 2023-06-28T13:16:37Z

Notes from discussion. Rather than interpret turn_restrictions, complicated_turn_restrictions, and all the other raw OSM level info, we can take advantage of existing logic that generates lane-level Turns and road-level Movements and figure out what's banned from that.

Change make_all_turns to return two Vec<Turn>s. The first is legal turns, the second is all the illegal stuff that currently gets filtered out. Continue to only store legal Turn when we serialize Map.
In the LTN tool, we can summarize all of the road-to-road turn restrictions in a way that's useful for drawing and editing. First call make_all_turns and only keep the second list, of illegal turns.
Then refactor Movement::for_i to take a specific list of turns as input. It groups Turns into Movements, which are at the entire road level.
To figure out restricted movements, we have to do set difference of illegal movements - legal movements. The reason is that when there are roads with dedicated turn lanes, there will be individual turns banned for some lanes, but not all.

This approach will break for complicated_turn_restrictions, but maybe we can just special-case that and continue to read those directly from Road as we do today.

could use some work.

This enables banned turn icons to be drawn for complicated_turn_restrictions. The icons are in the correct locations, though are not always legible.

This corrects the results in the "false_positive_u_turns.txt" golden file.

andrewphilipsmith · 2023-07-05T14:55:30Z


-use crate::{Intersection, Lane, LaneID, LaneType, Map, RoadID, Turn, TurnID, TurnType};
+use crate::{
+    map::turn_type_from_road_geom, Intersection, Lane, LaneID, LaneType, Map, RoadID, Turn, TurnID,


This might be naitve, but this feels wrong. Previously everything else is imported is a impl/struct, but here I'm importing in a function, which seems inconsistant. I don't know if this matters though.

I think it's fine. If there's an obvious struct where the method should exist on, we could move it there as an impl, but in this case, nothing jumps out -- maybe Map, maybe also Road, but a free-standing function is pretty clear.

andrewphilipsmith · 2023-07-05T15:00:30Z

@dabreegster I think this PR is ready to merge. Happy to hear your feedback.

dabreegster

Thanks for the awesome first PR! I'm good with merging this now, and we can follow up with lots of the other ideas discussed, iterating in smaller chunks

dabreegster · 2023-07-07T09:05:09Z

-use geom::Distance;
-use map_model::{AmenityType, ExtraPOIType, FilterType, Map};
+use geom::{Angle, Distance, Pt2D};
+// use map_model::make::turns::get_ban_turn_info;


Stray bit left in; I'll push a commit to this branch to clean it up

dabreegster · 2023-07-07T09:05:54Z

+        for (restriction, r2) in &r1.turn_restrictions {
+            // TODO "Invert" OnlyAllowTurns so we can just draw banned things
+            if *restriction == RestrictionType::BanTurns {
+                println!(


Also going to remove these, otherwise the console will get spammy. We could use debug! and similar from the log crate if these are useful debugging to keep in

dabreegster · 2023-07-07T09:15:32Z

+    let no_left_t = "system/assets/map/no_left_turn.svg";
+    let no_u_t = "system/assets/map/no_u_turn_left_to_right.svg";
+    let no_straight = "system/assets/map/no_straight_ahead.svg";
+    // TODO - what should we do with these?


Hmm, we don't expect the map model to ever tell us turn restrictions for crosswalks and similar -- that'd just be expressed by a lack of a crosswalk. We can keep things like this for now, but maybe as a future cleanup, it'd be OK to use unreachable!() in the match below on turn types

dabreegster · 2023-07-07T09:16:24Z

The icons are very nice, by the way! For my own curiosity, are these made in inkscape or something else?

I made them in Figma. The page is here.

I tried Inkscape originally, but the resulting files didn't display properly. Based on your comment here I switched to Figma which produced more compact files.

dabreegster · 2023-07-07T09:18:17Z


-use crate::{Intersection, Lane, LaneID, LaneType, Map, RoadID, Turn, TurnID, TurnType};
+use crate::{
+    map::turn_type_from_road_geom, Intersection, Lane, LaneID, LaneType, Map, RoadID, Turn, TurnID,


I think it's fine. If there's an obvious struct where the method should exist on, we could move it there as an impl, but in this case, nothing jumps out -- maybe Map, maybe also Road, but a free-standing function is pretty clear.

dabreegster · 2023-07-07T10:19:26Z

    }
+
+    // TODO This returns a mixture of different things related to the turn. It might be better
+    // to create and return a `Turn`.


Hmm, yeah, maybe even a new struct would be OK here. Still reading through, not sure if I understand yet if the full Turn makes sense or not

dabreegster · 2023-07-07T10:21:33Z

+            .must_dist_along((if r1.src_i == i { 0.2 } else { 0.8 }) * r1.center_pts.length());
+
+        // Correct the angle, based on whether the vector direction is towards or away from the intersection
+        // TODO what is the standard way of describing the vector direction (rather than the traffic direction) for roads?


What do you mean by this? Like given a road, the angle from its first center line point to the last? I don't think we have anything special...

let pl = &road.center_pts; pl.first_pt().angle_to(pl.last_pt())

dabreegster · 2023-07-07T10:22:40Z

+            .center_pts
+            .must_dist_along((if r2.src_i == i { 0.2 } else { 0.8 }) * r2.center_pts.length());
+
+        // Correct the angle, based on whether the vector direction is towards or away from the intersection


We do this sort of logic everywhere in osm2streets, and @BudgieInWA has some ideas (written somewhere...) for expressing better. We can look at it in the future

dabreegster · 2023-07-07T10:26:13Z

Just noting, this looks like an extract of real OSM data, right? So far we've put only synthetic inputs here, but real inputs are a good idea too. Maybe in the future, we could use Osmium or other tools to strip out date/author/changeset tags and smush down the size tracked by git

dabreegster · 2023-07-07T10:27:22Z

+    if regenerate_goldenfiles {
+        // Automatically fail when the goldenfiles are regenerated. This is so the test is not accidentally
+        // left in a set where there goldenfiles are recreated on each run, and the test does not achieve its purpose.
+        assert!(false, "Automatically fail when the goldenfiles are regenerated. This is so the test is not accidentally left in a set where there goldenfiles are recreated on each run, and the test does not achieve its purpose.");


panic! could be a bit more idiomatic

dabreegster reviewed Jun 24, 2023

View reviewed changes

dabreegster and others added 17 commits June 28, 2023 17:25

Add an optional layer showing existing turn restrictions. The style

aae5578

could use some work.

Add debugging notes

a45078a

fixes crash on complicated_turn_restrictions

5af6dd4

This enables banned turn icons to be drawn for complicated_turn_restrictions. The icons are in the correct locations, though are not always legible.

draws turn restriction icons from svg files.

e7b0041

adds *.profraw files to .gitignore

e44660a

Add debugging notes

0125565

WIP: unfinished changes to switch laptops

641216a

WIP: unfinished changes to switch laptops

ab852b2

adds public domain turn restriction signs

d29b986

adds goldenfiles for turn restrictions

3abadac

combines turn_type and turn_restriction golden files

da02569

partial refactor of make_vehicle_turns

9f5dd69

This corrects the results in the "false_positive_u_turns.txt" golden file.

removes old code

aa5f9dd

applies cargo fmt

4b868da

updates goldenfile format

bac2fc0

moves get_ban_turn_info to a method on Map

6b807f6

simplifies test return type

86a43c6

andrewphilipsmith force-pushed the ltn_turn_restrictions branch from 8f1c362 to 86a43c6 Compare June 28, 2023 17:11

andrewphilipsmith added 2 commits June 28, 2023 19:54

tidies up leftover comments etc

b22fc40

move turn_type_from_road_geom() out of make

b46da38

andrewphilipsmith commented Jul 5, 2023

View reviewed changes

andrewphilipsmith marked this pull request as ready for review July 5, 2023 14:58

Small PR code style fixes

e6002b3

dabreegster approved these changes Jul 7, 2023

View reviewed changes

dabreegster merged commit ffe9955 into main Jul 7, 2023

dabreegster deleted the ltn_turn_restrictions branch July 7, 2023 10:40

andrewphilipsmith mentioned this pull request Aug 10, 2023

LTN tool, possible improvements to displaying turn-restrictions #1096

Open

3 tasks

Uh oh!

Conversation

andrewphilipsmith commented May 10, 2023

Uh oh!

andrewphilipsmith commented May 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Todo:

Details

Decide on whether to use JOSM icons or not:

OSM tagging is applied to the legal turn, not the prohibited turn:

False positives for U-turn

Uh oh!

dabreegster commented May 25, 2023

Uh oh!

andrewphilipsmith commented Jun 23, 2023

Remaining Todo (or at least make as separate issues):

Uh oh!

dabreegster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dabreegster commented Jun 28, 2023

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andrewphilipsmith commented Jul 5, 2023

Uh oh!

dabreegster left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

andrewphilipsmith commented May 25, 2023 •

edited

Loading