Add arrival controller for multiple stops #2787

kmadsen · 2020-04-17T04:18:51Z

Description

Resolves: https://github.com/mapbox/navigation-sdks/issues/315

The arrival distances can be wildly incorrect so this may need to be fixed to make this interface work properly #2748

I have added any issue links
I have added all related labels (bug, feature, new API(s), SEMVER, etc.)
I have added the appropriate milestone and project boards

Goal

Provide a callback that notifies arrival for each stop onArrival doesn't get called if navigation is started with multiple stops #1904
Provide an interface to move to next leg
Provide an interface to stay at the current stop

Implementation

Creating an interface with the options to control arrival, as well as moving to the next step.

Testing

Please describe the manual tests that you ran to verify your changes

I have tested locally (including SNAPSHOT upstream dependencies if needed) through testapp/demo app and run all activities to avoid regressions
I have tested via a test drive, or a simulation/mock location app
I have added tests that prove my fix is effective or that my feature works
New and existing unit tests pass locally with my changes

Checklist

My code follows the style guidelines of this project
I have performed a self-review of my own code
I have updated the CHANGELOG including this PR

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

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

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

kmadsen · 2020-04-24T02:27:56Z

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

    }

+    override fun updateRouteLegIndex(routeIndex: Int, legIndex: Int) {
+        navigator.updateLegIndex(routeIndex, legIndex)


I don't think this function is working 🤔 . should we expect it to be working?

@mskurydin

cc: @Guardiola31337

codecov-io · 2020-04-24T03:04:44Z

Codecov Report

Merging #2787 into master will increase coverage by 0.11%.
The diff coverage is 60.86%.

@@             Coverage Diff              @@
##             master    #2787      +/-   ##
============================================
+ Coverage     35.00%   35.11%   +0.11%     
- Complexity     2107     2126      +19     
============================================
  Files           541      543       +2     
  Lines         19422    19488      +66     
  Branches       1841     1851      +10     
============================================
+ Hits           6799     6844      +45     
- Misses        11807    11818      +11     
- Partials        816      826      +10

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

Guardiola31337

Left some comments / questions to discuss before merging here.

Also, now that Core will provide the ArrivalController API we should revisit / refactor

mapbox-navigation-android/libnavigation-ui/src/main/java/com/mapbox/navigation/ui/legacy/RouteUtils.kt

Lines 28 to 50 in 5c1175f

    
               /** 
        
                * Looks at the current [RouteProgressState] and returns if 
        
                * is [RouteProgressState.ROUTE_ARRIVED]. 
        
                * 
        
                * @param routeProgress the current route progress 
        
                * @return true if in arrival state, false if not 
        
                */ 
        
               fun isArrivalEvent(routeProgress: RouteProgress): Boolean = 
        
                   routeProgress.currentState()?.let { currentState -> 
        
                       currentState == RouteProgressState.ROUTE_ARRIVED 
        
                   } ?: false 
        
               /** 
        
                * Uses the [RouteProgress] and compares it with [com.mapbox.navigation.ui.NavigationViewOptions] 
        
                * to find if the device is close enough to final destination 
        
                * 
        
                * @param routeProgress the current route progress 
        
                * @param maxMeters value to compare the arrival remaining distance with 
        
                * @return true if device is close enough, false if not 
        
                */ 
        
               fun deviceCloseEnoughToFinalDestination(routeProgress: RouteProgress, maxMeters: Float?): Boolean = 
        
                       (routeProgress.distanceRemaining() <= maxMeters 
        
                               ?: DEFAULT_MAX_METERS_AWAY_TO_TRIGGER_FINAL_DESTINATION_ARRIVAL)

and the features relying on that from the UI SDK cc @langsmith

Guardiola31337 · 2020-04-27T14:22:59Z

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

        routeRefreshController.start()
+
+        arrivalProgressObserver = ArrivalProgressObserver(tripSession)
+        tripSession.registerRouteProgressObserver(arrivalProgressObserver)


I believe we should unregister arrivalProgressObserver when in MapboxNavigation#onDestroy to prevent leaks.

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

Line 302 in 5c1175f

tripSession.unregisterAllRouteProgressObservers()

I didn't unregister this one because all of them are unregistered onDestroy. Which is interesting, how does the foreground activity continue to work?

Ohh, that's right 👍 Sorry for the confusion!

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

Guardiola31337 · 2020-04-27T14:29:25Z

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

+    }
+
+    fun navigateToNextStop(): Boolean {
+        val routeIndex = tripSession.getRouteProgress()?.route()?.routeIndex()?.toInt()


Watch out this is not the index expected by NN 👀 https://github.com/mapbox/mapbox-java/blob/6c76713f7c2ae19f120fa8a098fa2899be7beeac/services-directions-models/src/main/java/com/mapbox/api/directions/v5/models/DirectionsResponse.java#L211 This is the index of the different routes returned in the DirectionsResponse (alternatives).

does that mean it should just be 0? i'm not actually sure what's right

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

Guardiola31337 · 2020-04-27T15:00:01Z

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

+     * @param legIndex the index of the leg to navigate to
+     */
+    override fun updateRouteLegIndex(routeIndex: Int, legIndex: Int) {
+        navigator.updateLegIndex(routeIndex, legIndex)


See comment above re: routeIndex

See comment above re: misusing MapboxNavigation#navigateToNextStop

Maybe we can rely on the response returned by the Navigator NavigationStatus

/** * Follows a new route and leg of the already loaded directions. * Returns an initialized route state if no errors occurred * otherwise, it returns an invalid route state. * * @param routeIndex new route index * @param legIndex new leg index * * @return an initialized route state as [NavigationStatus] */ override fun updateLegIndex(routeIndex: Int, legIndex: Int): NavigationStatus = navigator.changeRouteLeg(routeIndex, legIndex)

yeah, actually that may help me figure out why it's not working

updated. also found that the navigator status is being updated correctly

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

Guardiola31337 · 2020-04-27T15:08:45Z

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

+     * Stop controlling arrival at stops, detaching will require the caller
+     * to call [navigateToNextStop] manually.


Could we add a quick example in the test app showcasing this use case?

hmm maybe need to add a manual mode to the MultipleStopsActivity?

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

Guardiola31337 · 2020-04-27T15:13:46Z

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

+     *
+     * @return true if navigation to next stop could be started, false otherwise
+     */
+    fun navigateToNextStop(): Boolean {


What happens if this is called before arriving at a stop and multiple times? Are we checking somehow that this API is not misused?

yeah the check for misuse is in the return value. i wasn't going to introduce crashes until this works as expected.

right now, it doesn't seem to be navigating to next stop

Could it be because downstream in NN they have a check 👀 for ROUTE_ARRIVED event so you can only navigateToNextStop safely? If so, we don't need to add any extra check on our side 🚀 Is that the case @mskurydin?

seems like if you call this too early you'll get an "off-route" event..

to the, what if you call it multiple times, your next calls could be dropped. the Boolean return is the best check for misuse right now

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

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

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

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

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

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

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

Guardiola31337 · 2020-04-28T13:50:33Z

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

+     * Once the [RouteProgress.currentState] has reached [RouteProgressState.ROUTE_ARRIVED]
+     * this is called continuously
+     */
+    fun onRouteArrival(routeProgress: RouteProgress) { }


Default interface methods could be problematic for Java users. We could use @JvmDefault but it is only supported since JVM target 1.8 😞 What about removing the default and leaving the AutoArrivalController implementation empty and "force" developers to implement on their side? Another option is having a separate interface for route arrival following the Interface segregation principle. What do you think?

the default implementation follows the Interface segregation principle in the sense that, if you implement the interface you're only forced to implement what you need to. removing the default makes the example have an empty interface.

I see a new interface for an option to remove the default, but we would also be adding another class for tests, another progress observer, and another set of methods to attach/detach. my opinion is it's not worth it.

libnavigator/src/main/java/com/mapbox/navigation/navigator/internal/MapboxNativeNavigator.kt

...avigator/src/main/java/com/mapbox/navigation/navigator/internal/MapboxNativeNavigatorImpl.kt

libnavigator/src/main/java/com/mapbox/navigation/navigator/internal/MapboxNativeNavigator.kt

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

Guardiola31337

Thanks for addressing the feedback @kmadsen 🚀

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

libnavigator/src/main/java/com/mapbox/navigation/navigator/internal/MapboxNativeNavigator.kt

...avigator/src/main/java/com/mapbox/navigation/navigator/internal/MapboxNativeNavigatorImpl.kt

kmadsen commented Apr 17, 2020

View reviewed changes

examples/src/main/java/com/mapbox/navigation/examples/core/MultipleStopsActivity.kt Outdated Show resolved Hide resolved

kmadsen mentioned this pull request Apr 17, 2020

Replay route with history player #2766

Merged

10 tasks

kmadsen force-pushed the km-add-arrival-controller branch from 81bcadb to 891f8f9 Compare April 17, 2020 18:21

kmadsen changed the base branch from km-add-waypoints-to-replay to master April 20, 2020 23:19

kmadsen force-pushed the km-add-arrival-controller branch from 891f8f9 to c4cb481 Compare April 23, 2020 23:21

kmadsen marked this pull request as ready for review April 23, 2020 23:23

kmadsen force-pushed the km-add-arrival-controller branch from a543882 to 24eb730 Compare April 23, 2020 23:37

kmadsen commented Apr 23, 2020

View reviewed changes

libnavigation-core/src/main/java/com/mapbox/navigation/core/stops/ArrivalProgressObserver.kt Outdated Show resolved Hide resolved

kmadsen commented Apr 23, 2020

View reviewed changes

examples/src/main/java/com/mapbox/navigation/examples/core/MultipleStopsActivity.kt Outdated Show resolved Hide resolved

kmadsen force-pushed the km-add-arrival-controller branch 7 times, most recently from d048ed6 to 644f890 Compare April 24, 2020 02:20

kmadsen added this to the v1.0.0 milestone Apr 24, 2020

kmadsen added the feature New feature request. label Apr 24, 2020

kmadsen commented Apr 24, 2020

View reviewed changes

kmadsen force-pushed the km-add-arrival-controller branch 2 times, most recently from bd48013 to 2a0812d Compare April 24, 2020 02:49

JunDai reviewed Apr 27, 2020

View reviewed changes

libnavigation-core/src/main/java/com/mapbox/navigation/core/stops/ArrivalProgressObserver.kt Show resolved Hide resolved

Guardiola31337 reviewed Apr 27, 2020

View reviewed changes

kmadsen force-pushed the km-add-arrival-controller branch 4 times, most recently from c052753 to c678435 Compare April 27, 2020 18:28

Guardiola31337 reviewed Apr 27, 2020

View reviewed changes

libnavigation-core/src/main/java/com/mapbox/navigation/core/stops/ArrivalProgressObserver.kt Outdated Show resolved Hide resolved

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

kmadsen force-pushed the km-add-arrival-controller branch from e2163ac to ab14717 Compare April 27, 2020 22:52

Guardiola31337 reviewed Apr 28, 2020

View reviewed changes

libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt Outdated Show resolved Hide resolved

libnavigation-core/src/main/java/com/mapbox/navigation/core/MapboxNavigation.kt Outdated Show resolved Hide resolved

Guardiola31337 reviewed Apr 28, 2020

View reviewed changes

kmadsen force-pushed the km-add-arrival-controller branch 7 times, most recently from 16d90b9 to 9fb8966 Compare April 28, 2020 15:48

Guardiola31337 approved these changes Apr 28, 2020

View reviewed changes

kmadsen force-pushed the km-add-arrival-controller branch from 9b8fe67 to ae7738d Compare April 28, 2020 19:29

Add arrival controller for waypoints

97a9c8d

kmadsen force-pushed the km-add-arrival-controller branch from ae7738d to 97a9c8d Compare April 28, 2020 19:37

kmadsen merged commit 94dfe84 into master Apr 28, 2020

kmadsen deleted the km-add-arrival-controller branch April 28, 2020 21:00

kmadsen mentioned this pull request Apr 29, 2020

Add ability to observe arrival independently #2855

Merged

10 tasks

Guardiola31337 mentioned this pull request Apr 29, 2020

Clean up MapboxNativeNavigator #2862

Merged

9 tasks

Guardiola31337 mentioned this pull request May 20, 2020

Ship with Java 8 bytecode #3003

Merged

9 tasks

	/**
	* Looks at the current [RouteProgressState] and returns if
	* is [RouteProgressState.ROUTE_ARRIVED].
	*
	* @param routeProgress the current route progress
	* @return true if in arrival state, false if not
	*/
	fun isArrivalEvent(routeProgress: RouteProgress): Boolean =
	routeProgress.currentState()?.let { currentState ->
	currentState == RouteProgressState.ROUTE_ARRIVED
	} ?: false

	/**
	* Uses the [RouteProgress] and compares it with [com.mapbox.navigation.ui.NavigationViewOptions]
	* to find if the device is close enough to final destination
	*
	* @param routeProgress the current route progress
	* @param maxMeters value to compare the arrival remaining distance with
	* @return true if device is close enough, false if not
	*/
	fun deviceCloseEnoughToFinalDestination(routeProgress: RouteProgress, maxMeters: Float?): Boolean =
	(routeProgress.distanceRemaining() <= maxMeters
	?: DEFAULT_MAX_METERS_AWAY_TO_TRIGGER_FINAL_DESTINATION_ARRIVAL)

		* Stop controlling arrival at stops, detaching will require the caller
		* to call [navigateToNextStop] manually.

Add arrival controller for multiple stops #2787

Add arrival controller for multiple stops #2787

Uh oh!

Conversation

kmadsen commented Apr 17, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Goal

Implementation

Testing

Checklist

Uh oh!

Uh oh!

Uh oh!

Uh oh!

kmadsen Apr 24, 2020 • 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!

codecov-io commented Apr 24, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

Uh oh!

Guardiola31337 left a comment

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!

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!

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!

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!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kmadsen commented Apr 17, 2020 •

edited

Loading

kmadsen Apr 24, 2020 •

edited

Loading

codecov-io commented Apr 24, 2020 •

edited

Loading