Route preview state #6107

VysotskiVadim · 2022-07-28T14:18:51Z

This PR introduces a preview state in MapboxNavigation. MapboxNavigation will be a single source of truth for different clients: mobile app, Drop-In UI, android auto. We wan't charge users for route preview, but if trip session is started, the users will be charged if they are in free drive.

The preview state doesn't mean that you can't implement route preview without it. You still can draw a route using route line without MapboxNavigation. But the route preview state will be the default approach for this as it automatically syncs the state with android auto.

States before this PR

New preview state

Is the PR ready for review?

Yes. It's still requires some polishing(documentation, code style, more tests) but it works. If some aspect doesn't work for you, please report it in comments 🙂

I recommend you checking PreviewRoutesTest and MapboxNavigationActivity to see the preview API in action.

route-preview-state.mp4

TODO in followup PRs:

record set route preview into history file
add telemetry event to measure if it's used
integrate preview with copilot

Next steps after "route preview" feature

The introduced route preview state doesn't cover the case when an app developers want to show route preview during active guidance. They can manually draw a route using route line, but AlternativesMetadata isn't available there. The next logical step I see, is to introduce a data structure which replaces List<NavigationRoute> everywhere(setNavigaitonRoutes, previewNavigationRoutes, etc).

class NavigationRoutes {
   val navigationRoutes: List<NavigationRoutes>
   val primaryRouteIndex: Int
   val primaryRoute: NavigationRoute
   val alternativeRoutes: List<AlternativeRoute>
   internal val routesData: RoutesData
}

class AlternativeRoute {
  val alternativeRoute: NavigationRoute
  val metadata: AlternativeMetadata
}

This data structure will solve the problems this PR doesn't address:

route preview UI can display a list of routes, their order shouldn't change when a user switches between routes
NN expect RoutesData on set route, which will be carried by NavigationRoutes
an app developers will be able to access alternative metadata without MapboxNavigation, which will let preview a route even during active navigation

This next step will need to be discussed. I described it ahead because I assume that you ask me about those unsolved problems.

dzinad · 2022-07-28T14:45:07Z

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

