Fixed bugs in closestCoordinate and addArrow functions. by hardsetting · Pull Request #284 · mapbox/mapbox-navigation-ios

hardsetting · 2017-06-15T14:21:02Z

Referring to #283

In closestCoordinate function:
when the closest point to the polyline segment is the projection, now the index is set to the index of the coordinate closest to the intersection.

In addArrow function:
shafts are now generated by concatenating the points before the maneuver in the right order with the points after the maneuver, making sure the closest point to the maneuver is taken once.

EDIT:
I now realize that the changes in closestCoordinate might not be strictly necessary, but it's probably clearer and safer now anyways.

hardsetting · 2017-06-15T14:48:12Z

I have closed the referenced issue, as it emerged from a change in my code.
I'll keep this open in case the changes I made were still useful, but feel free to close.

bsudekum · 2017-06-19T08:44:59Z

@hardsetting thanks for the PR! Could you distill this down to a simple example and maybe link some screenshots to geojson.io?

My first instinct is that adding index + 1 will in fact return the next closest coordinate and not the actual closest coordinate. FWIW, I believe at one point this function did accept an optional look next point? argument.

/cc @1ec5

1ec5

I agree with all the changes proposed here except for the reversal of the first addend in shaftCoordinates and shaftStrokeCoordinates, since the polyline should already be reversed.

1ec5 · 2017-06-20T05:54:43Z

