Make active leg of route appear above inactive legs. #6742
Conversation
Codecov Report
@@ Coverage Diff @@
## main #6742 +/- ##
=========================================
Coverage 72.68% 72.68%
Complexity 5572 5572
=========================================
Files 782 782
Lines 30166 30166
Branches 3562 3562
=========================================
Hits 21926 21926
Misses 6814 6814
Partials 1426 1426 |
cafesilencio
force-pushed
the
sb-NAVSDK-723-active-legs-over-inactive
branch
4 times, most recently
from
f144870 to
8d505c7
Compare
|
@LukasPaczos If you look at the video I posted above there's a delay in the update of the route line when the puck goes from the first leg of the route to the second. As I was investigating this I noticed an irregularity with the route progress updates. I commented out the new code and logged the calls before any work is done by the route line classes. There's a visible pattern in the incoming route progress calls vs the in coming updateTraveledRouteLine calls until the route leg changes. At that point there's a clear irregularity with route progress updates vs the updateTraveledRouteLine updates. I also put the logging in the activity before any calls to the route line related classes and I see the same irregularity when transitioning from the first leg to the second leg. Do you have an explanation for this? It looks like a problem. 14:04:08.635 20539-20539 .qa_test_app E this is route progress Heres's the logging from the activity before any other code is called. The irregularity is noticeable 14:08:52.908 20912-20912 .qa_test_app E received routeProgress |
|
@cafesilencio there's a possibility that a call to switch legs of the route triggers a forced progress update (same can happen with alternatives update or route refreshes), that's why you might see them unevenly spaced. That is not entirely unexpected though, as long as it recovers to be at the same pace as raw location updates after leg switching operation is complete. |
cafesilencio
force-pushed
the
sb-NAVSDK-723-active-legs-over-inactive
branch
2 times, most recently
from
0f9a30a to
719b483
Compare
| assertEquals( | ||
| "mapbox-layerGroup-1-restricted", | ||
| style.styleLayers[topLevelRouteLayerIndex - 1].id | ||
| ) | ||
| assertEquals( | ||
| "mapbox-layerGroup-1-traffic", | ||
| "mapbox-masking-layer-traffic", | ||
| style.styleLayers[topLevelRouteLayerIndex - 2].id | ||
| ) |
There was a problem hiding this comment.
Why is the restricted layer on top of the masking layer? Why is there no masking restrictions layer?
There was a problem hiding this comment.
Suppose the second leg is or contains a restricted road and unauthorized usage resulted in a penalty of some kind. If the restricted road is masked it won't be visible until the driver is on it and at that point it's too late to avoid it. I thought it better to always have the restricted road visible so drivers can make decisions about it.
Also the behavior of the current release is to show the entire restricted section/road even if independent leg styling option is true when the route is initially drawn. Masking the restricted section of inactive legs would change that behavior.
There was a problem hiding this comment.
Suppose the second leg is or contains a restricted road and unauthorized usage resulted in a penalty of some kind. If the restricted road is masked it won't be visible until the driver is on it and at that point it's too late to avoid it.
This actually creates, in my opinion, a very undesirable side effect. In situations where legs do not overlap, there's no problem in either of the approaches, restrictions are always visible. However, if legs do overlap and the restriction is only applicable in one way, in the current proposal we could be drawing a restriction on top of the current leg of the route even though it only applies in the other direction, which could be very confusing. Also, the restriction won't be masked until the driver is on it, it will only be masked until the driver reaches the first waypoint, so in all cases they should have an opportunity to deviate.
There was a problem hiding this comment.
@cafesilencio this still stands. I do think that restrictions on upcoming legs should appear below the current leg at all time.
There was a problem hiding this comment.
I made some adjustments resulting in the entire restricted section displaying when the line is first drawn so that on route overview the entire route is accurately represented. When navigation starts all the active leg layers are above inactive layers. See the Inactive Route Leg Styling With Restrictions activity in the QA app. for a demonstration.
| the primary route line layers except for the restricted line which appears above all other | ||
| primary route layers. The implementation will set the inactive route legs to transparent |
There was a problem hiding this comment.
Why would restrictions appear above everything else? If we have an overpass and our current lane/road is not restricted but the one below/above us is (and it's on the next leg), we don't want that restriction poking through our current leg.
There was a problem hiding this comment.
See comment above.
| @@ -730,34 +760,166 @@ class MapboxRouteLineApi( | |||
| updateUpcomingRoutePointIndex(routeProgress) | |||
| updateVanishingPointState(routeProgress.currentState) | |||
|
|
|||
| val routeLineMaskingLayerDynamicData = when ( | |||
| (routeProgress.currentLegProgress?.legIndex ?: INVALID_ACTIVE_LEG_INDEX) > | |||
There was a problem hiding this comment.
| (routeProgress.currentLegProgress?.legIndex ?: INVALID_ACTIVE_LEG_INDEX) > | |
| (routeProgress.currentLegProgress?.legIndex ?: INVALID_ACTIVE_LEG_INDEX) != |
There was a problem hiding this comment.
Could you add a unit test for this as well?
There was a problem hiding this comment.
If a driver is on the second leg (for example) and a route progress comes in with a different route, due to a reroute for example, and the route hasn't yet been received via setRoutes then using != would produce incorrect data. Using > ensures the data is calculated only if there's an increase in the legIndex else nothing is calculated.
There was a problem hiding this comment.
route progress comes in with a different route, due to a reroute for example, and the route hasn't yet been received via setRoutes
This is impossible, see
The current logic would prohibit developer returning back to the previous leg, which I don't think is prohibited on the core level.
| { | ||
| MapboxRouteLineUtils.getRestrictedLineExpression( | ||
| offset, | ||
| activeLegIndex, |
There was a problem hiding this comment.
This activeLegIndex usage is not guarded by routeLineOptions.styleInactiveRouteLegsIndependently, so even if styleInactiveRouteLegsIndependently was disabled, we still wouldn't show restrictions on legs other than currently active. It used to default to -1 if styleInactiveRouteLegsIndependently was disabled but that's not the case anymore
There was a problem hiding this comment.
When the (multi-leg) route is initially drawn the restricted section will display regardless of whether styleInactiveRouteLegsIndependently is true or false. This is the case with the current release and the work in this PR hasn't changed that. This can be seen with the InactiveRouteStylingWithRestrictionsActivity in the QA app. by enabling/disabling styleInactiveRouteLegsIndependently.
When navigation starts and styleInactiveRouteLegsIndependently is true the inactive leg(s) gets colored to the appropriate color including the restricted section of the inactive leg(s). This behavior hasn't changed between the current release and this PR and can be seen using the InactiveRouteStylingWithRestrictionsActivity in the QA app.
Adjustments had to be made to preserve this behavior. Until now activeLegIndex was only relevant if styleInactiveRouteLegsIndependently was true but now it's relevant for all multi-leg routes because it's needed to calculate the masking layer gradients correctly.
| getRestrictedLineExpressionProducer( | ||
| extractedRouteData, | ||
| restrictedExpressionData, | ||
| vanishingPointOffset = 0.0, | ||
| activeLegIndex = activeLegIndex, |
There was a problem hiding this comment.
This activeLegIndex usage is not guarded by routeLineOptions.styleInactiveRouteLegsIndependently, so even if styleInactiveRouteLegsIndependently was disabled, we still wouldn't show restrictions on legs other than currently active. It used to default to -1 if styleInactiveRouteLegsIndependently was disabled but that's not the case anymore
There was a problem hiding this comment.
See my comment above.
libnavui-maps/src/main/java/com/mapbox/navigation/ui/maps/route/line/api/MapboxRouteLineApi.kt
Show resolved
Hide resolved
| consumer: MapboxNavigationConsumer<Expected<RouteLineError, RouteLineUpdateValue>> | ||
| ) { | ||
| ifNonNull(routeLineOverlayDynamicData) { maskingLayerData -> | ||
| getRouteDrawData { expected -> |
There was a problem hiding this comment.
How expensive is this function call for very long routes? We ultimately only need the dynamic data from it, we don't need the geometries and all the rest. The vanishing update functions are able to get the dynamic data without rebuilding geometries so maybe we could try doing the same here, unless it's cheap to get all of it of course.
There was a problem hiding this comment.
With all the caching in the class calling this shouldn't be that expensive. It's only called when there is a transition from one route leg to another.
I'm going from memory here but I think the main reason I had to do this was because the data type returned (RoutelineUpdateValue) has non-nullable primary and alternative RouteLineDyamicData fields. I had to get data from somewhere and getRouteDrawData seemed like an obvious source. Because of RouteLineUpdateValue we're painted into a bit of a corner here.
| fun showRouteWithLegIndexHighlighted( | ||
| legIndexToHighlight: Int, | ||
| consumer: MapboxNavigationConsumer<Expected<RouteLineError, RouteLineUpdateValue>> | ||
| ) { | ||
| showRouteWithLegIndexHighlighted(legIndexToHighlight, null, consumer) | ||
| } |
There was a problem hiding this comment.
Shouldn't usage of this function reset activeLegIndex? What are other side effects of using this function while in active guidance with the current setup?
There was a problem hiding this comment.
This method doesn't change any state, not in the current release and not with the modifications in this PR. Its purpose is to get a view of the route line with a specific leg active. It's similar to getRotueDrawData it just gives a view of the route line in its current state. The docs explicitly indicate this method makes no state changes.
| jobControl.scope.launch(Dispatchers.Main) { | ||
| routeLineExpressionData = deferred.await() | ||
| } |
There was a problem hiding this comment.
We've made improvements over time, do we still need to defer this function? What's the execution time?
There was a problem hiding this comment.
On my device using a route across the U.S. the time is 155ms. If it's a multi-leg route the data is needed right away so the execution waits for it to complete but if it's a single leg route there's no penalty since it's done in the background which matches the behavior of the currently released code.
cafesilencio
force-pushed
the
sb-NAVSDK-723-active-legs-over-inactive
branch
2 times, most recently
from
d759e2c to
127572d
Compare
|
@LukasPaczos FYI this issue: #6784 was reported. Since this PR includes a minor refactor related to the caching of |
cafesilencio
force-pushed
the
sb-NAVSDK-723-active-legs-over-inactive
branch
from
127572d to
4e6a7f8
Compare
|
Wrt #6742 (comment), here's a before and after when applying this diff: diff --git a/qa-test-app/src/main/java/com/mapbox/navigation/qa_test_app/view/InactiveRouteStylingWithRestrictionsActivity.kt b/qa-test-app/src/main/java/com/mapbox/navigation/qa_test_app/view/InactiveRouteStylingWithRestrictionsActivity.kt
index 030cbb2614..3e85bda328 100644
--- a/qa-test-app/src/main/java/com/mapbox/navigation/qa_test_app/view/InactiveRouteStylingWithRestrictionsActivity.kt
+++ b/qa-test-app/src/main/java/com/mapbox/navigation/qa_test_app/view/InactiveRouteStylingWithRestrictionsActivity.kt
@@ -92,7 +92,7 @@ class InactiveRouteStylingWithRestrictionsActivity : AppCompatActivity() {
MapboxRouteLineOptions.Builder(this)
.withRouteLineResources(routeLineResources)
.withRouteLineBelowLayerId("road-label-navigation")
- .styleInactiveRouteLegsIndependently(true)
+ // .styleInactiveRouteLegsIndependently(true)
.displayRestrictedRoadSections(true)
.withVanishingRouteLineEnabled(true)
.build()
@@ -164,6 +164,7 @@ class InactiveRouteStylingWithRestrictionsActivity : AppCompatActivity() {
) { style ->
routeLineApi.setNavigationRoutes(listOf(navigationRoute)) {
routeLineView.renderRouteDrawData(style, it)
+ routeLineView.renderRouteLineUpdate(style, routeLineApi.setVanishingOffset(0.1))
}
val routeOrigin = Utils.getRouteOriginPoint(navigationRoute.directionsRoute)
As seen above, there are two regressions on this branch - the vanishing offset is not applied and the restrictions on legs that are not active is gone. Interestingly, |
cafesilencio
force-pushed
the
sb-NAVSDK-723-active-legs-over-inactive
branch
from
4e6a7f8 to
5e8ac7b
Compare
|
Regarding #6742 (comment) good catch. Fixed.
|
cafesilencio
force-pushed
the
sb-NAVSDK-723-active-legs-over-inactive
branch
from
5e8ac7b to
f2389d0
Compare
cafesilencio
force-pushed
the
sb-NAVSDK-723-active-legs-over-inactive
branch
2 times, most recently
from
581aa6b to
8090e30
Compare
ChangelogFeaturesBug fixes and improvements
Known issues
|
cafesilencio
force-pushed
the
sb-NAVSDK-723-active-legs-over-inactive
branch
3 times, most recently
from
07d97e5 to
ffa0438
Compare
There was a problem hiding this comment.
https://github.com/mapbox/mapbox-navigation-android/pull/6742/files#r1062394113 is still true.
diff --git a/qa-test-app/src/main/java/com/mapbox/navigation/qa_test_app/view/InactiveRouteStylingWithRestrictionsActivity.kt b/qa-test-app/src/main/java/com/mapbox/navigation/qa_test_app/view/InactiveRouteStylingWithRestrictionsActivity.kt
index 030cbb2614..77946fd9b8 100644
--- a/qa-test-app/src/main/java/com/mapbox/navigation/qa_test_app/view/InactiveRouteStylingWithRestrictionsActivity.kt
+++ b/qa-test-app/src/main/java/com/mapbox/navigation/qa_test_app/view/InactiveRouteStylingWithRestrictionsActivity.kt
@@ -92,7 +92,7 @@ class InactiveRouteStylingWithRestrictionsActivity : AppCompatActivity() {
MapboxRouteLineOptions.Builder(this)
.withRouteLineResources(routeLineResources)
.withRouteLineBelowLayerId("road-label-navigation")
- .styleInactiveRouteLegsIndependently(true)
+ .styleInactiveRouteLegsIndependently(false)
.displayRestrictedRoadSections(true)
.withVanishingRouteLineEnabled(true)
.build()
@@ -164,6 +164,9 @@ class InactiveRouteStylingWithRestrictionsActivity : AppCompatActivity() {
) { style ->
routeLineApi.setNavigationRoutes(listOf(navigationRoute)) {
routeLineView.renderRouteDrawData(style, it)
+ routeLineApi.setVanishingOffset(0.2).let {
+ routeLineView.renderRouteLineUpdate(style, it)
+ }
}
val routeOrigin = Utils.getRouteOriginPoint(navigationRoute.directionsRoute)the above diff results in
As you can see, the restrictions on the second leg were hidden, even though they shouldn't be. The check should be guarded by the option of whether legs are styled independently.
In best case, we should refactor all restriction generation util functions because they are incredibly messy right now and try to do too many tasks at once. They don't all need to take current active leg index as a parameter, and that parameter behaves differently if legs are styled independently or not.
...maps/src/main/java/com/mapbox/navigation/ui/maps/internal/route/line/MapboxRouteLineUtils.kt
Show resolved
Hide resolved
| the primary route line layers except for the restricted line which appears above all other | ||
| primary route layers. The implementation will set the inactive route legs to transparent |
There was a problem hiding this comment.
| the primary route line layers except for the restricted line which appears above all other | |
| primary route layers. The implementation will set the inactive route legs to transparent | |
| the primary route line layers. The implementation will set the inactive route legs to transparent |
| ) | ||
| else -> { | ||
| ifNonNull(routeProgress.currentLegProgress) { routeLegProgress -> | ||
| if (routeLegProgress.legIndex > activeLegIndex) { |
There was a problem hiding this comment.
This check is always false since you are assigning activeLegIndex = routeProgress.currentLegProgress?.legIndex above.
There was a problem hiding this comment.
Good catch. Adjusted.
| } else { | ||
| activeLegIndex | ||
| } |
There was a problem hiding this comment.
We should never end up in this branch of code. Shouldn't we just do activeLegIndex = 0? Is there a use case that needs to be considered?
There was a problem hiding this comment.
There might have been at one time or perhaps I was being conservative. It doesn't appear to be needed in the latest iteration.
There was a problem hiding this comment.
There might have been at one time or perhaps I was being conservative. It doesn't appear to be needed in the latest iteration.
|
@LukasPaczos @cafesilencio do you know approximate ETA for this PR and Nav SDK version? need for the https://github.com/mapbox/gm-navigation-app/issues/2695 |
|
@yuryybk we're targeting v2.11 that is planned to go stable by the end of Feb. |
cafesilencio
force-pushed
the
sb-NAVSDK-723-active-legs-over-inactive
branch
from
ffa0438 to
1815a21
Compare
|
@LukasPaczos This looks resolved now.
|
cafesilencio
force-pushed
the
sb-NAVSDK-723-active-legs-over-inactive
branch
from
1815a21 to
e8b2198
Compare
| { | ||
| MapboxRouteLineUtils.getRestrictedLineExpression( | ||
| offset, | ||
| -1, |
There was a problem hiding this comment.
| -1, | |
| INVALID_ACTIVE_LEG_INDEX, |
| val routeLineMaskingLayerDynamicData = when ( | ||
| (routeProgress.currentLegProgress?.legIndex ?: INVALID_ACTIVE_LEG_INDEX) != | ||
| activeLegIndex | ||
| ) { | ||
| true -> getRouteLineDynamicDataForMaskingLayers(currentPrimaryRoute, routeProgress) | ||
| false -> null | ||
| } | ||
| val legChange = (routeProgress.currentLegProgress?.legIndex ?: 0) > activeLegIndex | ||
| activeLegIndex = routeProgress.currentLegProgress?.legIndex ?: INVALID_ACTIVE_LEG_INDEX |
There was a problem hiding this comment.
| val routeLineMaskingLayerDynamicData = when ( | |
| (routeProgress.currentLegProgress?.legIndex ?: INVALID_ACTIVE_LEG_INDEX) != | |
| activeLegIndex | |
| ) { | |
| true -> getRouteLineDynamicDataForMaskingLayers(currentPrimaryRoute, routeProgress) | |
| false -> null | |
| } | |
| val legChange = (routeProgress.currentLegProgress?.legIndex ?: 0) > activeLegIndex | |
| activeLegIndex = routeProgress.currentLegProgress?.legIndex ?: INVALID_ACTIVE_LEG_INDEX | |
| val legChange = (routeProgress.currentLegProgress?.legIndex ?: INVALID_ACTIVE_LEG_INDEX) != activeLegIndex | |
| val routeLineMaskingLayerDynamicData = when ( | |
| (legChange) { | |
| true -> getRouteLineDynamicDataForMaskingLayers(currentPrimaryRoute, routeProgress) | |
| false -> null | |
| } | |
| activeLegIndex = routeProgress.currentLegProgress?.legIndex ?: INVALID_ACTIVE_LEG_INDEX |
| jobControl.scope.launch(Dispatchers.Main) { | ||
| mutex.withLock { | ||
| alternativelyStyleSegmentsNotInLeg( | ||
| routeLegProgress.legIndex, |
There was a problem hiding this comment.
| routeLegProgress.legIndex, | |
| activeLegIndex, |
| >, | ||
| List<RouteLineExpressionData>> by lazy { LruCache(2) } | ||
| List<RouteLineExpressionData>> by lazy { LruCache(4) } |
There was a problem hiding this comment.
Why cache 4 elements? There can be more than 4 legs in the route. Do we need this LRU cache at all? Couldn't we store data for the current active leg index only? We calculate the data only on leg change anyway and we won't need data for other (especially already traversed legs) anymore, we shouldn't need to store it.
cafesilencio
force-pushed
the
sb-NAVSDK-723-active-legs-over-inactive
branch
from
e8b2198 to
d00e194
Compare
…earance the active leg is above the inactive legs for multi-leg routes.
cafesilencio
force-pushed
the
sb-NAVSDK-723-active-legs-over-inactive
branch
from
d00e194 to
7cf7721
Compare
Description
In this pull request is an implementation that gives the appearance the active leg of the route line is above the inactive legs. Additional route line layers were added to mask the primary route line. They are placed just above the primary route line related layers in the layer stack. The section(s) of the route line representing the inactive route leg(s) are made transparent revealing the primary route line layers. However the active leg section of the line is left opaque thus masking the primary route line layers beneath.
This feature is visualized when a multi-leg route with an inactive leg overlaps an active leg. See the video demonstration below.
Screenshots or Gifs
az_recorder_20221213_121205_edited.mp4