Fix: waypoint didArrive: is never called if audio instructions are not enabled.

[michaelkirk]

(Note: this was extracted from #58, as requested)

Previously, we would never "arrive" at a waypoint if the route didn't
include spoken instructions. Now we only consider spoken instructions if
there were any present in the first place. I assume this was just a bug, maybe
because people were testing on the mapbox API which provides voice
instructions by default?

Previously, we would "arrive" at the second-to-last step, rather than at
the final step. This bug is more mysterious — I'm not sure how it could
have passed QA. Maybe mapbox provides/provided an additional zero length
step or something at the end of their instructions? I've tested this with my own API and Mapbox's directions API and both seem to perform reasonably now.

Note that "arrival" has some slop in it, see RouteControllerManeuverZoneRadius (default 40 meters).

[michaelkirk]

@boldtrn - To copy our relevant unresolved discussion from #58 (comment)

To be honest, I would prefer if we could actually create another variable like the RouteControllerManeuverZoneRadius, but for leg progress distance.

RouteControllerManeuverZoneRadius is used 5 times in the code base, and it seems to be used pretty consistently to say:

How close do we need to be to a thing to be considered "at" that thing?

TBH, that seems exactly in line with how we're using it here. Why would we want it to be different ? Do you actually have this use case? What value would you choose for it?

Examples pulled from the code:

Are we "at" the beginning of a leg:

let isWithinDepartureStep = distanceToFirstCoordinateOnLeg < RouteControllerManeuverZoneRadius

Are we "at" the intersection:

if absoluteDistanceToIntersection <= RouteControllerManeuverZoneRadius {

Are we at/along the current step?

// If user is close to an intersection, the radius will be lower, so reroutes will fire easier.
let radius = max(reroutingTolerance, RouteControllerManeuverZoneRadius)

Are we "at" the end of a step?

if userSnapToStepDistanceFromManeuver <= RouteControllerManeuverZoneRadius,

Are we not "at" the start of the route?

lastKnownUserAbsoluteDistance > RouteControllerManeuverZoneRadius

[boldtrn]

Maybe mapbox provides/provided an additional zero length
step or something at the end of their instructions?

Yes. This is correct. The last step is the arrival with 0 distance.

So IMHO the previous code was fine, considering it was based on the remaining voice instructions.

One place where you can check this is the Mapbox API Playground.

RouteControllerManeuverZoneRadius is used 5 times in the code base, and it seems to be used pretty consistently to say:
How close do we need to be to a thing to be considered "at" that thing?

Well it is used for Maneuvers in general. Arrival is a maneuver as well, so I get your point. In our app we use vastly different values for the two. For users a generic turn is something different than the arrival at a waypoint.

Probably the current solution is fine for 95% of apps. I think we should allow to make this configurable for that reason.

Could you also look into adding a Changelog entry before we merge this, as this is changing important behaviour of the SDK.

[michaelkirk]

Could you also look into adding a Changelog entry before we merge this, as this is changing important behaviour of the SDK.

Done.

Probably the current solution is fine for 95% of apps. I think we should allow to make this configurable for that reason.

I'm ok with making it configurable, but I don't have this use case and I'm still not really sure I understand your use case. Since it seems pretty separate from the "didArrive: is never called" issue that I'm trying to solve, are you OK to handle your feature of "configurable arrival distance" in a follow up PR?

[boldtrn]

@michaelkirk sorry for the late reply, I did check the code again. I am still wondering about the remaining steps == 0 check and still believe this is wrong (I haven't tested this PR yet). I would have at least expected a check <= 1. For some edge cases <=2 might be even better (or checking the remaining distance 😉).

Possible edge case is a destination is placed just after an intersection. So to reach the destination you still have two steps:

• turn at the intersection
• reach destination

Therefore the arrival can only happens after the two maneuvers are completed.

So I would just like to clarify if you tested this with the Mapbox API or with a compatible API (like GraphHopper Nav Endpoint)? And did you test this in the wild or only simulation? As simulation can lead to different results than actual on the road testing.

[michaelkirk]

Sorry for the slow reply-reply!

I am still wondering about the remaining steps == 0 check and still believe this is wrong

I'm not convinced, but it's OK. I'll leave that part as is for now.

I've further reduced this change to only change that trip completion can happen when the user is not using voice instructions. The code is a little more complicated, but the diff is at least smaller. Hopefully this is less controversial.

I've tested this (and my previous changes) on my own API and the Mapbox API:

arrival.mov.mp4

Please take another look @boldtrn

[michaelkirk]

If there were no voice instructions to begin with, we can never "arrive".

[boldtrn]

Thanks for looking into this. To be honest I liked your previous version better, but this fixes the bug. I still believe the check should be based on duration or distance remaining in combination with steps remaining.

[michaelkirk]

Thanks for the reviews @boldtrn!

I still believe the check should be based on duration or distance remaining in combination with steps remaining.

Since the "completion" of a step is already based on "distance remaining", I'm not sure what change in behavior you could actually achieve there. Like you alluded to earlier, maybe the only change you're talking about would be making that distance configurable? Or maybe I still just don't understand.

In any case, if someone who actually has that use case, and is in a good position to test it, wants to hammer out the details, it could be reasonable to include. I just don't have that use case, so I'm not motivated to hammer out those details.