MapboxCoreNavigation/Geometry.swift

        }
        if intersectionDistance != nil && intersectionDistance! < closestCoordinate?.distance ?? CLLocationDistanceMax {
-            closestCoordinate = CoordinateAlongPolyline(coordinate: intersectionPoint!, index: index, distance: intersectionDistance!)
+            closestCoordinate = CoordinateAlongPolyline(coordinate: intersectionPoint!, index: (distances.0 < distances.1 ? index : index+1), distance: intersectionDistance!)


This makes sense, although it does diverge from Turf.js behavior. polyline(along:from:to:) makes use of the index; the only problem would be if both the from and to coordinates passed into polyline(along:from:to:) fell closest to the same vertex on the polyline, in which case the returned array would be empty. But it’s quite an edge case, and I think it would be a problem regardless of this change.

It turns out this change does cause polyline(along:from:to:) (now LineString.sliced(from:to:)) to behave erratically: mapbox/turf-swift#27 (comment).

1ec5 · 2017-06-20T05:59:29Z

MapboxNavigation/MGLMapView.swift

        let shaftLength = max(min(50 * metersPerPoint(atLatitude: maneuverCoordinate!.latitude), 50), 10)
-        let shaftCoordinates = polyline(along: polylineCoordinates!, within: -shaftLength / 2, of: maneuverCoordinate!)
-            + polyline(along: polylineCoordinates!, within: shaftLength, of: maneuverCoordinate!)
+        let shaftCoordinates = Array(polyline(along: polylineCoordinates!, within: -shaftLength / 2, of: maneuverCoordinate!).reversed()


polyline(along:within:of:) already reverses the polyline, so there’s no need to do it here too.

1ec5 · 2017-06-20T06:02:12Z

MapboxNavigation/MGLMapView.swift

-        let shaftCoordinates = polyline(along: polylineCoordinates!, within: -shaftLength / 2, of: maneuverCoordinate!)
-            + polyline(along: polylineCoordinates!, within: shaftLength, of: maneuverCoordinate!)
+        let shaftCoordinates = Array(polyline(along: polylineCoordinates!, within: -shaftLength / 2, of: maneuverCoordinate!).reversed()
+            + polyline(along: polylineCoordinates!, within: shaftLength, of: maneuverCoordinate!).suffix(from: 1))


mapbox/mapbox-gl-native#8808 (which is in the v3.6.0 betas) makes the map SDK more resilient to redundant vertices, but it’s good to address the root cause too. 👍

1ec5 · 2017-06-20T06:18:54Z

MapboxCoreNavigation/Geometry.swift

        }
        if intersectionDistance != nil && intersectionDistance! < closestCoordinate?.distance ?? CLLocationDistanceMax {
-            closestCoordinate = CoordinateAlongPolyline(coordinate: intersectionPoint!, index: index, distance: intersectionDistance!)
+            closestCoordinate = CoordinateAlongPolyline(coordinate: intersectionPoint!, index: (distances.0 < distances.1 ? index : index+1), distance: intersectionDistance!)


My first instinct is that adding index + 1 will in fact return the next closest coordinate and not the actual closest coordinate.

@bsudekum, this change can be seen as completing the work begun in #39, making the index more consistent with the returned coordinate. index initially points to the vertex at the beginning of the segment being examined, so if the coordinate is closer to the end of the segment, the closest vertex would be at index + 1.

FWIW, I believe at one point this function did accept an optional look next point? argument.

You’re probably thinking of includeDistanceToNextCoordinate, which was removed in #9. In addition to conditionally incrementing the index in this third case, the includeDistanceToNextCoordinate argument also updated the intersectionDistance (incorrectly).

bsudekum · 2017-07-05T21:44:47Z

@hardsetting this is close! have a sec to update the PR with @1ec5's feedback?

hardsetting · 2017-07-06T00:28:08Z

I apologize for the delay, have been busy for the last couple weeks.
The suggested changes in closestCoordinate function are probably not that important, just a more linear way of doing the same thing.
When I made the change I mistakenly thought the index = index+1 would make the algorithm skip the next segment (thus leading to some edge case small bugs), as would happen in a C-like programming language, but I now see that's not the case.

Regarding the reversal of first half of the shaft, the polyline(along:within:of:) function indeed reverses the points along the main polyline, so that it starts to add up to distance from the coordinate specified as parameter, but as a result the returned collection of coordinates goes from the "center" of the arrow to the tail.
To my intuition, the first half of the shaft should go from tail to center, and the second half from center to tip.
I might not actually be a problem at all though.

1ec5

Ah, rereading the implementation of shaftCoordinates, I understand why you un-reversed the first half of the shaft. The implementation on master starts at the maneuver point, goes backwards, then skips back to the maneuver point before going forward to the tip of the arrow. The implementation here simply ensures continuity between the two halves of the shaft. 👍

* Begin adding telem events * fix * add timezone * Update * Add remaining events * conditional import * Move back to core * make private * Consolidate telemetry state, outline full telemetry spec * Update date format * Store more state; add device * Updated telemetry event handling * Round events * Add dialog view controller * Reset telemetry session if route updated after arrival * Remove logs; trailing closure syntax * Add userId to feedback event * Hide feedback dialog after 0.5s * Moar trailing closure * Add temporary pod dependency on Mapbox-iOS-SDK * Pass nil feedback description for now * Extend locations collected * Fix location serialization * Reduce locations collected * More battery level precision * remove battery enable flag * Add snapshot to reroute event * Add update + cancel feedback functions * Screenshots as jpeg * Wait until style has loaded before doing things to it (#339) * Fix location permissions and adopt iOS 11 description * Fixed bugs in closestCoordinate and addArrow functions. (#284) * Defer simulated location updates until next run loop (#344) * Did arrive once (#347) * add back * Use telem library * add init * add * add release * fix string * updater * fix * update args * add framework * add certs * move certs * struct * more struct * add * add to tests * add to obj-c * remove * update * bump * Add upcoming step information * Remove telem staging token check (handled upstream) * Add previous step information * Only capitalize first character of step maneuver keys * copy framework * trailing * add framework * add access token * fix * make optional * rename * update event lib * name not version * Better types * Update * No telem in sim * add back * Dont pass in access token * fix * Switch to nested step dictionary in feedback events * Add debug metrics user info key * Fix feedback event types * Moved all event dictionary generation to MMEEventsManager * Move feedback event dict update logic * Bump * Pin to MapboxDirections.swift 0.10.1

Fixed bugs in closestCoordinate and addArrow functions.

83598fe

1ec5 reviewed Jun 20, 2017

View reviewed changes

1ec5 approved these changes Jul 6, 2017

View reviewed changes

bsudekum merged commit c1839ea into mapbox:master Jul 7, 2017

bsudekum pushed a commit that referenced this pull request Jul 7, 2017

Fixed bugs in closestCoordinate and addArrow functions. (#284)

5f7fb85

1ec5 added this to the v0.5.0 milestone Jul 9, 2017

This was referenced Dec 14, 2018

Inconsistent results from Polyline.distance mapbox/turf-swift#27

Closed

Correct index of closest coordinate when rounding up mapbox/turf-swift#74

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Comments

Fixed bugs in closestCoordinate and addArrow functions.#284

Fixed bugs in closestCoordinate and addArrow functions.#284
bsudekum merged 1 commit intomapbox:masterfrom
hardsetting:fix-backward-arrow

hardsetting commented Jun 15, 2017 •

edited

Loading

Uh oh!

hardsetting commented Jun 15, 2017

Uh oh!

bsudekum commented Jun 19, 2017

Uh oh!

1ec5 left a comment

Uh oh!

1ec5 Jun 20, 2017

Uh oh!

1ec5 Dec 15, 2018

Uh oh!

1ec5 Jun 20, 2017

Uh oh!

1ec5 Jun 20, 2017

Uh oh!

1ec5 Jun 20, 2017

Uh oh!

bsudekum commented Jul 5, 2017

Uh oh!

hardsetting commented Jul 6, 2017

Uh oh!

1ec5 left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Comments

Conversation

hardsetting commented Jun 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hardsetting commented Jun 15, 2017

Uh oh!

bsudekum commented Jun 19, 2017

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

1ec5 Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

1ec5 Dec 15, 2018

Choose a reason for hiding this comment

Uh oh!

1ec5 Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

1ec5 Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

1ec5 Jun 20, 2017

Choose a reason for hiding this comment

Uh oh!

bsudekum commented Jul 5, 2017

Uh oh!

hardsetting commented Jul 6, 2017

Uh oh!

1ec5 left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

hardsetting commented Jun 15, 2017 •

edited

Loading