NAVAND-1073: fix alternative routes refresh #6848

dzinad · 2023-01-18T17:34:20Z

No description provided.

github-actions · 2023-01-18T17:34:48Z

Changelog

Features

Bug fixes and improvements

Fixed an issue where alternative routes might have had an incorrect or incomplete portion of the route refreshed or occasionally fail to refresh. #dd
Improved NavigationView camera behavior to go back into overview state if routes change during route preview state. #6840

Known issues ⚠️

Other changes

Android Auto Changelog

Features

Bug fixes and improvements

VysotskiVadim · 2023-01-19T10:48:31Z

...rc/androidTest/java/com/mapbox/navigation/instrumentation_tests/utils/coroutines/Adapters.kt

+suspend fun MapboxNavigation.requestAlternativeRoutesAsync() = suspendCoroutine<Expected<RouteAlternativesError, List<NavigationRoute>>> { continuation ->
+    requestAlternativeRoutes(object : NavigationRouteAlternativesRequestCallback {
+        override fun onRouteAlternativeRequestFinished(
+            routeProgress: RouteProgress,
+            alternatives: List<NavigationRoute>,
+            routerOrigin: RouterOrigin
+        ) {
+            continuation.resume(ExpectedFactory.createValue(alternatives))
+        }
+
+        override fun onRouteAlternativesRequestError(error: RouteAlternativesError) {
+            continuation.resume(ExpectedFactory.createError(error))
+        }
+    })
+}
+


does somebody use it?

No, thanks. I was just testing something.

VysotskiVadim · 2023-01-19T10:50:08Z

...ts/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshTest.kt

+        mockWebServerRule.requestHandlers.add(
+            FailByRequestMockRequestHandler(
+                MockDirectionsRefreshHandler(
+                    "route_response_alternative_for_multileg",
+                    readRawFileText(
+                        activity,
+                        R.raw.route_response_alternative_for_multileg_refreshed
+                    ),
+                    acceptedGeometryIndex = 11
+                )
+            )
+        )


why do you add the header in the middle of the test? Is that because the last header you add takes over all route refresh requests?

Not really. It just doesn't have any effect until we add an alternative. Moved it to the beginning of the test.

VysotskiVadim · 2023-01-19T11:13:35Z

...in/java/com/mapbox/navigation/core/routealternatives/AlternativeRouteProgressDataProvider.kt

