(feat)(core): Add traffic congestion segments to the main Route #293
Conversation
There was a problem hiding this comment.
Thanks for the PR @engali94!
I think @Archdoog is currently working on the best way(s) to expose annotations up to users (maybe that should be an RFD? 😉). I think that our current ideal scenario is to not make the route struct larger unless a field is likely to be supported by the vast majority of likely implementations. For example, we made the decision to expose voice and banner instructions at the step level because this is a de facto standard now (though it's still optional).
I'm not sure if traffic congestion warrants that level of treatment, but we DO want to get common data models and processing code into the codebase. I think the current idea is to push the whole set of annotations up at the step level (not the full route level, since usually this is more actionable at the step level, but apps could reshape this into a route-long traffic layer after fetching a route, for example, to preserve this across the trip even when steps are dropped).
Annotations at the step level have landed as of #261. #287 was just opened yesterday, and shows the approach that @Archdoog currently has in mind, which is to transform AnyAnnotations in Swift/Kotlin (still TODO). We'll provide the basic types in library code, as well as common annotation formats (ex: Valhalla). The beauty is that if your own server does something different, you just write your own struct (using the shared models, like speed limit and traffic congestion + anything that may be proprietary).
TL;DR, I think we can, with a bit of discussion, get all of the pros without the cons (which you correctly identified in your description :D).
Hopefully that makes sense, thanks for reading this tome, and thanks in advance for your patience as this part of the code is still in flux on our side :)
| fn haversine_distance(coord1: &GeographicCoordinate, coord2: &GeographicCoordinate) -> f64 { | ||
| const EARTH_RADIUS: f64 = 6371000.0; | ||
|
|
||
| let lat1 = coord1.lat.to_radians(); | ||
| let lat2 = coord2.lat.to_radians(); | ||
| let delta_lat = (coord2.lat - coord1.lat).to_radians(); | ||
| let delta_lon = (coord2.lng - coord1.lng).to_radians(); | ||
|
|
||
| let a = | ||
| (delta_lat / 2.0).sin().powi(2) + lat1.cos() * lat2.cos() * (delta_lon / 2.0).sin().powi(2); | ||
| let c = 2.0 * a.sqrt().atan2((1.0 - a).sqrt()); | ||
|
|
||
| EARTH_RADIUS * c | ||
| } |
There was a problem hiding this comment.
I'm pretty hesitant to hand-roll code like this when good library implementations already exist. If you search for haversine in the codebase, you'll find several uses of this over geometry structs (for which we use the geo crate).
There was a problem hiding this comment.
hi @ianthetechie I appreciate you very detailed review, I am very willing to discuss it further
do you have any better/faster mean of communication to discuss this further?
There was a problem hiding this comment.
@engali94 yes! Feel free to reach out on Slack (link in the README); feel free to DM (Ian Wagner).
There was a problem hiding this comment.
@engali94 quick bump here to let you know that I'm available on Slack either via DM or in the #ferrostar channel. Not sure if this works for you time zone-wise, but we also have a regular meeting at 1:00 UTC on Tuesdays (Monday 6pm Pacific / 10am Seoul) to review PRs and discuss direction. Shoot me a message if you'd like to join.
|
|
||
| while end_index < geometry.len() - 1 && remaining_distance > 0.0 { | ||
| let segment_distance = | ||
| haversine_distance(&geometry[end_index], &geometry[end_index + 1]); |
There was a problem hiding this comment.
It's worth noting that, since the approach proposed is parsing annotation data from an OSRM-ish source, that all "OSRM-compatible" APIs I'm aware of include a distance annotation too! We don't currently leverage that, but we very well could try to use that if it's available to save work doing thousands of tiny checks.
(This isn't a "we must do it this way" but a request for discussion btw.)
Sources:
|
I've created the following tickets to assist with wrapping annotations (including congestion up):
@engali94 feel free to comment on those issues and close this PR if they satisfy what you need plus #287 |
|
closing in favor of #287 |
This approach involves:
Pros:
Cons:
This is the implementation of #292