+    fun setPreviewedRoute() {
+        val routes = directionsSession.routes
+        if (routes.isNotEmpty()) {
+            internalSetNavigationRoutes(


How is it gonna affect billing? It receives a route in MapboxNavigation#setNavigationRoutes, not in internalSetNavigationRoutes .

thanks, but billing is not supposed to work in this prototype

dzinad · 2022-07-28T14:48:24Z

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

+        initialLegIndex: Int = 0,
+    ) {
+        //TODO: parse alternatives metadata using NN and set it route alternatives controller
+        directionsSession.setRoutes(routes, initialLegIndex, RoutesExtra.ROUTES_UPDATE_REASON_NEW)


directionsSession notifies RouteObservers. From the docs:

* The route at index 0, if exist, will be treated as the primary route for 'Active Guidance'. * * A list of routes can be modified internally and externally at any time with * [MapboxNavigation.setRoutes], or during automatic reroutes, faster route and route refresh operations.

Wouldn't it be too big a change in the behaviour? Because now it's not only about ActiveGuidance anymore. I'm not sure it would, just wondering.

new state won't break existing behaviour because only new methods can trigger new state

dzinad · 2022-07-28T14:50:16Z

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

+        initialLegIndex: Int = 0,
+    ) {
+        //TODO: parse alternatives metadata using NN and set it route alternatives controller
+        directionsSession.setRoutes(routes, initialLegIndex, RoutesExtra.ROUTES_UPDATE_REASON_NEW)


Which NavigationSessionState are we gonna be in after this invocation? The prev one, right, nothing changes here?

That's a very good question. I would vote for introducing a new state, but it requires deprecation of old states and introducing a new one.

codecov · 2022-07-28T15:00:24Z

Codecov Report

Merging #6107 (77993f8) into main (f0dc41b) will increase coverage by 0.00%.
The diff coverage is 92.30%.

❗ Current head 77993f8 differs from pull request most recent head 25708e0. Consider uploading reports for the commit 25708e0 to get more accurate results

@@            Coverage Diff            @@
##               main    #6107   +/-   ##
=========================================
  Coverage     70.10%   70.10%           
- Complexity     4824     4825    +1     
=========================================
  Files           706      706           
  Lines         27840    27830   -10     
  Branches       3276     3272    -4     
=========================================
- Hits          19518    19511    -7     
+ Misses         7042     7040    -2     
+ Partials       1280     1279    -1

Impacted Files	Coverage Δ
...mapbox/navigation/dropin/camera/CameraComponent.kt	`95.83% <88.88%> (+1.28%)`	⬆️
...i/app/internal/controller/CameraStateController.kt	`90.90% <100.00%> (+2.02%)`	⬆️
...tion/ui/app/internal/navigation/NavigationState.kt	`100.00% <0.00%> (+16.66%)`	⬆️

VysotskiVadim · 2022-09-02T09:23:15Z

Currently, this PR demonstrates a new API and I want to hear your feedback about the new API. The implementation is quite raw for now: it contains many workarounds and doesn't work well in all cases, so, please focus on the API during review. I will polish the implementation after the feedback about API.

@abhishek1508, @kmadsen , can you please check if the preview state solves the problem of sync between Drop-In and car play.

@Guardiola31337 , can you please check new session state, does it break something in copilot?

@LukasPaczos , @RingerJK , @dzinad , I'm requesting your feedback too 🙂

VysotskiVadim · 2022-09-02T10:19:37Z

libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/NavigationSession.kt

    ) : NavigationSessionState()
 }
+
+sealed class NavigationSessionStateV2 {


how to you think, should we replace sealed class by a set of string constants, to be able to add more states in the future?

I tend to think no. I doubt that we will need more states before v3. And for users it should be more comfortable to use sealed class.

dzinad · 2022-09-02T12:54:54Z

Regarding your diagram, I think you should also add a loop from AG to AG via setNavigationRoutes (if you change the routes). If it's just for the reviewers it's OK as-is but if it will be in some kind of documentation, I think we should mention that.

dzinad · 2022-09-02T12:57:51Z

I see that you now don't allow setting routes from "not active" state directly. It's only set route preview, only then set routes. Do you think it's a good idea? That would break the flow for those who don't use route preview, wouldn't it? Or is it just that the diagram is incomplete?

dzinad · 2022-09-02T13:04:03Z

Is setting route preview during Active Guidance, as mentioned by @RingerJK in another PR, not supported yet?
UPD: I see that you mentioned that in the description.

dzinad · 2022-09-02T13:08:29Z

class AlternativeRoute {
val alternativeRoute: NavigationRoute
val primaryRoute: NavigationRoute
val metadata: AlternativeMetadata
}

Why do you need a primary route in the alternative route object?

VysotskiVadim · 2022-09-02T13:11:22Z

I see that you now don't allow setting routes from "not active" state directly. It's only set route preview, only then set routes. Do you think it's a good idea? That would break the flow for those who don't use route preview, wouldn't it? Or is it just that the diagram is incomplete?

Good catch, thanks. I also missed ability to start route preview from active guidance. I will fix this

VysotskiVadim · 2022-09-02T13:40:41Z

I see that you now don't allow setting routes from "not active" state directly. It's only set route preview, only then set routes. Do you think it's a good idea? That would break the flow for those who don't use route preview, wouldn't it? Or is it just that the diagram is incomplete?

Good catch, thanks. I also missed ability to start route preview from active guidance. I will fix this

fixed

VysotskiVadim · 2022-09-02T14:26:07Z

class AlternativeRoute {
val alternativeRoute: NavigationRoute
val primaryRoute: NavigationRoute
val metadata: AlternativeMetadata
}

Why do you need a primary route in the alternative route object?

it maybe doesn't make sense. As I said, this data structure should be discussed in the future. I mentioned it just to cover the problems this PR doesn't intent to solve and my vision of how those problems should be solved later.

dzinad · 2022-09-02T13:13:25Z

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

+        threadController.getMainScopeAndRootJob().scope.launch(Dispatchers.Main.immediate) {
+            routeUpdateMutex.withLock {
+                val alternatives = routes.drop(1)
+                val routesData = RouteParser.createRoutesData(


Didn't you forget to commit RouteParser?

This class comes from NN

dzinad · 2022-09-02T13:19:07Z

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

+                    routes.first().nativeRoute(),
+                    alternatives.map { it.nativeRoute() }
+                )
+                routeAlternativesController.processAlternativesMetadata(


It would clear the cache of metadata that was set previously. But I guess that's OK because in the docs for getAlternativeMetadataFor we have the following:

* If the provided [navigationRoute] is an alternative route in the current session, * this function will return the associated route metadata.

and setting routes preview means the session's changes, right? BTW, what's "session" here?

dzinad · 2022-09-02T13:21:11Z

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

+                    routes,
+                    routesData.alternativeRoutes()
+                )
+                directionsSession.setRoutes(routes, BasicSetRoutesInfo(RoutesExtra.ROUTES_UPDATE_REASON_PREVIEW, initialLegIndex))


That would trigger RoutesObserver. Is that expected? The docs for RoutesObserver#onRoutesChanged say:

The route at index 0, if exist, will be treated as the primary route for 'Active Guidance'.

That's not true anymore.

UPD: I think that's expected. But isn't it a breaking change?

I don't think it's a breaking change. Users don't have to use preview state right after update. But if they start using it, they receive previewed route in the route observer.

OK, makes sense. But at least we should update the docs for RoutesObserver#onRoutesChanged.
BTW, we use MapboxNavigation#internalRoutesObserver to trigger refresh when routes are set. Shouldn't we adopt the logic? I suppose we don't need refresh for preview?

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

dzinad · 2022-09-02T13:58:00Z

...igation-core/src/main/java/com/mapbox/navigation/core/trip/session/NavigationSessionUtils.kt

+        hasRoutes: Boolean,
+        routePreview: Boolean
+    ): NavigationSessionStateV2 = when {
+        hasRoutes && routePreview -> {


What if we're driving here? It should be FreeDrive, shouldn't it?
For V1 it's FreeDrive and I think it makes more sense.

Do I understand you correctly?

mapboxNavigation.startTripSession() mapboxNavigation.setNavigationRoutesForPreview(routes) mapboxNavigation.getNavigationSessionStateV2() // Do you expect get FreeDrive state here?

...s/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/PreviewRoutesTest.kt

dzinad · 2022-09-02T14:18:25Z

...s/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/PreviewRoutesTest.kt

+    }
+
+    @Test
+    fun preview_route_with_alternative() = sdkTest {


Maybe get_alternative_metadata_for_previewed_route?

dzinad · 2022-09-02T14:20:22Z

...s/src/androidTest/java/com/mapbox/navigation/instrumentation_tests/core/PreviewRoutesTest.kt

+    @Test
+    fun preview_route() = sdkTest {
+        val routes = RoutesProvider.dc_very_short(activity).toNavigationRoutes()
+        mapboxNavigation.startTripSession()


I'd also add tests:

where you don't start trip session to check the transition from Idle to RoutePreview.

Where you are in AG and then stop trip session and set routes preview (to check AG -> RoutePreview transition)

dzinad · 2022-09-02T14:23:45Z

examples/src/main/java/com/mapbox/navigation/examples/core/MapboxNavigationActivity.kt

+        binding.navigate.visibility = VISIBLE
+        binding.navigate.setOnClickListener {
+            setRoute()
+            binding.navigate.visibility = INVISIBLE


Why INVISIBLE, not GONE? The same question for xml.

examples/src/main/java/com/mapbox/navigation/examples/core/MapboxNavigationActivity.kt

Guardiola31337 · 2022-09-12T12:55:24Z

can you please check new session state, does it break something in copilot?

Discussed with @VysotskiVadim internally. Capturing the highlights for reference and visibility.

Copilot is going to be a feature of the Nav SDK so they should go hand by hand i.e. if there are breaking changes in Copilot affecting the SDK, we should adapt the code to support them (as with any other dependency). Worth mentioning that Copilot is going to be an internal feature (we’re not going to expose anything of the guts to external users). The key on this topic from Copilot is HistoryRecordingStateChangeObserver - if that observer doesn’t need to change we should be good, although internally that depends on the state so we also need to adapt that part of the code i.e. HistoryRecordingStateHandler which extends from TripSessionStateObserver.

VysotskiVadim · 2022-10-13T12:51:15Z

Guys, I really want to hear some feedback from you about route preview before I polish the code:

@abhishek1508 , does it solve the problem of synchronisation between Drop-In and Android auto?
@Guardiola31337 , how does copilot should react on route preview? Should it be just an event during free driving session?

@mapbox/navigation-android , do you see any cases where the feature doesn't work good?

libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/NavigationSession.kt

Guardiola31337 · 2022-10-17T18:05:36Z

@VysotskiVadim

how does copilot should react on route preview? Should it be just an event during free driving session?

Yeah, I don't think Copilot will be impacted as it only cares about the transitions from Free Drive to Active Guidance and viceversa. So yeah, as long as route preview state doesn't interfere with those we should be good.

VysotskiVadim · 2022-10-19T16:40:00Z

closed in favour to #6495

VysotskiVadim force-pushed the vv-prototype-of-preview-state branch from 20521c0 to 724d720 Compare July 28, 2022 14:20

dzinad reviewed Jul 28, 2022

View reviewed changes

SevaZhukov approved these changes Jul 28, 2022

View reviewed changes

SevaZhukov self-requested a review July 28, 2022 18:09

VysotskiVadim force-pushed the vv-prototype-of-preview-state branch 2 times, most recently from 5c78a48 to 5d4d237 Compare September 1, 2022 09:56

VysotskiVadim requested review from Guardiola31337, LukasPaczos, RingerJK, abhishek1508, dzinad and kmadsen September 2, 2022 09:23

VysotskiVadim changed the title ~~Very raw prototype of possible route preview state for internal discussion~~ Raw prototype of possible route preview state Sep 2, 2022

VysotskiVadim commented Sep 2, 2022

View reviewed changes

VysotskiVadim requested review from SevaZhukov and removed request for SevaZhukov September 2, 2022 10:21

dzinad reviewed Sep 2, 2022

View reviewed changes

VysotskiVadim force-pushed the vv-prototype-of-preview-state branch 2 times, most recently from d8be4f6 to f948d8c Compare October 13, 2022 07:16

prototyping

3c323da

VysotskiVadim force-pushed the vv-prototype-of-preview-state branch from f948d8c to 3c323da Compare October 13, 2022 07:24

VysotskiVadim added 10 commits October 13, 2022 09:50

test switch from route preview to active guidance

cf35d73

fixed route progress in preview state

ea4008d

fixed event during active guidance -> preview switch

16af4e3

fixed fixed route refresh

c6bfad8

fixed subscription in direction session

e087d2f

cleanup tests

1c8e8eb

test cleanup

522537b

tests cleanup

55fc923

tests cleanup

db24a1c

test idle -> preview

7b43599

VysotskiVadim requested a review from dzinad October 13, 2022 12:51

VysotskiVadim changed the title ~~Raw prototype of possible route preview state~~ Route preview state Oct 13, 2022

added some documentation

25708e0

kmadsen reviewed Oct 13, 2022

View reviewed changes

libnavigation-core/src/main/java/com/mapbox/navigation/core/trip/session/NavigationSession.kt Show resolved Hide resolved

VysotskiVadim mentioned this pull request Oct 19, 2022

Route preview iteration 2 #6495

Merged

VysotskiVadim closed this Oct 19, 2022

Route preview state #6107

Route preview state #6107

Uh oh!

Conversation

VysotskiVadim commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

States before this PR

New preview state

Is the PR ready for review?

TODO in followup PRs:

Next steps after "route preview" feature

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!

codecov bot commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

VysotskiVadim commented Sep 2, 2022

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dzinad commented Sep 2, 2022

Uh oh!

dzinad commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dzinad commented Sep 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dzinad commented Sep 2, 2022

Uh oh!

VysotskiVadim commented Sep 2, 2022

Uh oh!

VysotskiVadim commented Sep 2, 2022

Uh oh!

VysotskiVadim commented Sep 2, 2022

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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!

Guardiola31337 commented Sep 12, 2022

Uh oh!

VysotskiVadim commented Oct 13, 2022

Uh oh!

Uh oh!

VysotskiVadim commented Jul 28, 2022 •

edited

Loading

codecov bot commented Jul 28, 2022 •

edited

Loading

dzinad commented Sep 2, 2022 •

edited

Loading

dzinad commented Sep 2, 2022 •

edited

Loading