Expose list of key points leading up to the enhanced location update #2452
Conversation
LukasPaczos
force-pushed
the
lp-location-key-points
branch
2 times, most recently
from
26e9f22
to
390d1f7
Compare
|
Took this for a spin and run into the following native 💥 crash when testing This is pretty reproducible as soon as location updates start coming in (puck moves) and when long press (requesting a route). Maybe because changes brought from Maps
I also run into the following leak 👇
cc @kmadsen @LukasPaczos @mskurydin @SiarheiFedartsou @zugaldia |
That seems similar to mapbox/mapbox-gl-native#16180. |
…82f94024f3763797 to fix mapbox/mapbox-gl-native#16180 reproducible in the navigation sdk mapbox/mapbox-navigation-android#2452
…82f94024f3763797 to fix mapbox/mapbox-gl-native#16180 reproducible in the navigation sdk mapbox/mapbox-navigation-android#2452
|
Can't reproduce the crash in #2452 (comment) anymore with 3ab6921 but after updating to either
It seems a regression introduced when bringing the different NN changes into cc @kmadsen @langsmith @mskurydin @LukasPaczos @andrewychen @Aurora-Boreal |
|
Next steps here as follows: Plan A: NN team to fix issue. |
|
@Guardiola31337 maybe it is because of fix of our previous bug when we messed up longitude/latitude in key points. @LukasPaczos knows the details. He applied workaround for it on your side just by swapping Lon/lat |
|
Double checked what we return in status.location (first point in a row) and status.key_points (the rest in a row). Note, there are some places where there are no key points - these are the statuses when we are in free driving and doing the map-matching - corner interpolation doesn't work for free driving (it needs a different algorithm at the moment). The longtitude and latitude order is the same. And also no aspects of it were changed in the latest snapshot from yesterday. As for travelling back and forth between DC and Antarctica - it's a lon/lat swap. During the very first PoC for corner interpolation our code had a bag where lon/lat were swapped. Lukas pointed this out and we fixed it in our next snapshot. There is a chance though this warkaround was left by accident in the android code. Anyway, the numbers I show are printed in the order of X (lon), Y (lat) [this order is prevailing everywhere on the backend] from the statuses we return to our clients.
{
"type": "FeatureCollection",
"features": [
{
"type": "Feature",
"properties": {},
"geometry": {
"type": "Point",
"coordinates": [
-77.025146484375,
38.950865400919994
]
}
},
{
"type": "Feature",
"properties": {},
"geometry": {
"type": "Point",
"coordinates": [
38.950865400919994,
-77.025146484375
]
}
}
]
} |
|
Follow up addressing above-mentioned issues in #2458 |
|
@Guardiola31337 Is this PR still a draft, or is it ready for review? |
This could be probably ready for review (although I cannot change the In any case, #2458 needs to be merged first and also we're using a cc @LukasPaczos |
…82f94024f3763797 to fix mapbox/mapbox-gl-native#16180 reproducible in the navigation sdk mapbox/mapbox-navigation-android#2452
mapbox/mapbox-gl-native-android#166 has been merged, but let's keep using the |
* make configure router http interface parameter nullable * add nn uncertain route state * expose nn cache last route api in mapbox navigator * remove lat long workaround not needed anymore as nn master-SNAPSHOT-0 includes the fix * bump mapbox-navigation-native version to ms-bearing-from-shape-on-interpolation-SNAPSHOT-2
…gl-native-android#166) which includes fix for mapbox-gl-native mapbox/mapbox-gl-native#16180 crash
* make configure router http interface parameter nullable * add nn uncertain route state * expose nn cache last route api in mapbox navigator * remove lat long workaround not needed anymore as nn master-SNAPSHOT-0 includes the fix * bump mapbox-navigation-native version to ms-bearing-from-shape-on-interpolation-SNAPSHOT-2
LukasPaczos
force-pushed
the
lp-location-key-points
branch
from
0c2d15f
to
69b34f0
Compare
|
All the features and test changes have landed, this is ready for a review. Changes in this PR include:
|
examples/src/main/java/com/mapbox/navigation/examples/activity/SimpleMapboxNavigationKt.kt
Outdated
Show resolved
Hide resolved
Codecov Report
@@ Coverage Diff @@
## master #2452 +/- ##
=========================================
Coverage 32.29% 32.29%
Complexity 621 621
=========================================
Files 143 143
Lines 5605 5605
Branches 434 434
=========================================
Hits 1810 1810
Misses 3595 3595
Partials 200 200 |
.../src/main/java/com/mapbox/services/android/navigation/v5/routeprogress/RouteProgressState.kt
Show resolved
Hide resolved
There was a problem hiding this comment.
Great work @LukasPaczos 🚀
Left some minor comments / questions.
examples/src/main/java/com/mapbox/navigation/examples/activity/SimpleMapboxNavigationKt.kt
Outdated
Show resolved
Hide resolved
...rc/test/java/com/mapbox/services/android/navigation/v5/internal/navigation/HttpClientTest.kt
Outdated
Show resolved
Hide resolved
.../src/main/java/com/mapbox/services/android/navigation/v5/routeprogress/RouteProgressState.kt
Show resolved
Hide resolved
LukasPaczos
force-pushed
the
lp-location-key-points
branch
from
f5243d5
to
7cf24f9
Compare
There was a problem hiding this comment.
I'm still experiencing the leak mentioned in #2452 (comment) 👀
Also unrelated to this PR but note the flashing behavior when switching routes ☝️
Other than that this looks good to me.
Thanks for addressing the feedback @LukasPaczos
|
@LukasPaczos Going to cut a ticket to investigate the leak above-mentioned #2452 (review) and merge here to unblock @kmadsen |
Follow up ticket 👉 #2472 |
Follow up ticket 👉 #2473 |
This PR exposes an additional list of points as part of
LocationObserver#onEnhancedLocationChanged. When available, this list created out of a route-snapped target and intermediate points leading up to it. This way, in UI components we can interpolate the puck's position along the route's geometry.Before:
After:
This is based on changes from the Maps SDK - mapbox/mapbox-gl-native-android#166.
We can merge whenever a new Nav Native snapshot is released (and bumped here) with both
key_pointsanduncertainstate changes.