Skip to content

Comments

Fix uturn step increment and snapping#642

Merged
bsudekum merged 4 commits intomasterfrom
uturn-fix
Sep 22, 2017
Merged

Fix uturn step increment and snapping#642
bsudekum merged 4 commits intomasterfrom
uturn-fix

Conversation

@bsudekum
Copy link
Contributor

@bsudekum bsudekum commented Sep 21, 2017

Closes: #639

This addresses two issues:

  1. When a step has coordinates that are close to the current step (think in a uturn), we sometimes snap to the wrong step coordinates. To fix this, when the upcoming maneuver modifier is a uturn, we only look at the current step coordinates for snapping
  2. There is a bug in OSRM where exit bearings are calculated incorrectly. This prevents us from incrementing the step counter because the users course never is equal to the exit bearing. This hack should be removed when fixed upstream.

/cc @danpat @1ec5 @frederoni

@1ec5
Copy link
Contributor

1ec5 commented Sep 21, 2017

There is a bug in OSRM where exit bearings are calculated incorrectly. This prevents us from incrementing the step counter because the users course never is equal to the exit bearing. This hack should be removed when fixed upstream.

How urgent is this fix? Can we wait until the fix lands in the Directions API?

/cc @ericrwolfe

@ericrwolfe
Copy link
Contributor

It's been in this long already now. I think we can wait for an upstream fix.

@bsudekum
Copy link
Contributor Author

I think if we can fix this now, we should. Once it's fixed upstream, the code path won't even be hit. It's very harmless code.

@bsudekum
Copy link
Contributor Author

removed hack. OSRM fix should be 1-2 weeks out.

guard let snappedCoordinate = closestCoordinate(on: stepCoordinates, to: location.coordinate) else { return location }

let nearByCoordinates = routeProgress.currentLegProgress.nearbyCoordinates
var nearByCoordinates = routeProgress.currentLegProgress.currentAndUpcomgingStepCoordinates
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe this should remain nearbyCoordinates? Although I cant think of any reason to know about the previous step coords

Copy link
Contributor

Choose a reason for hiding this comment

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

At a glance, I can’t think of any reason it would make a difference whether we include the previous coordinates or not.

/**
Returns an array of `CLLocationCoordinate2D` of the current and upcoming step geometry.
*/
public var currentAndUpcomgingStepCoordinates: [CLLocationCoordinate2D] {
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo: upcoming.

public var currentAndUpcomgingStepCoordinates: [CLLocationCoordinate2D] {
let upcomingCoords = upComingStep?.coordinates ?? []
let currentCoords = currentStep.coordinates ?? []
let nearby = currentCoords + upcomingCoords
Copy link
Contributor

Choose a reason for hiding this comment

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

Is a separate method really needed to concatenate these two arrays, given that there’s only one call site?

@bsudekum
Copy link
Contributor Author

@1ec5 fixed.

@bsudekum bsudekum merged commit fc90a59 into master Sep 22, 2017
@bsudekum bsudekum deleted the uturn-fix branch September 22, 2017 21:42
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.

Snap to road on two way streets

3 participants