Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog/unreleased/bugfixes/6841.md
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
- Removed `NavigationRoute#hasUnexpectedClosures` and added `RouteProgress#hasUnexpectedUpcomingClosures` instead that checks whether route has upcoming unexpected closures (the algorithm does not take into account closures that the puck has already been on)
2 changes: 1 addition & 1 deletion libnavigation-base/api/current.txt
Original file line number Diff line number Diff line change
Expand Up @@ -1745,7 +1745,7 @@ package com.mapbox.navigation.base.utils {
package com.mapbox.navigation.base.utils.route {

public final class NavigationRouteUtils {
method @com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI public static suspend Object? hasUnexpectedClosures(com.mapbox.navigation.base.route.NavigationRoute, kotlin.coroutines.Continuation<? super java.lang.Boolean>);
method @com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI public static suspend Object? hasUnexpectedUpcomingClosures(com.mapbox.navigation.base.trip.model.RouteProgress, kotlin.coroutines.Continuation<? super java.lang.Boolean>);
}

}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -8,35 +8,57 @@ import com.mapbox.api.directions.v5.models.RouteOptions
import com.mapbox.geojson.Point
import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
import com.mapbox.navigation.base.route.NavigationRoute
import com.mapbox.navigation.base.trip.model.RouteProgress
import com.mapbox.navigation.base.utils.DecodeUtils.stepGeometryToPoints
import com.mapbox.navigation.utils.internal.ifNonNull
import com.mapbox.navigation.utils.internal.logD
import kotlinx.coroutines.Dispatchers
import kotlinx.coroutines.withContext

private const val LOG_CATEGORY = "NavigationRouteUtils"

/**
* This function checks whether the [NavigationRoute] has unexpected closures, which could be a reason to re-route.
* This function checks whether the [NavigationRoute] has unexpected upcoming closures, which could be a reason to re-route.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what is the difference between unexpected and expected closure? Is the unexpected the one we receive after route refresh?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the difference is in snapping options (see com.mapbox.api.directions.v5.models.RouteOptions).

Whenever snapping is not allowed, that means the closure is unexpected

*
* `true` is returned whenever there's any [RouteLeg.closures] outside of a direct region around a coordinate from [RouteOptions.coordinatesList]
* The algorithm does not take to account current closures, where the puck is on.
*
* `true` is returned whenever there's any upcoming [RouteLeg.closures] outside of a direct region around a coordinate from [RouteOptions.coordinatesList]
* that was allowed for snapping to closures with [RouteOptions.snappingIncludeClosuresList] or [RouteOptions.snappingIncludeStaticClosuresList].
* flags. Otherwise, `false` is returned.
*/
@ExperimentalPreviewMapboxNavigationAPI
suspend fun NavigationRoute.hasUnexpectedClosures(): Boolean =
suspend fun RouteProgress.hasUnexpectedUpcomingClosures(): Boolean =
withContext(Dispatchers.Default) {
val snappingResultList = directionsRoute.getSnappingResultList()
val snappingResultList = navigationRoute.directionsRoute.getSnappingResultList()

val routeProgressData = ifNonNull(
currentLegProgress,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Question which is outside of the scope of this PR.
Hm, interesting why current leg progress is nullable. As I see we always create it, don't we?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suppose it's in the scope actually. I had the same doubts, found that RouteLegProgress and RouteStepProgress are not null, so we should be good.

) { legProgress ->
RouteProgressData(legProgress.legIndex, legProgress.geometryIndex)
}

var currentLegFirstWaypointIndex = 0
directionsRoute.legs()?.forEachIndexed { routeLegIndex, routeLeg ->
navigationRoute.directionsRoute.legs()?.forEachIndexed { routeLegIndex, routeLeg ->

val legLastGeometryIndex by lazy {
directionsRoute.stepsGeometryToPoints(routeLeg).lastIndex
navigationRoute.directionsRoute.stepsGeometryToPoints(routeLeg).lastIndex
}

val silentWaypoints = routeLeg.silentWaypoints()

routeLeg.closures()?.forEach { closure ->
if (routeProgressData != null) {
if (routeProgressData.currentLegIndex > routeLegIndex) {
// skipping passed legs
return@forEach
} else if (
routeProgressData.currentLegIndex == routeLegIndex &&
routeProgressData.currentGeometryLegIndex >= closure.geometryIndexStart()
) {
// skipping current and passed closures on the current leg
return@forEach
}
}
val silentWaypointsInClosureRange = silentWaypoints.inGeometryRange(
closure.geometryIndexStart(),
closure.geometryIndexEnd()
Expand All @@ -49,9 +71,10 @@ suspend fun NavigationRoute.hasUnexpectedClosures(): Boolean =
snappingResultList.getOrNull(silentWaypointIndex) ?: false
if (!isSnapAllowed) {
logD(
"Route with id [${[email protected]}] has closure " +
"at leg index $routeLegIndex, that overlaps silent (via) " +
"waypoint",
"Route with id " +
"[${[email protected]}] " +
"has closure at leg index $routeLegIndex, that overlaps " +
"silent (via) waypoint",
LOG_CATEGORY
)
return@withContext true
Expand All @@ -63,17 +86,19 @@ suspend fun NavigationRoute.hasUnexpectedClosures(): Boolean =
silentWaypointsInClosureRange.isEmpty()
) {
logD(
"Route with id [${[email protected]}] has closure at leg " +
"index $routeLegIndex",
"Route with id " +
"[${[email protected]}] has " +
"closure at leg index $routeLegIndex",
)
return@withContext true
}
if (closure.geometryIndexStart() == 0 &&
snappingResultList.getOrNull(currentLegFirstWaypointIndex) != true
) {
logD(
"Route with id [${[email protected]}] has closure at the " +
"start of the leg, leg index $routeLegIndex",
"Route with id " +
"[${[email protected]}] has " +
"closure at the start of the leg, leg index $routeLegIndex",
LOG_CATEGORY
)
return@withContext true
Expand All @@ -84,8 +109,9 @@ suspend fun NavigationRoute.hasUnexpectedClosures(): Boolean =
) != true
) {
logD(
"Route with id [${[email protected]}] has closure at " +
"the end of the leg, leg index $routeLegIndex",
"Route with id " +
"[${[email protected]}] has " +
"closure at the end of the leg, leg index $routeLegIndex",
LOG_CATEGORY
)
return@withContext true
Expand All @@ -98,6 +124,11 @@ suspend fun NavigationRoute.hasUnexpectedClosures(): Boolean =
return@withContext false
}

private class RouteProgressData(
val currentLegIndex: Int,
val currentGeometryLegIndex: Int,
)

private fun DirectionsRoute.getSnappingResultList(): List<Boolean> {
val snappingIncludeClosuresList = routeOptions()?.snappingIncludeClosuresList()
val snappingIncludeStaticClosuresList = routeOptions()?.snappingIncludeStaticClosuresList()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -31,7 +31,7 @@ import org.junit.runner.RunWith
import org.junit.runners.Parameterized
import java.net.URL

class NavigationRouteExTest {
class RouteProgressExTest {

@Test
fun `update Navigation route`() {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,12 @@ import com.mapbox.api.directions.v5.DirectionsCriteria
import com.mapbox.api.directions.v5.models.DirectionsRoute
import com.mapbox.navigation.base.ExperimentalPreviewMapboxNavigationAPI
import com.mapbox.navigation.base.route.NavigationRoute
import com.mapbox.navigation.base.trip.model.RouteProgress
import com.mapbox.navigation.testing.FileUtils
import com.mapbox.navigation.testing.LoggingFrontendTestRule
import io.mockk.every
import io.mockk.mockk
import junit.framework.Assert
import junit.framework.Assert.assertEquals
import kotlinx.coroutines.runBlocking
import org.junit.Rule
import org.junit.Test
Expand All @@ -17,22 +18,32 @@ import org.junit.runners.Parameterized

private typealias BooleansProvider = () -> List<Boolean?>

class NavigationRouteExTest {
class RouteProgressExTest {

@OptIn(ExperimentalPreviewMapboxNavigationAPI::class)
@RunWith(Parameterized::class)
class RouteHasUnexpectedClosures(
class RouteHasUnexpectedUpcomingClosures(
private val description: String,
private val routeRaw: String,
private val snappingIncludeClosures: BooleansProvider?,
private val snappingIncludeStaticClosures: BooleansProvider?,
private val expectedHasUnexpectedClosures: Boolean,
private val currentLegIndex: Int,
private val currentGeometryLegIndex: Int,
private val expectedHasUnexpectedUpcomingClosures: Boolean,
) {

@get:Rule
val loggerRule = LoggingFrontendTestRule()

companion object {
/**
* - `route_closure_start_coordinate`: closure[0, 8], leg [0];
* - `route_closure_second_waypoint`: closure[5, 8], leg [0] and closure[0, 8], leg[1];
* - `route_closure_second_silent_waypoint`: closure[5, 16], leg [0];
* - `route_closure_last_coordinate`: closure[4, 7], leg [1];
* - `route_closure_between_silent_and_regular_waypoints`: closure[12, 13], leg [0];
* - `route_closure_between_two_regular_waypoints`: closure[2, 3], leg [1];
*/
@JvmStatic
@Parameterized.Parameters(name = "{0}")
fun data(): List<Array<Any?>> =
Expand All @@ -42,90 +53,147 @@ class NavigationRouteExTest {
"multileg_route.json",
{ null },
{ null },
0,
0,
false,
),
arrayOf(
"route closure at starting waypoint, include closures = false",
"route closure at starting waypoint, include closures = false; " +
"the puck is in the very beginning",
"route_closure_start_coordinate.json",
provideBooleansProvider(false, true, true),
provideBooleansProvider(false, true, true),
true,
0,
0,
false,
),
arrayOf(
"route closure at starting waypoint, include closures = true",
"route_closure_start_coordinate.json",
provideBooleansProvider(true, false, false),
provideBooleansProvider(true, false, false),
0,
0,
false,
),
arrayOf(
"route closure at starting waypoint, include closures = true",
"route_closure_start_coordinate.json",
provideBooleansProvider(false, false, false),
provideBooleansProvider(true, false, false),
0,
0,
false,
),
arrayOf(
"route closure at starting waypoint, include closures = true",
"route_closure_start_coordinate.json",
provideBooleansProvider(true, false, false),
provideBooleansProvider(false, false, false),
0,
0,
false,
),
arrayOf(
"route closure at second waypoint, include closures = false",
"route_closure_second_waypoint.json",
provideBooleansProvider(true, false, true),
provideBooleansProvider(true, false, true),
0,
0,
true,
),
arrayOf(
"route closure at second waypoint, include closures = false; " +
"the puck is on the first closure of 2",
"route_closure_second_waypoint.json",
provideBooleansProvider(true, false, true),
provideBooleansProvider(true, false, true),
0,
6,
true,
),
arrayOf(
"route closure at second waypoint, include closures = false; " +
"the puck is on the second closure of 2",
"route_closure_second_waypoint.json",
provideBooleansProvider(true, false, true),
provideBooleansProvider(true, false, true),
1,
7,
false,
),
arrayOf(
"route closure at second waypoint, include closures = true",
"route_closure_second_waypoint.json",
provideBooleansProvider(false, true, false),
provideBooleansProvider(false, true, false),
0,
0,
false,
),
arrayOf(
"route closure at second *silent* waypoint, include closures = false",
"route_closure_second_silent_waypoin.json",
"route_closure_second_silent_waypoint.json",
provideBooleansProvider(true, false, true),
provideBooleansProvider(true, false, true),
0,
5,
false,
),
arrayOf(
"route closure at second *silent* waypoint, include closures = false;" +
"the puck has just stepped on the closure",
"route_closure_second_silent_waypoint.json",
provideBooleansProvider(true, false, true),
provideBooleansProvider(true, false, true),
0,
0,
true,
),
arrayOf(
"route closure at second *silent* waypoint, include closures = true",
"route_closure_second_silent_waypoin.json",
"route_closure_second_silent_waypoint.json",
provideBooleansProvider(false, true, false),
provideBooleansProvider(false, true, false),
0,
0,
false,
),
arrayOf(
"route closure at last waypoint, include closures = false",
"route_closure_last_coordinate.json",
provideBooleansProvider(true, true, false),
provideBooleansProvider(true, true, false),
0,
0,
true,
),
arrayOf(
"route closure at last waypoint, include closures = true",
"route_closure_last_coordinate.json",
provideBooleansProvider(false, false, true),
provideBooleansProvider(false, false, true),
0,
0,
false,
),
arrayOf(
"route closure between silent and regular waypoints",
"route_closure_between_silent_and_regular_waypoints.json",
provideBooleansProvider(true, true, true),
provideBooleansProvider(true, true, true),
0,
0,
true,
),
arrayOf(
"route closure between two regular waypoints",
"route_closure_between_two_regular_waypoints.json",
provideBooleansProvider(true, true, true),
provideBooleansProvider(true, true, true),
0,
0,
true,
),
)
Expand Down Expand Up @@ -158,25 +226,41 @@ class NavigationRouteExTest {
}
}

private fun mockRouteProgress(
navigationRoute: NavigationRoute,
currentLegIndex: Int,
currentLegGeometryIndex: Int,
): RouteProgress = mockk {
every { [email protected] } returns navigationRoute
every { currentLegProgress } returns mockk {
every { legIndex } returns currentLegIndex
every { geometryIndex } returns currentLegGeometryIndex
}
}

private fun provideBooleansProvider(
vararg bool: Boolean?,
): BooleansProvider = { bool.asList() }
}

@Test
fun testCases() = runBlocking {
val navRoute = mockNavigationRoute(
routeRaw,
snappingIncludeClosures,
snappingIncludeStaticClosures,
val routeProgress = mockRouteProgress(
mockNavigationRoute(
routeRaw,
snappingIncludeClosures,
snappingIncludeStaticClosures,
),
currentLegIndex,
currentGeometryLegIndex,
)

val hasUnexpectedClosures = navRoute.hasUnexpectedClosures()
val hasUnexpectedUpcomingClosures = routeProgress.hasUnexpectedUpcomingClosures()

Assert.assertEquals(
assertEquals(
description,
expectedHasUnexpectedClosures,
hasUnexpectedClosures
expectedHasUnexpectedUpcomingClosures,
hasUnexpectedUpcomingClosures
)
}
}
Expand Down