New speed limit layers #169

dabreegster · 2023-05-16T15:18:22Z

For #69. To try this out: https://acteng.github.io/atip/speed_limit/scheme.html?authority=West%20of%20England%20Combined%20Authority

The new features

After drawing a route, you can now add a layer showing the current speed limits along that route, according to OSM:

screencast.mp4

And you can also color all roads with known data:

screencast.mp4

Data quality

This is maxspeed from OSM. As you can see, it's often not tagged at all, especially on smaller streets:

I also spot suspicious differences between two sides of the A38 dual carriageway -- 40mph vs 70mph!

The help tooltip warns the user about data quality problems. We can consider integrating with something like https://github.com/westnordost/osm-legal-default-speeds in the future to infer speeds for unknown roads, or to sanity check explicitly tagged speeds.

Design problems

I chose colors and 4 buckets of speeds (in mph) pretty arbitrarily. This is very easy to change, if anyone in ATE has an opinion.

There's a fair bit of clunkiness with the new speed limit layer interacting with other controls. For example, you can still hover and click on any interventions drawn:

screencast.mp4

Should we hide everything else on the map and disable all the toolboxes on the right while a speed limit layer is active? Or is that overkill?

Implementation details

Context for Pete: #154. We load a per-area file containing https://github.com/a-b-street/osm2streets data, load it async in a web worker, and call Rust code through WASM.

Related backend changes are already done: a-b-street/osm2streets#217, acteng/atip-data-prep@2a5dc68

…logic, and vageuly get something started on the frontend

…apper uses

…alue

call it anywhere from the UI yet.

work either for showing everything or for one route. No mutual exclusion between layers yet.

…, instead of trying to glue stuff together. We never make use of that.

dabreegster · 2023-05-18T14:35:56Z

index.html

@@ -11,7 +11,7 @@
  <body>
    <div id="app"></div>
    <script type="module">
-      import ChooseArea from "./src/ChooseArea.svelte";
+      import ChooseArea from "./src/pages/ChooseArea.svelte";


Oh yeah, I forgot to mention in the PR description.. I also renamed/organized a bunch of Svelte components as part of this PR. Apologies; it makes the diff larger. I can split into a separate PR if useful. I just wound up getting annoyed at the too-flat structure in the middle of working on this PR.

dabreegster · 2023-05-18T14:36:15Z

route_info/src/lib.rs

