Skip to content
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

Support incidents for OSRM responses #358

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

ahmedre
Copy link
Contributor

@ahmedre ahmedre commented Nov 13, 2024

This parses incidents on the OSRM response route leg, and splits the
incidents across the corresponding route step. Since an incident may
span multiple steps, this will duplicate an incident and adjust the
indices accordingly whenever that happens. Clients can de-duplicate
these if necessary based on the incident identifier.

@@ -500,4 +535,88 @@ mod tests {
Some(vec!["right".to_string()])
);
}

#[test]
fn deserialize_incidents() {
Copy link
Contributor Author

@ahmedre ahmedre Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i got this example from hitting the api with points of -71.078874,42.376381 to -73.900821,40.659135 using the traffic routing profile.

the documentation has a lot of details about the objects, but no sample unfortunately. i also found some things that came back from their api call that weren't in their documentation, so i intentionally left those out (ex length, affected_road_names_unknown, etc).

@ahmedre ahmedre force-pushed the support_incidents branch 3 times, most recently from 00b5679 to 1520125 Compare November 13, 2024 20:02
@@ -322,6 +322,8 @@ pub struct RouteStep {
pub spoken_instructions: Vec<SpokenInstruction>,
/// A list of json encoded strings representing annotations between each coordinate along the step.
pub annotations: Option<Vec<String>>,
/// A list of incidents that occur along the step.
pub incidents: Vec<Incident>,
Copy link
Contributor Author

@ahmedre ahmedre Nov 13, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking out loud - this is provided via the API as a "per leg" response, not a "per step" response. the indices for the start and stop of the incident in the coordinates is therefore global and not localized per step. does it make sense to move this into the step level and convert and split this so it belongs to each step instead? we would just use the indices as the way to do that - the complication would be if a particular incident "spans" more than one leg, we'd need to actually duplicate it. thoughts?

update - i just realized, that's what i am doing here and i just didn't realize it - but my logic is wrong in common/ferrostar/src/routing_adapters/osrm/mod.rs, since i need to also slice these just like we're doing with annotations. will mark this as a draft while i fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, but please see the discussion below for some trade-offs.

@@ -126,6 +133,7 @@ impl Route {
step,
step_geometry,
annotation_slice,
incident_items.clone(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i need to fix this - this shouldn't be all the items, just the items relevant to this step.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

fixed, but please see the discussion below for some trade-offs.

@ahmedre ahmedre marked this pull request as draft November 13, 2024 20:45
This parses incidents on the OSRM response route leg, and splits the
incidents across the corresponding route step. Since an incident may
span multiple steps, this will duplicate an incident and adjust the
indices accordingly whenever that happens. Clients can de-duplicate
these if necessary based on the incident identifier.
@ahmedre
Copy link
Contributor Author

ahmedre commented Nov 14, 2024

@ianthetechie would appreciate your input here, since we have a few possible solutions that we were discussing internally, each with their set of pros and cons (we chose one of the solutions for this PR) - but want to know what you'd recommend.

The Problem

Mapbox's SDK emits incidents on the leg instead of on the step. While this is similar to the case of how annotations are handled, there are differences. For example, unlike annotations, where geometry.size == annotations.size for the overall route, the incidents array is not one to one with either since it's really a set of events that occur on this path. Consequently, a given incident may (theoretically anyway, the documentation is sparse about this) span multiple steps. This brings about the complication.

Possible Solutions

Incidents on Legs

Keep incidents on the leg instead of on the step (i.e. on TripState.Navigating in this case). Since the steps are dynamic but the global routeGeometry is not going to change, this means we'd need to hydrate information that helps people using the api to map a given incident to the proper relevant RouteSteps (i.e. if we just say starts at routeGeometry index 3000 and ends at routeGeometry index 3050, I have no idea which RouteStep that corresponds to - has it passed? Is it up ahead?) We'd need something to say "the current RouteStep is at index x of the overall routeGeometry for such calculations to be feasible.

  • Pros - it's modeled like the api models it.
  • Cons - we'd need to expose currentStepRouteGeometryStartIndex, which is kind of unfortunate. It is also cumbersome for customers to make use of this data, since they would still need to do calculations from there to figure out which RouteSteps this applies to.

Incidents on Routes

Keep incidents on the RouteStep depending on the corresponding geometry_index_start (and, if present geometry_index_end). This is the approach taken by this patch. If a particular Incident spans multiple RouteSteps, this will duplicate the Incident, and adjust the indices accordingly. For example, if an Incident is starts in step 3 at index x and ends in step 4 at index y, this would result in 2 copies of the Incident - one in step 3 between the mapped x to the end of step 3's indices, and one in step 4 between 0 and the mapped end. If it had spanned another step, there would be an additional copy spanning the entire middle step.

  • Pros - it's in line with what's there today, and is easier for clients to work with.
  • Cons - duplicating Incidents means clients would need to de-dup them if necessary to avoid re-notifying, etc.

@ahmedre
Copy link
Contributor Author

ahmedre commented Nov 14, 2024

also, i am not sure why ios CI fails with:

fatal: invalid reference: support_incidents

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant