Remove Location filter and check Location#getAccuracy

[danesfeder]

We are seeing issues with what I think to have tracked down is the velocity check in the LocationValidator. I think it's best to roll back the filtering here in favor of the less aggressive filter we had prior.

Capturing some discussion with @Guardiola31337, this filter is not ideal as it can "build up" to eventually be accepting poor updates. We should continue to look into filtering out erroneous data for situations like urban canyons and such.

TODO:

• Allow setting of accuracy threshold in MapboxNavigationOptions (default 100)

[dsilvera]

I test this update in real life and with simulator. It works fine and solve my problem.
The algorithm is really simpler.

[mclucky]

Hi @danesfeder

We're currently implementing a pilot based on this SDK.
While doing that, we've observed some weird behaviours regarding GPS location.

I personally investigated in the old source before your recent commit, and I've found it well thought, the only line which I doubt was:
LocationValidator.java LINE 120
return accuracyAcceptable || timeSinceLastValidUpdate > locationUpdateTimeInMillisThreshold;

That would mean, as long as there was enough time between updates the current location update would be valid regardless of the value of accuracyAcceptable.

Can you explain why you're doing such a radical change to LocationValidator.java with you recent commit, because all of what was well thought before would be gone.

Thank you,
Lukas

[Guardiola31337]

Are we ok here breaking SemVer?
If so, we should document this in the release notes.

[Guardiola31337]

Are we ok here breaking SemVer?
If so, we should document this in the release notes.

[danesfeder]

@mclucky we were noticing some problems with this logic and opted to remove it.

The first problem was the "percent threshold" rule for accuracy. This logic could potentially allow the Location accuracy to be X percent worse and let the acceptable accuracy grow to an actually unacceptable value:
https://github.com/mapbox/mapbox-navigation-android/pull/1157/files#diff-d7e02b2b6e966516d978dc02e8d89be6L85

The second problem was using the lastValidLocation for the velocity check. For example, if you went through a tunnel, your last valid location would be at the beginning of the tunnel. Once out of the tunnel, your velocity would continually be invalid because the distance from the last valid location to the current location would always be too far for a valid velocity:
https://github.com/mapbox/mapbox-navigation-android/pull/1157/files#diff-d7e02b2b6e966516d978dc02e8d89be6L101

We could potentially "band-aid" these issues, but we are going to remove the logic for now in favor of simple filter - and then will revisit with a more robust implementation. Thanks for your understanding!

[mclucky]

@danesfeder thank you for that in deep explanation, we understand!

[Guardiola31337]

Hey @mclucky 👋
To add to what @danesfeder mentioned there's also another drawback wrt isValidLocation and locationUpdateTimeInMillisThreshold rule https://github.com/mapbox/mapbox-navigation-android/pull/1157/files#diff-d7e02b2b6e966516d978dc02e8d89be6L120

• As reported here Create LocationValidator for checking new location updates #690 (comment)

(numbers are location accuracies):
5, 10, 10, 10, 10, 10, 500, 500, 10, 10, 10
If you get a location every second, you'd skip all of the 10s, but then when 5 second timer expires for isValidLocation(), it will take 500 without regard to its accuracy, and then it will revert back to 10, which from the user point of view will look like two sudden location jumps, first from one with 5 to 500 (probably inaccurate) and then again back to 10.

But the proper thing with this sequence would be to ignore both 500 updates.

[mclucky]

Hi @Guardiola31337

Thank you for further explaining.
I guess what you wrote "... without regard to its accuracy" was what originally catched my eye in the source code.

But however, we'll test your latest changes and we'll give you feedback!!

[Guardiola31337]

Yeah, not a problem anymore because we check against accuracyThreshold 👀

mapbox-navigation-android/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/location/LocationValidator.java

Line 16 in 9b40259

return checkLastValidLocation(location) || location.getAccuracy() < accuracyThreshold;

which developers can tweak in MapboxNavigationOptions#locationAcceptableAccuracyInMetersThreshold

mapbox-navigation-android/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigationOptions.java

Line 95 in 9b40259

public abstract Builder locationAcceptableAccuracyInMetersThreshold(int accuracyInMetersThreshold);

if the default value

mapbox-navigation-android/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/navigation/NavigationConstants.java

Line 197 in 9b40259

static final int ONE_HUNDRED_METER_ACCEPTABLE_ACCURACY_THRESHOLD = 100;

doesn't fit your needs.