@@ -17,8 +26,9 @@ impl RouteInfo {
    pub fn new(input_bytes: &[u8]) -> Result<RouteInfo, JsValue> {
        // Panics shouldn't happen, but if they do, console.log them.
        console_error_panic_hook::set_once();
+        console_log::init_with_level(log::Level::Info).unwrap();


Any info! and similar logs wind up in browser console

dabreegster · 2023-05-18T14:36:30Z

route_info/src/lib.rs

-        } else {
-            return Err(err_to_js("couldn't get line-string"));
-        };
+        let (pt1, pt2) = self.linestring_endpoints(&feature).map_err(err_to_js)?;


Refactored into map_matching.rs

dabreegster · 2023-05-18T14:36:55Z

route_info/src/lib.rs

+            if let (Waypoint::Snapped(_, i1), Waypoint::Snapped(_, i2)) = (&pair[0], &pair[1]) {
+                if let Some(path) = self.geometric_path(*i1, *i2) {
+                    // Add one LineString per Road where speed_limit is defined.
+                    // We could try to glue together contiguous LineStrings where the speed is the


A previous commit in this PR did things in a much more complicated way. It's preserved in git if we ever need something like it again.

dabreegster · 2023-05-18T14:38:49Z

route_info/src/map_matching.rs

+        Ok(waypoints)
+    }
+
+    /// Calculates a path between intersections, ignoring all semantics of the roads (lane types,


There is a chance that when we draw with route-snapper, then come back and use the recorded waypoints and re-pathfind here, that we wind up with some differences. This is a very trivial form of map matching here. I expect we'll need to improve it over time, or reconsider using two slightly different data sources for the route-snapper and route-info components. But I think it's good enough for now.

And this is because the .bin files from route-snapper could plausibly be created with an older version of OSM data?

Correct. The tension here is that we're using two different files. The route-snapper uses a simple one, and osm2streets uses a richer one with much more structure. The route-snapper one is built from osm2streets and the pathfinding implemented in this PR matches everything, so the results should be the same, if the same input .osm was used to generate both.

In the short term, I think doing this simple matching and generating both files in https://github.com/acteng/abstreet-to-atip/ off the same OSM input will be fine. We might hit some weird edge case.

Longer term, it'd be nice to only load one file, instead of the two. The route snapper could dynamically create its in-memory graph from the osm2streets StreetNetwork. That'd make our lives easier -- less chance of things getting out of sync, less files to generate and manage. The two different files are an artifact of the order things have been developed.

If we hit edge cases, it'll be visibly obvious to the user. They see the route they draw in blue. When they overlay the speed limit, if it doesn't match up with their route, it'll be pretty clear something has gone wrong.

dabreegster · 2023-05-18T14:39:47Z

src/lib/common/HelpIcon.svelte

+  export let contents: string;
+</script>
+
+<img src={icon} title={contents} alt={contents} />


In Firefox, the tooltip takes a second to appear. And maybe the help icon looks clickable and it isn't, and that's an issue? Happy to do something better if there are specific ideas/examples

dabreegster · 2023-05-18T14:40:17Z

src/lib/forms/FormV1.svelte

@@ -68,7 +69,8 @@
  Length: {prettyPrintMeters(length_meters)}
  <br /><button type="button" on:click={() => autoFillDetails()}
    >Auto-fill details</button
-  >
+  ><br />
+  <RouteInfoLayers {routeInfo} {id} />


TODO later: use all of this for v2 as well

dabreegster · 2023-05-18T14:41:10Z

src/lib/layers/ContextualLayers.svelte

@@ -0,0 +1,28 @@
+<script lang="ts">


Making room for more map-wide overlays here! The hard part, IMO, is UX decisions raised in the PR description about disabling other layers/controls while one of these is active.

dabreegster · 2023-05-18T14:41:37Z

src/lib/layers/SpeedLimits.svelte

+  let source = "speed-limits";
+  let layer = "speed-limits";
+  const colorBySpeedLimit: DataDrivenPropertyValueSpecification<string> = [
+    "step",


https://maplibre.org/maplibre-style-spec/expressions/#step for reference

dabreegster · 2023-05-18T14:42:13Z

src/lib/layers/SpeedLimits.svelte

+    speedLimitSteps[2],
+    colors[3],
+  ];
+  // NOTE! There's only ever one source and layer with this name. This component


This is the first time in ATIP we do something like this; all other sources and layers live for the duration of the app. So we need the onDestroy here.

dabreegster

And just recording some older thoughts about how to implement this. I was also thinking about the Rust layer returning segments described by linear referencing. Something like "from 0 to 500m, the speed is 30mph. from 500 to 700m, it's unknown. from 700 to 1km, it's 50mph". Using linear referencing then raises the question of which geometry to use -- the raw linestring from route-snapper? Or the version in the osm2streets StreetNetwork that we reconstruct?

In the end, all of that thinking is complicated. At the end of the day, we want to draw on the map with tooltips, so we need geometry anyway. We can just continue to communicate in GeoJSON and return more things to draw.

dabreegster · 2023-05-18T14:46:57Z

src/lib/layers/SpeedLimits.svelte

+  import DiscreteLegend from "../common/DiscreteLegend.svelte";
+  import HelpIcon from "../common/HelpIcon.svelte";
+
+  export let routeInfo: Remote<RouteInfo>;


Plumbing is getting kind of annoying. It's logically a global; maybe we should use a Svelte store.

Feels like it should be a store? Especially if we're going to draw out more contextual layers like this.

Done! Much cleaner now. We're a little bit unsafe in the places we use it; it might be null, but we still try to call methods on it. All those places are in a try/catch anyway, so it's OK. I left a TODO about detecting/communicating the status of RouteInfo loading

dabreegster · 2023-05-18T14:47:55Z

src/lib/layers/SpeedLimits.svelte

+    ...drawLine(colorBySpeedLimit, lineWidth, 1.0),
+  });
+
+  onMount(async () => {


There's a slight delay between enabling this layer and seeing it appear. It'd be cool to have some kind of explicit loading indicator while routeInfo works. Haven't really figure out how to effectively use https://svelte.dev/tutorial/await-blocks yet

robinlovelace-ate · 2023-05-19T08:14:52Z

This looks amazing Dustin. The fact that you can see all roads is a bonus!

Speed is absolutely critical yet under-considered in network design. In unpublished work I did ages ago 20 mph seemed to be highly correlated with active travel. So makes sense for this to be the first 'data layer'.

One question in terms of UI/UX: this does add complexity. So if we wanted to make a stripped down version for certain use cases, or if it's deemed 'tmi' for next survey, is it easy to turn off? Imagine so given the modular way ATIP is being built but thought I'd ask and assume we will not turn it off as the feature is awesome.

robinlovelace-ate

Approved based on outcome not code.

dabreegster · 2023-05-19T08:20:19Z

is it easy to turn off?

Yes, we can "feature gate" things. An example is disabling most draw controls in planning mode:

atip/src/lib/draw/Toolbox.svelte

Line 145 in fed6519

{#if schema != "planning"}

. Hopefully we can iterate on the UX and make this less confusing before v2 ships. Is there something in the UI that feels particularly complex that we could de-emphasize by default?

robinlovelace-ate · 2023-05-19T08:53:15Z

Is there something in the UI that feels particularly complex that we could de-emphasize by default?

Not right now for me. That question is probably best asked of users in inspections.

robinlovelace-ate · 2023-05-19T09:00:46Z

Just spotted an issue. Easy to fix this before merging @dabreegster ?

#177

dabreegster · 2023-05-19T11:23:06Z

Fixed #177 now:

Sparrow0hawk · 2023-05-19T11:55:08Z

route_info/src/map_matching.rs

+
+        if let Some(geojson::Geometry {
+            value: geojson::Value::LineString(ref pts),
+            ..


I think I understand if let but can you run me through this @dabreegster? And also the ..?

This is how you unpack the pts from a GeoJSON feature I think

We have a geojson::Feature struct. It's got a geometry: Option<Geometry> field. Inside there is a value enum with a bunch of cases. The if let is like Rust's match statement, but for handling just one case. So we're saying "if this GJ feature has geometry that's defined, and the value inside is the LineString enum case". If so, we're grabbing the points in that enum case and calling them pts. ref means to immutably borrow -- we don't need to "move" or "take ownership" of the data in there, just have a read-only pointer to it.

The array lookup stuff below is based on the type that we extracted: https://docs.rs/geojson/latest/geojson/type.LineStringType.html

https://doc.rust-lang.org/rust-by-example/flow_control/match/destructuring/destructure_enum.html is a better reference for match / if let

Gotcha, it was the value enum bit that was catching me out, not something i've done before. Thanks!

Sparrow0hawk · 2023-05-19T12:36:21Z

src/worker.ts

+
+  speedLimitForRoute(waypoints: Waypoint[]): string {
+    if (!this.inner) {
+      throw new Error("Need to loadFile before using RouteInfo");


This error appears as a popup if you try and turn on Speed Limits immediately after clicking a local authority. It is potentially not a useful error message for end users?

Would something like:
Route Tool not yet loaded, please retry or something along those lines be better?

Done. I'd like to maybe surface a second progress bar (but where?) and disable the controls that try to invoke route info until it's ready. Not sure how yet, so a better error message is a good interim

Could we not have a single progress bar for "Route Elements" and it just cover loading of both route-snapper and RouteInfo stuff? I might be oversimplifying here to avoid multiple progress bars

UX-wise that's a little confusing to me, since the snap tool usually finishes loading first (smaller file) and the tool is usable before the route info is ready. I was thinking about putting the progress bar near the two dropdowns that let you enable speed limit, but maybe that's also a bit confusing.

I also want to overhaul how we do progress bars; the one for the route snapper is scary old non-TS code. I started https://github.com/acteng/atip/tree/progress, but could use help polishing / figuring out all the edge cases. Oddly enough, I've had trouble finding a simple library on NPM to do this kind of thing!

Sparrow0hawk

Good to go from me.

On the design questions:

I chose colors and 4 buckets of speeds (in mph) pretty arbitrarily. This is very easy to change, if anyone in ATE has an opinion.

At this stage I'm comfortable with an arbitrary choice and think we should iterate on this based on some user feedback. The one thing we should consider (with all colour based choices) is accessibility and whether the palette we've visible to people with things like red-green colour blindness.

There's a fair bit of clunkiness with the new speed limit layer interacting with other controls.

On the interaction between speed-limit layer and other controls, my inclination to to reduce the number of available controls you can use, maybe entirely, although the current smoothness of switching out of speed limit view (by finishing drawing an intervention) is maybe nicer than clicking back to the dropdown.

What is the use of the layer here to the user? Is it to assist someone drawing an intervention along a road based on speed (if so do we make the speed limits more transparent when in draw route mode?)? Or are we providing it as an extra layer that surfaces some data that someone wants to check about an existing plotted intervention (in which case maybe disable everything). To answer that we need to go back a bit to what the users want this for.

dabreegster · 2023-05-19T12:59:44Z

Thanks for the review! I'll merge this now, though async feedback from Pete is still welcome. This is very much just a first cut still.

At this stage I'm comfortable with an arbitrary choice and think we should iterate on this based on some user feedback

SGTM! Already updated these arbitrary choices thanks to Robin's feedback (#177 for context). Agreed about color schemes; we need to revisit these generally.

Is it to assist someone drawing an intervention along a road based on speed (if so do we make the speed limits more transparent when in draw route mode?)? Or are we providing it as an extra layer that surfaces some data that someone wants to check about an existing plotted intervention (in which case maybe disable everything).

Good question... I don't know. The one case I'm imagining is to understand context near an intervention. Speed along a route is one thing, but if some of the cross streets to the route are high-speed, maybe that matters. I could see a use for editing geometry while this layer is on, in which case we do need to adjust opacity and z-order.

For now I'll leave things as they are, and we can iterate as we get feedback / design advice.

dabreegster force-pushed the speed_limit branch from ec8f871 to a3263cd Compare May 18, 2023 12:36

dabreegster added 11 commits May 18, 2023 14:14

Start prototyping speed limit info for a route. Add the backend Rust …

cfe8bee

…logic, and vageuly get something started on the frontend

Get something displayed from Rust!

35e4352

Display debug tooltips for each chunk of 'speed limit'

260e191

Move map matching strategies into their own module

c77ea03

Use the same lane-oblivious pathfinding to match routes that route-sn…

df3d2b5

…apper uses

Actually use speed limit from osm2streets now, not some dummy other v…

de8e60a

…alue

Use a real color scheme and add a legend

baba714

Clean up the speed limit layer when closing a route

ec06fb3

Add a disclaimer about speed limits

384daea

Organize directory structure

c4af936

Also add the ability to show all known speed limits everywhere. Don't

c920445

call it anywhere from the UI yet.

dabreegster force-pushed the speed_limit branch from 0c59dba to c920445 Compare May 18, 2023 13:15

dabreegster added 2 commits May 18, 2023 15:01

Add controls to show all speed limits. Refactor the speed limit code to

bba5425

work either for showing everything or for one route. No mutual exclusion between layers yet.

Disable map-wide layers when any form is open

a4dccf9

dabreegster force-pushed the speed_limit branch from 5b2ca88 to a4dccf9 Compare May 18, 2023 14:10

dabreegster added 2 commits May 18, 2023 15:13

Cleanup popups carefully

6313679

Simplify the Rust code by just returning one feature per Road segment…

c03338b

…, instead of trying to glue stuff together. We never make use of that.

dabreegster changed the title ~~Show speed limit along a route~~ New speed limit layers May 18, 2023

dabreegster marked this pull request as ready for review May 18, 2023 14:33

dabreegster requested review from Sparrow0hawk, robinlovelace-ate and Pete-Y-CS May 18, 2023 14:34

dabreegster commented May 18, 2023

View reviewed changes

robinlovelace-ate approved these changes May 19, 2023

View reviewed changes

robinlovelace-ate mentioned this pull request May 19, 2023

Add 20 mph roads to speed limit legend #177

Closed

6 tasks

dabreegster added 2 commits May 19, 2023 10:53

Don't show speeds for railways when showing all

e9e495a

Use specific speed limit values. #177

58fee16

Fix typechecking for DiscreteLegend

fdfc48d

Sparrow0hawk reviewed May 19, 2023

View reviewed changes

dabreegster added 2 commits May 19, 2023 13:38

Improve user-visible message in web worker

2769bfe

Plumb RouteInfo through a store, since it's global

6af5edf

Sparrow0hawk approved these changes May 19, 2023

View reviewed changes

dabreegster mentioned this pull request May 19, 2023

Improve progress bars #181

Closed

3 tasks

dabreegster merged commit 8d3e463 into main May 19, 2023

dabreegster deleted the speed_limit branch May 19, 2023 13:07

dabreegster mentioned this pull request Jun 21, 2023

Infer missing speed limits #243

Closed

New speed limit layers #169

New speed limit layers #169

Conversation

dabreegster commented May 16, 2023 • edited Loading

The new features

Data quality

Design problems

Implementation details

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

robinlovelace-ate commented May 19, 2023

robinlovelace-ate left a comment

Choose a reason for hiding this comment

dabreegster commented May 19, 2023

robinlovelace-ate commented May 19, 2023

robinlovelace-ate commented May 19, 2023

dabreegster commented May 19, 2023

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

Sparrow0hawk left a comment

Choose a reason for hiding this comment

dabreegster commented May 19, 2023

dabreegster commented May 16, 2023 •

edited

Loading