-
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
Create a new RouteInfo service, and use it to auto-fill the name of a route #154
Conversation
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'm working on generating the map data files everywhere, will followup soon.
Two UX questions immediately:
- Should we have a second progress bar somewhere to show the second file loading?
- Where should this auto-fill button go? It's just in the old v1 form for now. It's only relevant for routes, so maybe also in v2.
.github/workflows/web.yml
Outdated
- name: Build main branch | ||
run: | | ||
npm ci | ||
# TODO Remove the directory check after this is present everywhere |
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.
After merging this PR, I'll rebase all other branches right now, so we can always run the wasm-pack step and simplify things
.github/workflows/web.yml
Outdated
@@ -31,6 +36,8 @@ jobs: | |||
for branch in $(git branch -r | grep -vE "origin/(main|gh-pages)" | sed 's/origin\///'); do | |||
echo "Building $branch" | |||
git checkout $branch | |||
# TODO Handle failure 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.
Same as above. We can just include npm run wasm-release
before the npm run build
below
.gitignore
Outdated
@@ -6,3 +6,5 @@ node_modules/ | |||
/playwright-report/ | |||
/playwright/.cache/ | |||
dist | |||
|
|||
assets/bristol.bin |
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: Remove this. I've manually produced and copied this file over. Working on stuff on the https://github.com/acteng/abstreet-to-atip/ side to generate files everywhere
- `npm run fmt` to auto-format code | ||
- `npm run check` to see TypeScript errors | ||
|
||
If you're using Firefox locally to develop and get "import declarations may |
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.
It works fine in Chrome. See comments in App.svelte
for details
|
||
web_sys::console::log_1(&format!("Got {} bytes, deserializing", map_bytes.len()).into()); | ||
|
||
let map: Map = bincode::deserialize(map_bytes).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.
So after we pass in a binary map file (produced in abstreet-to-atip and soon hosted in S3), we deserialize it. Then we also build a quadtree to snap to an intersection, since all the queries will make use of that
pub fn name_for_route(&self, raw_line_string: JsValue) -> Result<String, JsValue> { | ||
let feature: geojson::Feature = serde_wasm_bindgen::from_value(raw_line_string)?; | ||
// TODO Duplicate non-adjacent points; make a more permissive version of PolyLine. | ||
//let polyline = PolyLine::from_geojson(&feature, Some(self.map.get_gps_bounds())).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.
A/B Street's geometry library has methods to much more easily parse GeoJSON, but they're too strict about duplicate points. We could improve this later to simplify code here.
return Err(err_to_js("couldn't get line-string")); | ||
}; | ||
|
||
// Find the closest intersections to the route endpoints |
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.
The route snapper just gives raw GPS coordinates. Here we do a super simple version of map matching to find the closest intersection in the street network to each of the route's raw endpoints.
.closest_intersection | ||
.closest_pt(pt2, threshold) | ||
.ok_or_else(|| err_to_js("no intersection close to last pt"))?; | ||
let i1_name = self.map.get_i(i1).name(lang, &self.map); |
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.
Then over in A/B Street, there's an existing method to print a nice name for an intersection, based on road names: https://github.com/a-b-street/abstreet/blob/dc74f7de5455b122c5636b34eb0d83b80116a20b/map_model/src/objects/intersection.rs#L229
(This can even use alternate languages, when a street is tagged with multiple names!)
// | ||
// Note this should work fine in older browsers when doing 'npm run build'. | ||
// It's only a problem during local dev mode. | ||
interface WorkerConstructor { |
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 a small amount of setup / TS boilerplate to make Comlink wrap our class
@@ -3,6 +3,10 @@ | |||
import { onMount } from "svelte"; | |||
import authoritiesUrl from "../assets/authorities.geojson?url"; | |||
import type { Schema } from "./types"; | |||
import bristolUrl from "../assets/bristol.bin?url"; | |||
import * as Comlink from "comlink"; | |||
import workerWrapper from "./worker?worker"; |
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.
The ?worker
syntax is Vite build magic... which only works in some browsers when doing npm run dev
. See comments below
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 magic to me ✨
Just glancing at this and will try give it a proper look through tomorrow. Quick question, is there a reason |
No hard limit, but in the short-term, I think it's more convenient to keep it here. The local development workflow is more of a hassle when you have to edit a few local repos in sync, merge PRs in sync, etc. I've hit that annoyance already with route-snapper; https://github.com/dabreegster/route_snapper/blob/main/CHANGES.md is the amusing result. I feel like we're in the exploratory phase right now, and the API of the route info service will change constantly. I expect to write new Svelte components tightly coupled to the data it returns (like to display speed limit broken down along a route). So I'd propose keeping it here for now, and splitting it out into a standalone repo once the dust has settled a bit. |
Regarding your proposals
Agreed. Seeing the routes as part of the network along which people travel makes sense and is the most future proof option. I see it as hard to build and cannot imagine building such functionality, hence had not thought of it, but fully approve of the thinking here if it's actually doable, which I believe is.
Sounds reasonable. I've seen A/B Street in action and must agree: it's rock solid. Questions:
I imagine you've thought of all these already just raising them for conversation. Overall: love the ambition and it make sense to build on what is good and works well. |
If the business logic is written in Rust wouldn't that allow us to get best of both worlds, using the Rust code in the browser via Wasm and in the backend via a CLI like |
No strong feelings. Having 2 progress bars is not common and may look a bit complex but that's just a random thought without having seen it. As a sketch, where would the 2nd progress bar go compared with this?
In the left hand panel just below each question would make sense to me on first glance. |
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'm not qualified to comment on the technical details here. This looks like some hardcore modern TypeScript and Svelte code, combined with a nice bit of tightly integrated high-performance yet concise Rust of good measure. Based on the video and thinking expressed in the PR comments, which greatly helps understand the intention, it's a thumbs-up from me pending a few questions in my previous comments.
Thanks for asking great, critical questions! This is a longer-term decision starting in this PR, so very nice to get some discussion before diving farther in.
Yes. For Alex's benefit, some context: about a year ago, we started splitting https://github.com/a-b-street/osm2streets out from the rest of A/B Street. That's made it easier to spin out smaller projects that just need to understand the street network part of OSM data. It's also a chance to more rapidly iterate on that code without having to immediately keep all A/B Street apps up-to-date. The decision to split has paid off very well (unlike the one here of ATIP and the route snapper). I'm trying to slowly move more and more of A/B Street logic into osm2streets, taking the chance to tackle it with fresh perspective and rewrite -- things like turns, representing edits to the network, pathfinding. I started basing off A/B Street map model, which is a layer after osm2streets, partly based on convenience/rapid prototyping speed, and partly because the map model layer also has buildings snapped to roads. That could be helpful if we want to show amenities along a route. But...
I think this is a strong argument. The osms2streets codebase is comparatively much cleaner, better documented, better tested, and less burdened with older assumptions. Things like zebra and unmarked crossings are a total mess in the map model layer, with US-specific assumptions baked pretty deeply in and crossing data tagged in OSM ignored. We'll likely want to revamp that relatively soon to understand the pedestrian experience. osm2streets is a much better place to do that. And for buildings, there are also some old hacks (like losing any association between an OSM relation of a university campus and the 100 individual buildings in it) that'd be helpful to improve for ATIP. Additionally, storing files with just the osm2streets StreetNetwork will be smaller. Map model files have more stuff crammed in, and we don't need it all. So, I propose:
osm2streets and A/B Street are the same here. Roads and routes are "either" 1D or 2D. Through the API, you can access just 1D road center-lines and glue them together into a longer route. Or you can use the road's estimated width and see more of a 2D perspective. We won't make use of that yet.
+1, also true. I would summarize it like this:
Agreed, and once the tool is loaded, there's not even an immediately actionable thing for the user to do with it. So actually, how about by the autofill button nested in a route's form? If the RouteInfo API isn't ready to use yet, that button can be disabled with a tooltip saying it's loaded, or we just put the progress bar there. Most people won't even see it, unless they open a route before it finishes loading. |
👍 to that. |
Very cool, I didn't realise that. The fact that a multi-lane road can simultaneously be represented as a 1D linestring and as a complex polygon is amazing. Great that we'll benefit from the ability to almost instantly switch between the two representations. Big 👍 |
Sounds reasonable, let's see how it looks... |
models. Add a button to v1 to calculate a name for a route automagically.
workflow, and document it
…though it's not present in every branch yet. This'll cause errors until the other branches have been rebased
Just noting we'll see Github actions failures until this is merged and the other pending branches are rebased. I tried making the wasm-pack step be conditional on whether the |
Since we discussed the high-level approach yesterday, going to merge this now and continue:
Happy for async feedback as always! |
This is a big step towards #69, #137, and all other ideas where we start to understand routes drawn in ATIP not just as geospatial line-strings, but as deeply rich semantic structures. First, the demo. After drawing a route in the v1 mode, you can press a new Autofill button, and generate a name for the route based on street names:
screencast.mp4
Technical design decisions
We want to understand and visualize many things about the route someone draws and things nearby:
I'm proposing two design decisions to do that kind of thing:
Don't attempt to do this analysis in JS or TS, or on top of "raw" OpenStreetMap data. We could try to express some of this as Overpass queries and post-process data coming back. I think that's doable for some of the simpler questions we have, but will quickly break down in complexity.
Use A/B Street's map model as a starter. Years of development and polish have gone into the map importer there, handling various problems in OSM. The map model API has been re-shaped over time to handle traffic simulations, walkshed analyses, low-traffic neighbourhood analysis, etc. I don't think it'll do everything we need in its current form (crossings as they're tagged in OSM are a big gap right now), but I'd push for investing in more improvements there, rather than starting something from scratch.
As an alternative to this approach, we could consider doing all of the route analysis "offline" -- read in the ATIP GeoJSON files, write R or Python scripts to calculate some of the things listed above. This would limit interactivity in ATIP -- maybe someone could later load the results of that and match things up, but the method started in this PR gives instant results. The results of the analysis will require us to develop custom UIs to display things like turn conflicts, and I think the Svelte components are the right place to do that frontend work. We can treat the Rust layer as a backend and keep the two pieces coupled.
Future-proofing
Because initially deserializing the A/B Street map file is slow, running using web workers is immediately desirable, because it happens off the main thread. I found https://github.com/GoogleChromeLabs/comlink, which makes dealing with web workers very easy. This gives us another advantage as well -- the Svelte layer has to deal with async queries upfront. This means we could just as easily make HTTP requests to some new remote API somewhere to answer the same questions. So if we wanted, we could implement the
RouteInfo
service with a Python server running somewhere, and use whatever backend methods/data we want.File management
Edit: acteng/atip-data-prep#7 imports all of the maps for the UK. They're already in S3, aside from 4 areas with bugs.