Skip to content

NAVAND-940: fix buildings highlighting #6749

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
dzinad wants to merge 1 commit into from
Closed

NAVAND-940: fix buildings highlighting #6749

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,7 @@ Mapbox welcomes participation and contributions from everyone.
- Fixed the `NavigationView` issue where route arrows are being rendered above navigation puck after device rotation. [#6747](https://github.com/mapbox/mapbox-navigation-android/pull/6747)
- Fixed an issue where a refresh of the routes or a change in alternatives that occurred while user was off-route would call `RerouteController#interrupt` and interrupt the potentially ongoing reroute process without recovery. [#6719](https://github.com/mapbox/mapbox-navigation-android/pull/6719)
- Improved OpenLR matching rate. Updates take lowest FRC to next point into account during path generation. [#6750](https://github.com/mapbox/mapbox-navigation-android/pull/6750)
- Fixed an issue where `MapboxBuildingsApi` would fail to highlight buildings. [#6749](https://github.com/mapbox/mapbox-navigation-android/pull/6749)

## Mapbox Navigation SDK 2.9.5 - 13 December, 2022
### Changelog
Expand Down
26 changes: 16 additions & 10 deletions libnavui-maps/src/main/java/com/mapbox/navigation/ui/maps/building/BuildingProcessor.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,10 +2,9 @@ package com.mapbox.navigation.ui.maps.building

import com.mapbox.bindgen.ExpectedFactory
import com.mapbox.maps.RenderedQueryOptions
import com.mapbox.navigation.base.internal.extensions.isLegWaypoint
import com.mapbox.navigation.base.internal.utils.internalWaypoints
import com.mapbox.navigation.ui.maps.building.model.BuildingError
import com.mapbox.navigation.ui.maps.building.model.BuildingValue
import com.mapbox.navigation.utils.internal.ifNonNull
import kotlin.coroutines.resume
import kotlin.coroutines.suspendCoroutine

Expand Down Expand Up @@ -48,18 +47,25 @@ internal object BuildingProcessor {
fun queryBuildingOnWaypoint(
action: BuildingAction.QueryBuildingOnWaypoint
): BuildingResult.GetDestination {
val waypoints = action.progress.navigationRoute.internalWaypoints()
val waypointIndex = action.progress.currentLegProgress?.legIndex!! + 1
val waypoint = waypoints.filter { it.isLegWaypoint() }.getOrNull(waypointIndex)
return BuildingResult.GetDestination(waypoint?.target ?: waypoint?.location)
val routeOptions = action.progress.route.routeOptions()
Copy link
Contributor

@RingerJK RingerJK Dec 15, 2022

Choose a reason for hiding this comment

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

the intention of doing this changed is EV waypoints, that added implicitly and this logic is not working.

Copy link
Contributor Author

@dzinad dzinad Dec 15, 2022

Choose a reason for hiding this comment

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

What exactly is not working? I don't see anything here that would be affected by adding EV waypoints.

Copy link
Contributor

@kmadsen kmadsen Dec 15, 2022

Choose a reason for hiding this comment

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

@RingerJK could you demonstrate the issue with a unit test?

It seems we're trying to get this into the 2.10.0-rc.1, and it'd be nice to release that soon

Copy link
Contributor

@RingerJK RingerJK Dec 15, 2022

Choose a reason for hiding this comment

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

let me provide the sample:

  • the SDK requested an EV route with 2 waypoints (origin and destination);
  • received the route with an additional waypoint in the middle (EV waypoint);
  • so we have the route with 3 waypoints, 2 legs, but originally we requested 1 leg route (2 waypoints)
  • as soon as we start to work with requested waypoints (RouteOptions) we get inconsistency because routeProgerss#legIndex == 1 is actually not the destination, it's the EV waypoint.

Copy link
Contributor

@abhishek1508 abhishek1508 Dec 15, 2022

Choose a reason for hiding this comment

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

We have 2 APIs: queryBuildingOnWaypoint and queryBuildingOnFinalDestination. In the example provided above, I would guess that queryBuildingOnWaypoint would be triggered when the user reaches the 1st waypoint and queryBuildingOnFinalDestination will be triggered when they reach the final destination. What am I missing?

cc @RingerJK

Copy link
Contributor Author

@dzinad dzinad Dec 15, 2022

Choose a reason for hiding this comment

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

Oh, legIndex is also affected.
Then we can't merge it. Thanks, I'll continue investigating tomorrow.

Copy link
Contributor

@RingerJK RingerJK Dec 15, 2022

Choose a reason for hiding this comment

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

@abhishek1508 at my example when you arrive at the first waypoint and try to take waypoint data via queryBuildingOnWaypoint you will receive data for destination because the index of destination in RouteOptions is actually 1. Does it make sense?

val coordinatesList = routeOptions?.coordinatesList()
val waypointTargets = routeOptions?.waypointTargetsList()
val coordinateIndex = action.progress.currentLegProgress?.legIndex!! + 1
val routablePoint = coordinatesList?.getOrNull(coordinateIndex)
val locationPoint = waypointTargets?.getOrNull(coordinateIndex)
return ifNonNull(locationPoint) { point ->
BuildingResult.GetDestination(point)
} ?: BuildingResult.GetDestination(routablePoint)
}

fun queryBuildingOnFinalDestination(
action: BuildingAction.QueryBuildingOnFinalDestination
): BuildingResult.GetDestination {
val lastWaypoint = action.progress.navigationRoute
.internalWaypoints()
.lastOrNull()
return BuildingResult.GetDestination(lastWaypoint?.target ?: lastWaypoint?.location)
val routeOptions = action.progress.route.routeOptions()
val lastCoordinate = routeOptions?.coordinatesList()?.lastOrNull()
val lastWaypointTarget = routeOptions?.waypointTargetsList()?.lastOrNull()
return ifNonNull(lastWaypointTarget) { point ->
BuildingResult.GetDestination(point)
} ?: BuildingResult.GetDestination(lastCoordinate)
}
}
226 changes: 101 additions & 125 deletions libnavui-maps/src/test/java/com/mapbox/navigation/ui/maps/building/BuildingProcessorTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,6 @@ import com.mapbox.maps.MapboxMap
import com.mapbox.maps.QueryFeaturesCallback
import com.mapbox.maps.RenderedQueryOptions
import com.mapbox.maps.ScreenCoordinate
import com.mapbox.navigation.base.internal.route.Waypoint
import com.mapbox.navigation.base.internal.utils.WaypointFactory
import com.mapbox.navigation.base.internal.utils.internalWaypoints
import com.mapbox.navigation.base.trip.model.RouteLegProgress
import com.mapbox.navigation.base.trip.model.RouteProgress
import io.mockk.every
Expand All @@ -26,111 +23,107 @@ class BuildingProcessorTest {

@Test
fun `map query on waypoint arrival with waypoint targets`() {
val waypoints = listOf(
provideWaypoint(
Point.fromLngLat(-122.4192, 37.7627),
Waypoint.REGULAR,
"",
Point.fromLngLat(-122.4192, 37.7627),
),
provideWaypoint(
Point.fromLngLat(-122.4182, 37.7651),
Waypoint.REGULAR,
"",
Point.fromLngLat(-122.4183, 37.7653),
),
provideWaypoint(
Point.fromLngLat(-122.4145, 37.7653),
Waypoint.REGULAR,
"",
Point.fromLngLat(-122.4146, 37.7655),
),
)
val mockOriginForWaypointTarget = Point.fromLngLat(-122.4192, 37.7627)
val mockWaypointForWaypointTarget = Point.fromLngLat(-122.4183, 37.7653)
val mockFinalForWaypointTarget = Point.fromLngLat(-122.4146, 37.7655)
val mockOriginForCoordinates = Point.fromLngLat(-122.4192, 37.7627)
val mockWaypointForCoordinates = Point.fromLngLat(-122.4182, 37.7651)
val mockFinalForCoordinates = Point.fromLngLat(-122.4145, 37.7653)
val mockRouteOptions = mockk<RouteOptions>(relaxed = true) {
every { coordinatesList() } returns listOf(
mockOriginForCoordinates,
mockWaypointForCoordinates,
mockFinalForCoordinates
)
every { waypointTargetsList() } returns listOf(
mockOriginForWaypointTarget,
mockWaypointForWaypointTarget,
mockFinalForWaypointTarget
)
}
val mockRoute = mockk<DirectionsRoute>(relaxed = true) {
every { routeOptions() } returns mockRouteOptions
}
val mockRouteLegProgress = mockk<RouteLegProgress>(relaxed = true) {
every { legIndex } returns 0
}
val mockRouteProgress = mockk<RouteProgress>(relaxed = true) {
every { currentLegProgress } returns mockRouteLegProgress
every { navigationRoute.internalWaypoints() } returns waypoints
every { route } returns mockRoute
}
val mockAction = BuildingAction.QueryBuildingOnWaypoint(mockRouteProgress)

val result = BuildingProcessor.queryBuildingOnWaypoint(mockAction)

assertEquals(waypoints[1].target!!, result.point)
assertEquals(result.point, mockWaypointForWaypointTarget)
}

@Test
fun `map query on waypoint arrival with some waypoint targets`() {
val waypoints = listOf(
provideWaypoint(
Point.fromLngLat(-122.4192, 37.7627),
Waypoint.REGULAR,
"",
Point.fromLngLat(-122.4192, 37.7627),
),
provideWaypoint(
Point.fromLngLat(-122.4182, 37.7651),
Waypoint.REGULAR,
"",
null,
),
provideWaypoint(
Point.fromLngLat(-122.4145, 37.7653),
Waypoint.REGULAR,
"",
Point.fromLngLat(-122.4146, 37.7655),
),
)
val mockOriginForWaypointTarget = Point.fromLngLat(-122.4192, 37.7627)
val mockWaypointForWaypointTarget = null
val mockFinalForWaypointTarget = Point.fromLngLat(-122.4146, 37.7655)
val mockOriginForCoordinates = Point.fromLngLat(-122.4192, 37.7627)
val mockWaypointForCoordinates = Point.fromLngLat(-122.4182, 37.7651)
val mockFinalForCoordinates = Point.fromLngLat(-122.4145, 37.7653)
val mockRouteOptions = mockk<RouteOptions>(relaxed = true) {
every { coordinatesList() } returns listOf(
mockOriginForCoordinates,
mockWaypointForCoordinates,
mockFinalForCoordinates
)
every { waypointTargetsList() } returns listOf(
mockOriginForWaypointTarget,
mockWaypointForWaypointTarget,
mockFinalForWaypointTarget
)
}
val mockRoute = mockk<DirectionsRoute>(relaxed = true) {
every { routeOptions() } returns mockRouteOptions
}
val mockRouteLegProgress = mockk<RouteLegProgress>(relaxed = true) {
every { legIndex } returns 0
}
val mockRouteProgress = mockk<RouteProgress>(relaxed = true) {
every { currentLegProgress } returns mockRouteLegProgress
every { navigationRoute.internalWaypoints() } returns waypoints
every { route } returns mockRoute
}

val mockAction = BuildingAction.QueryBuildingOnWaypoint(mockRouteProgress)

val result = BuildingProcessor.queryBuildingOnWaypoint(mockAction)

assertEquals(waypoints[1].location, result.point)
assertEquals(result.point, mockWaypointForCoordinates)
}

@Test
fun `map query on waypoint arrival without waypoint targets`() {
val waypoints = listOf(
provideWaypoint(
Point.fromLngLat(-122.4192, 37.7627),
Waypoint.REGULAR,
"",
null,
),
provideWaypoint(
Point.fromLngLat(-122.4182, 37.7651),
Waypoint.REGULAR,
"",
null,
),
provideWaypoint(
Point.fromLngLat(-122.4145, 37.7653),
Waypoint.REGULAR,
"",
null,
),
)
val mockOriginForCoordinates = Point.fromLngLat(-122.4192, 37.7627)
val mockWaypointForCoordinates = Point.fromLngLat(-122.4182, 37.7651)
val mockFinalForCoordinates = Point.fromLngLat(-122.4145, 37.7653)
val mockRouteOptions = mockk<RouteOptions>(relaxed = true) {
every { coordinatesList() } returns listOf(
mockOriginForCoordinates,
mockWaypointForCoordinates,
mockFinalForCoordinates
)
every { waypointTargetsList() } returns null
}
val mockRoute = mockk<DirectionsRoute>(relaxed = true) {
every { routeOptions() } returns mockRouteOptions
}
val mockRouteLegProgress = mockk<RouteLegProgress>(relaxed = true) {
every { legIndex } returns 0
}
val mockRouteProgress = mockk<RouteProgress>(relaxed = true) {
every { currentLegProgress } returns mockRouteLegProgress
every { navigationRoute.internalWaypoints() } returns waypoints
every { route } returns mockRoute
}
val mockAction = BuildingAction.QueryBuildingOnWaypoint(mockRouteProgress)

val result = BuildingProcessor.queryBuildingOnWaypoint(mockAction)

assertEquals(waypoints[1].location, result.point)
assertEquals(result.point, mockWaypointForCoordinates)
}

@Test
Expand Down Expand Up @@ -158,74 +151,69 @@ class BuildingProcessorTest {

@Test
fun `map query on final destination arrival with waypoint targets`() {
val waypoints = listOf(
provideWaypoint(
Point.fromLngLat(-122.4192, 37.7627),
Waypoint.REGULAR,
"",
Point.fromLngLat(-122.4192, 37.7627),
),
provideWaypoint(
Point.fromLngLat(-122.4182, 37.7651),
Waypoint.REGULAR,
"",
Point.fromLngLat(-122.4183, 37.7653),
),
provideWaypoint(
Point.fromLngLat(-122.4145, 37.7653),
Waypoint.REGULAR,
"",
Point.fromLngLat(-122.4146, 37.7655),
),
)
val mockOriginForWaypointTarget = Point.fromLngLat(-122.4192, 37.7627)
val mockWaypointForWaypointTarget = Point.fromLngLat(-122.4183, 37.7653)
val mockFinalForWaypointTarget = Point.fromLngLat(-122.4146, 37.7655)
val mockOriginForCoordinates = Point.fromLngLat(-122.4192, 37.7627)
val mockWaypointForCoordinates = Point.fromLngLat(-122.4182, 37.7651)
val mockFinalForCoordinates = Point.fromLngLat(-122.4145, 37.7653)
val mockRouteOptions = mockk<RouteOptions>(relaxed = true) {
every { coordinatesList() } returns listOf(
mockOriginForCoordinates,
mockWaypointForCoordinates,
mockFinalForCoordinates
)
every { waypointTargetsList() } returns listOf(
mockOriginForWaypointTarget,
mockWaypointForWaypointTarget,
mockFinalForWaypointTarget
)
}
val mockRoute = mockk<DirectionsRoute>(relaxed = true) {
every { routeOptions() } returns mockRouteOptions
}
val mockRouteLegProgress = mockk<RouteLegProgress>(relaxed = true) {
every { legIndex } returns 0
}
val mockRouteProgress = mockk<RouteProgress>(relaxed = true) {
every { currentLegProgress } returns mockRouteLegProgress
every { navigationRoute.internalWaypoints() } returns waypoints
every { route } returns mockRoute
}
val mockAction = BuildingAction.QueryBuildingOnFinalDestination(mockRouteProgress)

val result = BuildingProcessor.queryBuildingOnFinalDestination(mockAction)

assertEquals(waypoints.last().target!!, result.point)
assertEquals(result.point, mockFinalForWaypointTarget)
}

@Test
fun `map query on final destination arrival without waypoint targets`() {
val waypoints = listOf(
provideWaypoint(
Point.fromLngLat(-122.4192, 37.7627),
Waypoint.REGULAR,
"",
null,
),
provideWaypoint(
Point.fromLngLat(-122.4182, 37.7651),
Waypoint.REGULAR,
"",
null,
),
provideWaypoint(
Point.fromLngLat(-122.4145, 37.7653),
Waypoint.REGULAR,
"",
null,
),
)
val mockOriginForCoordinates = Point.fromLngLat(-122.4192, 37.7627)
val mockWaypointForCoordinates = Point.fromLngLat(-122.4182, 37.7651)
val mockFinalForCoordinates = Point.fromLngLat(-122.4145, 37.7653)
val mockRouteOptions = mockk<RouteOptions>(relaxed = true) {
every { coordinatesList() } returns listOf(
mockOriginForCoordinates,
mockWaypointForCoordinates,
mockFinalForCoordinates
)
every { waypointTargetsList() } returns null
}
val mockRoute = mockk<DirectionsRoute>(relaxed = true) {
every { routeOptions() } returns mockRouteOptions
}
val mockRouteLegProgress = mockk<RouteLegProgress>(relaxed = true) {
every { legIndex } returns 0
}
val mockRouteProgress = mockk<RouteProgress>(relaxed = true) {
every { currentLegProgress } returns mockRouteLegProgress
every { navigationRoute.internalWaypoints() } returns waypoints
every { route } returns mockRoute
}
val mockAction = BuildingAction.QueryBuildingOnFinalDestination(mockRouteProgress)

val result = BuildingProcessor.queryBuildingOnFinalDestination(mockAction)

assertEquals(waypoints.last().location, result.point)
assertEquals(result.point, mockFinalForCoordinates)
}

@Test
Expand Down Expand Up @@ -253,16 +241,4 @@ class BuildingProcessorTest {
assertTrue(optionsSlot.captured.layerIds!!.contains("building"))
assertTrue(optionsSlot.captured.layerIds!!.contains("building-extrusion"))
}

private fun provideWaypoint(
location: Point,
@Waypoint.Type type: Int,
name: String,
target: Point?,
): Waypoint = WaypointFactory.provideWaypoint(
location,
name,
target,
type
)
}