+        val legGeometryIndex: Int?
+        val primaryFork = alternativeMetadata.forkIntersectionOfPrimaryRoute
+        val alternativeFork = alternativeMetadata.forkIntersectionOfAlternativeRoute
+        if (primaryRouteProgressData.routeGeometryIndex < primaryFork.geometryIndexInRoute) {


do I understand it correctly?

Suggested change

if (primaryRouteProgressData.routeGeometryIndex < primaryFork.geometryIndexInRoute) {

val isForkPointAheadOnPrimaryRoute = primaryRouteProgressData.routeGeometryIndex < primaryFork.geometryIndexInRoute

if (isForkPointAheadOnPrimaryRoute) {

it's up to you to apply suggestion or not

Yes. I'll introduce a val.

VysotskiVadim · 2023-01-19T11:16:17Z

...in/java/com/mapbox/navigation/core/routealternatives/AlternativeRouteProgressDataProvider.kt

+        } else {
+            legIndex = alternativeFork.legIndex
+            routeGeometryIndex = alternativeFork.geometryIndexInRoute
+            legGeometryIndex = alternativeFork.geometryIndexInLeg
+        }


does it mean, that we have passed alternative, but didn't remove it from the tracking routes, we will be refreshing it starting from fork point?

VysotskiVadim · 2023-01-19T11:28:36Z

...in/java/com/mapbox/navigation/core/routealternatives/AlternativeRouteProgressDataProvider.kt

+        if (isForkPointAheadOnPrimaryRoute) {
+            val legIndexDiff = primaryFork.legIndex - alternativeFork.legIndex
+            legIndex = primaryRouteProgressData.legIndex - legIndexDiff
+            val routeGeometryIndexDiff =


As I know, even if 2 routes follow the same roads, they could have a different points in their geometry. I remember that Directions API inserts additional points if traffic between two points varies. I don't know which range of error in index we should expect and if it's critical. Maybe it is, maybe it's not. Anyway current solution makes things better, so maybe this suggestion is more for a followup PR.

I can propose following alternatives which shouldn't be affected by the difference in geometries:

Make the NN track current alternative geometry index.

Track it by ourselves: find nearest point in geometry to the current map matched position. As optimisation we can start search from routeGeometryIndexDiff as it should be very close.

@mapbox/navnative , any suggestions from your side?

Make the NN track current alternative geometry index.

That would be perfect, of course. :)

Track it by ourselves: find nearest point in geometry to the current map matched position. As optimisation we can start search from routeGeometryIndexDiff as it should be very close.

I'm not sure it's a very good idea to implement this platform-side.

As I know, even if 2 routes follow the same roads, they could have a different points in their geometry. I remember that Directions API inserts additional points if traffic between two points varies. I don't know which range of error in index we should expect and if it's critical. Maybe it is, maybe it's not. Anyway current solution makes things better, so maybe this suggestion is more for a followup PR.

Where would the fork be in this case? If the routes follow the same roads but the geometries are different? @mskurydin do you know?

I'm not sure about routes that have actually different geometry, but there can be routes that have different number of geometry points.

For example, if there's an incident that appeared later on in the trip, the alternative route might have it while the primary route won't. So even though the actual geometry doesn't change, the introduction of an incident will make the alternative route have one more point than the primary route (the point where incident happened). This is a universal risk that we're carrying and a major problem with the refresh feature in itself.

I think that's just the risk will have to bear with for now (and make sure that the code doesn't crash in these situations).

the introduction of an incident will make the alternative route have one more point than the primary route

As far as I remember we Directions API also adds points for traffic when it needs more granularity, doesn't it?

Yes, I believe so.

VysotskiVadim · 2023-01-19T11:35:42Z

libnavigation-core/src/main/java/com/mapbox/navigation/core/RoutesProgressDataProvider.kt

+internal data class RoutesProgressData(
+    val primaryRoute: NavigationRoute,
+    val primaryRouteProgressData: RouteProgressData,
+    val alternativeRoutesProgressData: List<Pair<NavigationRoute, RouteProgressData?>>


Not important suggestion, just a matter of taste

Suggested change

val alternativeRoutesProgressData: List<Pair<NavigationRoute, RouteProgressData?>>

val alternativeRoutesProgressData: Map<NavigationRoute, RouteProgressData>

I want to keep the order.

...igation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt

libnavigation-core/src/test/java/com/mapbox/navigation/core/RoutesProgressDataProviderTest.kt

codecov · 2023-01-19T12:07:43Z

Codecov Report

Merging #6848 (307c91d) into main (1098030) will increase coverage by 0.02%.
The diff coverage is 87.50%.

❗ Current head 307c91d differs from pull request most recent head 60a89c1. Consider uploading reports for the commit 60a89c1 to get more accurate results

@@             Coverage Diff              @@
##               main    #6848      +/-   ##
============================================
+ Coverage     72.64%   72.67%   +0.02%     
- Complexity     5564     5575      +11     
============================================
  Files           781      783       +2     
  Lines         30116    30174      +58     
  Branches       3561     3570       +9     
============================================
+ Hits          21878    21929      +51     
- Misses         6810     6814       +4     
- Partials       1428     1431       +3

Impacted Files	Coverage Δ
...box/navigation/core/NavigationComponentProvider.kt	`45.23% <0.00%> (ø)`
...e/routealternatives/RouteAlternativesController.kt	`75.37% <ø> (ø)`
...ore/routerefresh/RouteRefreshControllerProvider.kt	`0.00% <0.00%> (ø)`
...gation/core/routerefresh/RouteRefreshController.kt	`90.90% <64.70%> (-3.02%)`	⬇️
...ternatives/AlternativeRouteProgressDataProvider.kt	`96.29% <96.29%> (ø)`
...ava/com/mapbox/navigation/core/MapboxNavigation.kt	`71.05% <100.00%> (+0.04%)`	⬆️
...avigation/core/PrimaryRouteProgressDataProvider.kt	`100.00% <100.00%> (ø)`
...pbox/navigation/core/RoutesProgressDataProvider.kt	`100.00% <100.00%> (ø)`

VysotskiVadim

Looks good to me. I don't see any major/blocking problems with this fix.

The only concern I have is how to test it. Current we rely on 1 instrumentation test. Maybe we need to highlight on UI of the test app the part of the route which was refreshed so that testers/early adopters could visually verify if expected part of the route was updated?

changelog/unreleased/bugfixes/6848.md

...ts/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshTest.kt

...rc/androidTest/java/com/mapbox/navigation/instrumentation_tests/utils/coroutines/Adapters.kt

.../androidTest/java/com/mapbox/navigation/instrumentation_tests/utils/routes/RoutesProvider.kt

...ts/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/RouteRefreshTest.kt

...in/java/com/mapbox/navigation/core/routealternatives/AlternativeRouteProgressDataProvider.kt

dzinad · 2023-01-19T12:21:56Z

Maybe we need to highlight on UI of the test app the part of the route which was refreshed so that testers/early adopters could visually verify if expected part of the route was updated?

Idk, it looks to me that the logic for highlighting the refreshed part of the route might not be much simpler than the logic introduced here. And I'm not sure if it can be done without the changes on the SDK side (but I might be wrong).
When I was thinking about how better test it, I decided that the instrumentation tests are the best solution in this case.

...in/java/com/mapbox/navigation/core/routealternatives/AlternativeRouteProgressDataProvider.kt

VysotskiVadim · 2023-01-19T13:42:15Z

Idk, it looks to me that the logic for highlighting the refreshed part of the route might not be much simpler than the logic introduced here. And I'm not sure if it can be done without the changes on the SDK side (but I might be wrong).
When I was thinking about how better test it, I decided that the instrumentation tests are the best solution in this case.

I have a simple idea. Add a button to the test application, which clears annotations that affect visual displaying of congestions. Wait for route refresh and you will be able to see what has been updated by appearing congestions. In this way we will be able to test manually and probably find even more cases similar to this. What do you think?

...in/java/com/mapbox/navigation/core/routealternatives/AlternativeRouteProgressDataProvider.kt

dzinad · 2023-01-20T14:37:11Z

CP: #6857

github-actions bot requested review from RingerJK and SevaZhukov January 18, 2023 17:35

VysotskiVadim reviewed Jan 19, 2023

View reviewed changes

dzinad force-pushed the NAVAND-1073-dd branch 2 times, most recently from 18c1b64 to 4170356 Compare January 19, 2023 10:52

VysotskiVadim reviewed Jan 19, 2023

View reviewed changes

dzinad force-pushed the NAVAND-1073-dd branch 2 times, most recently from ea36cba to b137e7c Compare January 19, 2023 11:23

VysotskiVadim reviewed Jan 19, 2023

View reviewed changes

...igation-core/src/main/java/com/mapbox/navigation/core/routerefresh/RouteRefreshController.kt Outdated Show resolved Hide resolved

VysotskiVadim reviewed Jan 19, 2023

View reviewed changes

libnavigation-core/src/test/java/com/mapbox/navigation/core/RoutesProgressDataProviderTest.kt Outdated Show resolved Hide resolved

VysotskiVadim approved these changes Jan 19, 2023

View reviewed changes

dzinad force-pushed the NAVAND-1073-dd branch from e360078 to 4bedcda Compare January 19, 2023 12:14

LukasPaczos reviewed Jan 19, 2023

View reviewed changes

...in/java/com/mapbox/navigation/core/routealternatives/AlternativeRouteProgressDataProvider.kt Outdated Show resolved Hide resolved

NAVAND-1073: fix alternative routes refresh

80444d9

LukasPaczos added the needs backporting Requires cherry-picking to a currently running release branch label Jan 19, 2023

dzinad force-pushed the NAVAND-1073-dd branch from 1cccc9b to 2058253 Compare January 19, 2023 16:45

LukasPaczos reviewed Jan 19, 2023

View reviewed changes

...in/java/com/mapbox/navigation/core/routealternatives/AlternativeRouteProgressDataProvider.kt Outdated Show resolved Hide resolved

...in/java/com/mapbox/navigation/core/routealternatives/AlternativeRouteProgressDataProvider.kt Outdated Show resolved Hide resolved

NAVAND-1073: fix geometry leg index calculation

44a0db8

dzinad force-pushed the NAVAND-1073-dd branch from a0316e4 to 44a0db8 Compare January 20, 2023 09:25

NAVAND-1073: add example

cbf6d04

dzinad force-pushed the NAVAND-1073-dd branch from b62f509 to cbf6d04 Compare January 20, 2023 11:09

dzinad force-pushed the NAVAND-1073-dd branch 4 times, most recently from 69259ab to e8c177b Compare January 20, 2023 13:38

NAVAND-1073: meet code review

302aa7e

dzinad force-pushed the NAVAND-1073-dd branch from da7ab52 to 302aa7e Compare January 20, 2023 14:03

Rename changelog files

60a89c1

dzinad mentioned this pull request Jan 20, 2023

NAVAND-1073: fix alternative routes refresh #6857

Merged

dzinad merged commit e0c5710 into main Jan 20, 2023

dzinad deleted the NAVAND-1073-dd branch January 20, 2023 14:49

	if (primaryRouteProgressData.routeGeometryIndex < primaryFork.geometryIndexInRoute) {
	val isForkPointAheadOnPrimaryRoute = primaryRouteProgressData.routeGeometryIndex < primaryFork.geometryIndexInRoute
	if (isForkPointAheadOnPrimaryRoute) {

	val alternativeRoutesProgressData: List<Pair<NavigationRoute, RouteProgressData?>>
	val alternativeRoutesProgressData: Map<NavigationRoute, RouteProgressData>

NAVAND-1073: fix alternative routes refresh #6848

NAVAND-1073: fix alternative routes refresh #6848

Uh oh!

Conversation

dzinad commented Jan 18, 2023

Uh oh!

github-actions bot commented Jan 18, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Features

Bug fixes and improvements

Known issues ⚠️

Other changes

Features

Bug fixes and improvements

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

VysotskiVadim Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

codecov bot commented Jan 19, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

VysotskiVadim left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

dzinad commented Jan 19, 2023

Uh oh!

Uh oh!

VysotskiVadim commented Jan 19, 2023

Uh oh!

Uh oh!

Uh oh!

dzinad commented Jan 20, 2023

Uh oh!

Uh oh!

github-actions bot commented Jan 18, 2023 •

edited

Loading

VysotskiVadim Jan 19, 2023 •

edited

Loading

codecov bot commented Jan 19, 2023 •

edited

Loading