-
Notifications
You must be signed in to change notification settings - Fork 4
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
New speed limit layers #169
Conversation
…logic, and vageuly get something started on the frontend
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.
@@ -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"; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
@@ -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(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any info!
and similar logs wind up in browser console
} else { | ||
return Err(err_to_js("couldn't get line-string")); | ||
}; | ||
let (pt1, pt2) = self.linestring_endpoints(&feature).map_err(err_to_js)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Refactored into map_matching.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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Ok(waypoints) | ||
} | ||
|
||
/// Calculates a path between intersections, ignoring all semantics of the roads (lane types, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this is because the .bin files from route-snapper could plausibly be created with an older version of OSM data?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
export let contents: string; | ||
</script> | ||
|
||
<img src={icon} title={contents} alt={contents} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
src/lib/forms/FormV1.svelte
Outdated
@@ -68,7 +69,8 @@ | |||
Length: {prettyPrintMeters(length_meters)} | |||
<br /><button type="button" on:click={() => autoFillDetails()} | |||
>Auto-fill details</button | |||
> | |||
><br /> | |||
<RouteInfoLayers {routeInfo} {id} /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO later: use all of this for v2 as well
@@ -0,0 +1,28 @@ | |||
<script lang="ts"> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/lib/layers/SpeedLimits.svelte
Outdated
let source = "speed-limits"; | ||
let layer = "speed-limits"; | ||
const colorBySpeedLimit: DataDrivenPropertyValueSpecification<string> = [ | ||
"step", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
speedLimitSteps[2], | ||
colors[3], | ||
]; | ||
// NOTE! There's only ever one source and layer with this name. This component |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
src/lib/layers/SpeedLimits.svelte
Outdated
import DiscreteLegend from "../common/DiscreteLegend.svelte"; | ||
import HelpIcon from "../common/HelpIcon.svelte"; | ||
|
||
export let routeInfo: Remote<RouteInfo>; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Plumbing is getting kind of annoying. It's logically a global; maybe we should use a Svelte store.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Feels like it should be a store? Especially if we're going to draw out more contextual layers like this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
...drawLine(colorBySpeedLimit, lineWidth, 1.0), | ||
}); | ||
|
||
onMount(async () => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approved based on outcome not code.
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
|
Not right now for me. That question is probably best asked of users in inspections. |
Just spotted an issue. Easy to fix this before merging @dabreegster ? |
Fixed #177 now: |
|
||
if let Some(geojson::Geometry { | ||
value: geojson::Value::LineString(ref pts), | ||
.. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think I understand if let
but can you run me through this @dabreegster? And also the ..
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is how you unpack the pts
from a GeoJSON feature I think
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
https://doc.rust-lang.org/rust-by-example/flow_control/match/destructuring/destructure_enum.html is a better reference for match
/ if let
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Gotcha, it was the value enum bit that was catching me out, not something i've done before. Thanks!
src/worker.ts
Outdated
|
||
speedLimitForRoute(waypoints: Waypoint[]): string { | ||
if (!this.inner) { | ||
throw new Error("Need to loadFile before using RouteInfo"); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
SGTM! Already updated these arbitrary choices thanks to Robin's feedback (#177 for context). Agreed about color schemes; we need to revisit these generally.
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. |
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