Skip calculating step distance when too far away from the step #75
Conversation
|
@Fabi755 would you be interested in reviewing this pull request? |
|
Yes, I will review this PR this week 👍 |
| Feature feature = TurfMisc.nearestPointOnLine(locationToPoint, stepPoints, TurfConstants.UNIT_KILOMETERS); | ||
|
|
||
| // Check distance to route line, if it's too high, it makes no sense to snap and we assume the step distance is the whole distance of the step | ||
| Number distance = feature.getNumberProperty("dist"); |
There was a problem hiding this comment.
It feels a little bit ugly that we read a property here.
On other code parts we use something like this:
double distance = TurfMeasurement.distance(locationToPoint, (Point) feature.geometry());But your solution is calculate the distance only once, while my variant is calculate the distance twice.
I think you can keep this, but if we need this a second time, we should wrap up this in some util function.
There was a problem hiding this comment.
Yes, I know that this is pretty weird, but this is how you are supposed to use Turf AFAIK. Turf.js uses the same approach.
|
The code looks good for me 👍 For my understanding only two questions:
|
Quick explainer: with snapping in this context I mean that your location will be used for internal calculations on the route, not the visual snapping. Yes, this is only relevant when you disable the route recalculation, so it is probably not super relevant for most users of the SDK. You can be 50km away from the route, the SDK always snaps you to the closes point of the route, which makes no sense, if you are very far away from the route. The route progress will be increased and you skip ahead to a future part of the route. If you want to go back on the route at your original location, it can happen that the SDK does not recognises you to be back on the route, as you have skipped ahead already. I hope this makes sense? If not, let me know, I think it can better explained with a drawing :).
I wanted to pick a reasonable distance where it makes sense that we actually consider a route snap. Right now I don't think we will need any higher values, if anything, maybe even lower values. While you are driving towards the route line, you will come closer than 1km to the route, and you will be snapped before you are on the route. |
|
Now I get this! Thanks for your explanation! All things look good to me and we can merge this. |
Thanks, maybe you can approve the PR (using the review option on Github)? |
|
Ahh interesting, I thought this is only possible as maintainer |
This PR improves progress updates if the user is too far from the route. If the user is further away then 1km from the route, we don't progress along the step anymore.
Before this was done, independent of how far the user is away from the route.
This change should only be relevant if you have a fixed route, if you reroute, once the user if offroute, this should not be a relevant change.