NAVAND-714 - Added Waypoints to NavigationRoute; - RouteOptionsUpdater refactored; #6005

RingerJK · 2022-07-04T10:53:36Z

Description

codecov · 2022-07-13T11:17:43Z

Codecov Report

Merging #6005 (0636da9) into main (5e27577) will decrease coverage by 0.12%.
The diff coverage is 62.22%.

❗ Current head 0636da9 differs from pull request most recent head 8270348. Consider uploading reports for the commit 8270348 to get more accurate results

@@             Coverage Diff              @@
##               main    #6005      +/-   ##
============================================
- Coverage     70.35%   70.22%   -0.13%     
- Complexity     4913     4914       +1     
============================================
  Files           718      722       +4     
  Lines         27980    28023      +43     
  Branches       3299     3303       +4     
============================================
- Hits          19685    19680       -5     
- Misses         7017     7068      +51     
+ Partials       1278     1275       -3

Impacted Files	Coverage Δ
...avigation/base/internal/utils/DirectionsRouteEx.kt	`40.00% <0.00%> (-36.93%)`	⬇️
.../mapbox/navigation/base/internal/utils/RouterEx.kt	`45.45% <0.00%> (-4.55%)`	⬇️
.../navigation/base/internal/utils/WaypointFactory.kt	`0.00% <0.00%> (ø)`
...mapbox/navigation/base/trip/model/RouteProgress.kt	`0.00% <ø> (ø)`
...pbox/navigation/core/accounts/BillingController.kt	`88.09% <ø> (ø)`
...avigation/core/history/model/HistoryEventMapper.kt	`0.00% <0.00%> (ø)`
.../mapbox/navigation/base/internal/route/Waypoint.kt	`11.53% <11.53%> (ø)`
...x/navigation/ui/maps/building/BuildingProcessor.kt	`68.00% <62.50%> (+2.37%)`	⬆️
...avigation/core/routeoptions/RouteOptionsUpdater.kt	`83.33% <85.05%> (-4.44%)`	⬇️
.../navigation/base/internal/extensions/WaypointEx.kt	`94.73% <94.73%> (ø)`
... and 13 more

libnavigation-base/src/main/java/com/mapbox/navigation/base/internal/route/Waypoint.kt

libnavigation-base/src/main/java/com/mapbox/navigation/base/route/NavigationRoute.kt

dzinad · 2022-10-25T12:24:40Z

Which ticket is it?

RingerJK · 2022-10-25T12:29:34Z

Which ticket is it?

Added ticket in the title

dzinad · 2022-10-26T08:17:22Z

A more general concern: RouteProgress is public API. And it has remainingWaypoints property.

Docs should be updated to specify which waypoints these are;
An idea: would it make sense to add another field for the other type of waypoints? My concern is that we might have to apply the logic that's in RouteOptionsUpdater somewhere else if we use RouteProgress#remainingWaypoints somewhere else. Also the end user can use this property. Now that the waypoints are of different types, it would make sense to allow the user to choose which ones they'd like to use. TBH I don't really understand what's happening in RouteOptionsUpdater. What's refactoring and what's logic change. IMO it would be nice to separate: the logic change would be in calculateRemainingWaypoints or somewhere near it. WDYT?

dzinad · 2022-10-26T08:18:05Z

BillingController uses RouteProgress#remainingWaypoints. Should its logic stay the same? I'd think yes, but want to clarify.

libnavigation-base/src/main/java/com/mapbox/navigation/base/internal/extensions/WaypointEx.kt

libnavigation-base/src/main/java/com/mapbox/navigation/base/internal/utils/DirectionsRouteEx.kt

libnavigation-base/src/main/java/com/mapbox/navigation/base/route/NavigationRoute.kt

dzinad · 2022-10-26T07:46:00Z

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

+                        mutableListOf<Double?>().also {
+                            it.addAll(
+                                radiusesList.subList(
+                                    nextCoordinateIndex - 1,


Is it guaranteed somewhere that nextCoordinateIndex is > 0?

nextCoordinateIndex is receiving from indexOfNextRequestedCoordinate, which is always return value between [1; size_of_waypoint_list) (null is also possible, but it filtered out)

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

libnavigation-base/src/main/java/com/mapbox/navigation/base/internal/extensions/WaypointEx.kt

libnavigation-base/src/main/java/com/mapbox/navigation/base/internal/route/Waypoint.kt

LukasPaczos · 2022-10-25T16:29:50Z

libnavigation-base/src/main/java/com/mapbox/navigation/base/internal/route/Waypoint.kt

+import com.mapbox.geojson.Point
+
+data class Waypoint(
+    val location: Point,


Suggested change

val location: Point,

val directionsWaypoint: DirectionsWaypoint?,

This would be handy instead of copying the location/name data. It could be null for silent waypoints or we could have sealed classes.

do you mean fakeing DirectionsWaypoint based on Waypoint instance?

We don't need be faking them, we can be finding them in the route response object depending on the index.

We don't always have DirectionsWaypoint: for instance, we don't have it when creating NavigationRoute based on DirecionsRoute(s) and a Waypoint object is created based on the NN's Waypoint.

Okay, let's revisit later if needed.

libnavigation-base/src/main/java/com/mapbox/navigation/base/internal/utils/RouterEx.kt

libnavigation-base/src/main/java/com/mapbox/navigation/base/route/NavigationRoute.kt

LukasPaczos · 2022-10-26T10:53:11Z

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

@@ -379,7 +380,7 @@ internal class MapboxTripSession(
        }

        private fun calculateRemainingWaypoints(tripStatus: TripStatus): Int {
-            val routeCoordinates = tripStatus.route?.routeOptions?.coordinatesList()
+            val routeCoordinates = tripStatus.route?._waypoints()


This would now also report silent waypoints which is a breaking change. It should only report regular and EV waypoints, so actually the ones that create legs.

why it's breaking changes? We haven't had EV waypoints, as they appeared, now is a part of the API if anybody uses it

I'm talking about silent waypoints. They are not really waypoints and should not be part of the remaining waypoints count.

~~good catch~~
@LukasPaczos wait a sec, the line tripStatus.route?.routeOptions?.coordinatesList() also take into account silent waypoints 🤔

Also, silent and Ev waypoints are under NavigationStatus#getNextWaypointIndex(which is root of the remainingWaypoints value)

Maybe take a look at #6005 (comment), I think it's related to this discussion.

I think it's rather confusing that we call this a remainingWaypoint when a silent waypoint is not truly a waypoint in the Directions API terminology.

I'm worried that if someone used to use remainingWaypoints to compare it with RouteOptions#coordinates (as that's the only practical usage because remainingWaypoints contains silent waypoints) then the proposed change will break that logic.

from the SDK side, it seems like not a breaking change, it might be a misunderstanding between the SDK docs and customers. Let's considered it in the API doc and add the warning to the changelog. WDYT?

Yea, this is fine, refs #6005 (comment). But I agree that we should add a note in the docs that remainingWaypoints now might include more than requested waypoints if EV is used.

RingerJK · 2022-10-26T15:10:21Z

@dzinad

Docs should be updated to specify which waypoints these are;

if we are speaking about Waypoint.kt - I'll do it 👍

An idea: would it make sense to add another field for the other type of waypoints? My concern is that we might have to apply the logic that's in RouteOptionsUpdater somewhere else if we use RouteProgress#remainingWaypoints somewhere else. Also the end user can use this property. Now that the waypoints are of different types, it would make sense to allow the user to choose which ones they'd like to use. TBH I don't really understand what's happening in RouteOptionsUpdater. What's refactoring and what's logic change. IMO it would be nice to separate: the logic change would be in calculateRemainingWaypoints or somewhere near it. WDYT?

The Waypoint class is internal itself so we're good to experiment and change it.

What fields would you have in mind to add for other types of waypoints?
how do you see customers might choose the particular type of waypoints? For now, they are mostly like passive observer
Yeah, I agree, the logic is a bit hard to get in RouteOptionsUpdater. Would it help if I move everything into separate functions?

BillingController uses RouteProgress#remainingWaypoints. Should its logic stay the same? I'd think yes, but want to clarify.

great catch actually. I would say it should be switched to the Waypoint as well (I see there's using DirecionsResponce#DirectionsWaypoint. @LukasPaczos do you have objections?

LukasPaczos · 2022-10-26T15:51:51Z

We can switch the billing controller to use Waypoint but it should ignore silent waypoints, it's only interested in leg waypoints.

libnavigation-base/src/main/java/com/mapbox/navigation/base/internal/extensions/WaypointEx.kt

LukasPaczos · 2022-10-26T16:15:53Z

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

+                        }
+                    }
+                )
+                .approachesList(


Approaches should be specified per waypoint, not per coordinate (capturing from https://docs.mapbox.com/api/navigation/directions/#optional-parameters), so it should use getUpdatedWaypointsList.

We can address separately, let's cut a ticket.

NAVAND-834

LukasPaczos · 2022-10-26T16:26:11Z

great catch actually. I would say it should be switched to the Waypoint as well (I see there's using DirecionsResponce#DirectionsWaypoint. @LukasPaczos do you have objections?

We can switch the billing controller to use Waypoint but it should ignore silent waypoints, it's only interested in leg waypoints.

Sorry, this always surprises me time and time again that DirectionsResponse#waypoints contains silent waypoints. Since DirectionsResponse#waypoints contains all of regular, silent, and EV waypoints and RouteProgress#remainingWaypoints is already updated to account for EV waypoints, then we shouldn't need to make any changes in the billing controller. Unless I'm missing something @RingerJK?

dzinad · 2022-10-26T19:32:58Z

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

-                    .waypointIndicesList(
-                        getUpdatedWaypointIndicesList(
-                            routeOptions.waypointIndicesList(),
-                            coordinatesList.size - remainingWaypoints - 1


Does this really correspond to nextCoordinateIndex?

dzinad · 2022-10-26T19:35:39Z

if we are speaking about Waypoint.kt - I'll do it 👍

No, I was talking about RouteProgress#remainingWaypoints doc.

The Waypoint class is internal itself so we're good to experiment and change it.

Again, I was talking about RouteProgress.

What fields would you have in mind to add for other types of waypoints?
how do you see customers might choose the particular type of waypoints? For now, they are mostly like passive observer

Requested waypoints and all waypoints? I don't know how they are used now. As far as I understood from the ticket description, the problem is that now in the response we may have more waypoints than in the request. Who knows hot this data will be used by users, why not expose both numbers?

Yeah, I agree, the logic is a bit hard to get in RouteOptionsUpdater. Would it help if I move everything into separate functions?

Nah, not really. Let's leave as is.

RingerJK · 2022-10-27T09:46:20Z

Sorry, this always surprises me time and time again that DirectionsResponse#waypoints contains silent waypoints. Since DirectionsResponse#waypoints contains all of regular, silent, and EV waypoints and RouteProgress#remainingWaypoints is already updated to account for EV waypoints, then we shouldn't need to make any changes in the billing controller. Unless I'm missing something @RingerJK?

I would say that for now, we can keep it as is, and if the Waypoint experiment becomes a success we can switch to that as a single source of truth.

we also need utils methods for us and customers to work with waypoints, as it's always a challenge to handle them right (for instance getNextRequestedCoordinate might be hard to make on the application side)

@LukasPaczos I think it's a typo in the doc, shouldn't be silent, right?

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

Lines 395 to 403 in 934e668

    
               /** 
        
                * Returns a list of [Point]s that mark ends of legs on the route, 
        
                * ignoring origin and silent waypoints. 
        
                */ 
        
               private fun getWaypointsOnRoute(navigationRoute: NavigationRoute): List<Point>? { 
        
                   return navigationRoute.directionsResponse.waypoints()?.drop(1)?.map { 
        
                       it.location() 
        
                   } 
        
               }

LukasPaczos · 2022-10-27T10:31:24Z

I think it's a typo in the doc, shouldn't be silent, right?

Yes 👍

RingerJK · 2022-10-27T15:53:25Z

latest updates:

cut NAVAND-825 to address NAVAND-714 - Added Waypoints to NavigationRoute; - RouteOptionsUpdater refactored; #6005 (comment). Let's discuss if we want to expose it in the ticket
fixed docs in RouteProgress and BillingController;
added skip changelog, as it fails for my changes.

CHANGELOG.md

libnavigation-base/src/main/java/com/mapbox/navigation/base/internal/extensions/WaypointEx.kt

LukasPaczos · 2022-10-28T10:47:04Z

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

+                        }
+                    }
+                )
+                .approachesList(


We can address separately, let's cut a ticket.

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

LukasPaczos

LGTM but it'd be great to add an instrumentation test that verifies a re-route with EV routes.

LukasPaczos · 2022-10-28T13:58:47Z

CHANGELOG.md

@@ -9,6 +9,8 @@ Mapbox welcomes participation and contributions from everyone.
 - Fixed an issue with `NavigationView` that caused road label position to not update in some cases. [#6508](https://github.com/mapbox/mapbox-navigation-android/pull/6508)
 - Fixed an issue where the SDK was trying to send telemetry events when telemetry is switched off globally. [#6512](https://github.com/mapbox/mapbox-navigation-android/pull/6512)

+:warning: `RouteProgress#remainingWaypoints` defines the amount of all remaining waypoints, including explicit (requested with `RouteOptions#coordinatesList`) and implicit (added on the fly, like Electric Vehicle waypoints - [see](https://docs.mapbox.com/api/navigation/directions/#electric-vehicle-routing)) ones.


Suggested change

:warning: `RouteProgress#remainingWaypoints` defines the amount of all remaining waypoints, including explicit (requested with `RouteOptions#coordinatesList`) and implicit (added on the fly, like Electric Vehicle waypoints - [see](https://docs.mapbox.com/api/navigation/directions/#electric-vehicle-routing)) ones.

- Fixed an issue where re-routes could have failed for EV routes. [#6005](https://github.com/mapbox/mapbox-navigation-android/pull/6005)

- :warning: `RouteProgress#remainingWaypoints` defines the amount of all remaining waypoints, including explicit (requested with `RouteOptions#coordinatesList`) and implicit (added on the fly, like Electric Vehicle waypoints - [see](https://docs.mapbox.com/api/navigation/directions/#electric-vehicle-routing)) ones.

we can address separately when creating the changelog cc @Guardiola31337

If we're adding the test above then it'd be good to address this as well.

dzinad · 2022-10-28T14:19:38Z

LGTM but it'd be great to add an instrumentation test that verifies a re-route with EV routes.

Oh, I was going to add the test in https://mapbox.atlassian.net/browse/NAVAND-775. It's about alternatives but there is no basic test. It would be real nice if it appeared. :)

CHANGELOG.md

LukasPaczos

We should be in a good place with the implementation (and unit tests) and this lands only in the alpha but let's prioritize the instrumentation tests right after this.

RingerJK · 2022-10-31T08:53:29Z

We should be in a good place with the implementation (and unit tests) and this lands only in the alpha but let's prioritize the instrumentation tests right after this.

NAVAND-835

RingerJK self-assigned this Jul 4, 2022

RingerJK force-pushed the kyv-directions-route-missing-conditions-and-fix-waypoints-source branch from 1c8ac6d to 4caf1be Compare July 5, 2022 12:14

RingerJK added skip changelog Should not be added into version changelog. ⚠️ DO NOT MERGE PR should not be merged! labels Jul 5, 2022

RingerJK force-pushed the kyv-directions-route-missing-conditions-and-fix-waypoints-source branch from ee11d88 to 1f91ae4 Compare July 13, 2022 10:43

VysotskiVadim reviewed Jul 26, 2022

View reviewed changes

libnavigation-base/src/main/java/com/mapbox/navigation/base/internal/route/Waypoint.kt Outdated Show resolved Hide resolved

RingerJK force-pushed the kyv-directions-route-missing-conditions-and-fix-waypoints-source branch from c2936a0 to fcb5e56 Compare October 24, 2022 10:51

LukasPaczos mentioned this pull request Oct 25, 2022

Don't conflate coordinates and waypoints #6507

Closed

RingerJK force-pushed the kyv-directions-route-missing-conditions-and-fix-waypoints-source branch from fcb5e56 to 1cef0e9 Compare October 25, 2022 10:40

RingerJK removed the ⚠️ DO NOT MERGE PR should not be merged! label Oct 25, 2022

RingerJK commented Oct 25, 2022

View reviewed changes

libnavigation-base/src/main/java/com/mapbox/navigation/base/route/NavigationRoute.kt Outdated Show resolved Hide resolved

RingerJK marked this pull request as ready for review October 25, 2022 12:22

RingerJK requested a review from a team as a code owner October 25, 2022 12:22

RingerJK changed the title ~~- Added Waypoints to NavigationRoute; - RouteOptionsUpdater refactored;~~ NAVAND-714?- Added Waypoints to NavigationRoute; - RouteOptionsUpdater refactored; Oct 25, 2022

RingerJK changed the title ~~NAVAND-714?- Added Waypoints to NavigationRoute; - RouteOptionsUpdater refactored;~~ NAVAND-714 - Added Waypoints to NavigationRoute; - RouteOptionsUpdater refactored; Oct 25, 2022

dzinad reviewed Oct 26, 2022

View reviewed changes

LukasPaczos reviewed Oct 26, 2022

View reviewed changes

LukasPaczos removed the skip changelog Should not be added into version changelog. label Oct 26, 2022

dzinad mentioned this pull request Oct 26, 2022

Support EV route refresh #6511

Merged

RingerJK force-pushed the kyv-directions-route-missing-conditions-and-fix-waypoints-source branch from 1cef0e9 to faa2cc7 Compare October 26, 2022 12:40

LukasPaczos reviewed Oct 26, 2022

View reviewed changes

libnavigation-base/src/main/java/com/mapbox/navigation/base/internal/extensions/WaypointEx.kt Outdated Show resolved Hide resolved

LukasPaczos reviewed Oct 26, 2022

View reviewed changes

dzinad reviewed Oct 26, 2022

View reviewed changes

RingerJK force-pushed the kyv-directions-route-missing-conditions-and-fix-waypoints-source branch from bf165b5 to f3efe77 Compare October 27, 2022 13:49

RingerJK added the skip changelog Should not be added into version changelog. label Oct 27, 2022

RingerJK force-pushed the kyv-directions-route-missing-conditions-and-fix-waypoints-source branch from f3efe77 to ee210c1 Compare October 27, 2022 15:53

RingerJK requested review from VysotskiVadim, LukasPaczos and dzinad October 27, 2022 15:53

RingerJK mentioned this pull request Oct 28, 2022

DirectionsRoute to NavigationRoute refactoring #6516

Draft

dzinad reviewed Oct 28, 2022

View reviewed changes

LukasPaczos reviewed Oct 28, 2022

View reviewed changes

RingerJK force-pushed the kyv-directions-route-missing-conditions-and-fix-waypoints-source branch from 0636da9 to 4fcb206 Compare October 28, 2022 14:39

LukasPaczos reviewed Oct 28, 2022

View reviewed changes

CHANGELOG.md Outdated Show resolved Hide resolved

- Added Waypoints to NavigationRoute; - RouteOptionsUpdater refactored.

8270348

RingerJK force-pushed the kyv-directions-route-missing-conditions-and-fix-waypoints-source branch from 4fcb206 to 8270348 Compare October 28, 2022 14:45

RingerJK added skip changelog Should not be added into version changelog. and removed skip changelog Should not be added into version changelog. labels Oct 28, 2022

RingerJK requested review from LukasPaczos and dzinad October 28, 2022 14:49

LukasPaczos approved these changes Oct 28, 2022

View reviewed changes

RingerJK merged commit a45f7d4 into main Oct 28, 2022

RingerJK deleted the kyv-directions-route-missing-conditions-and-fix-waypoints-source branch October 28, 2022 16:02

dzinad mentioned this pull request Dec 15, 2022

NAVAND-940: fix buildings highlighting #6749

Closed

	val location: Point,
	val directionsWaypoint: DirectionsWaypoint?,

	:warning: `RouteProgress#remainingWaypoints` defines the amount of all remaining waypoints, including explicit (requested with `RouteOptions#coordinatesList`) and implicit (added on the fly, like Electric Vehicle waypoints - [see](https://docs.mapbox.com/api/navigation/directions/#electric-vehicle-routing)) ones.
	- Fixed an issue where re-routes could have failed for EV routes. [#6005](https://github.com/mapbox/mapbox-navigation-android/pull/6005)
	- :warning: `RouteProgress#remainingWaypoints` defines the amount of all remaining waypoints, including explicit (requested with `RouteOptions#coordinatesList`) and implicit (added on the fly, like Electric Vehicle waypoints - [see](https://docs.mapbox.com/api/navigation/directions/#electric-vehicle-routing)) ones.

NAVAND-714 - Added Waypoints to NavigationRoute; - RouteOptionsUpdater refactored; #6005

NAVAND-714 - Added Waypoints to NavigationRoute; - RouteOptionsUpdater refactored; #6005

Conversation

RingerJK commented Jul 4, 2022 • edited Loading

Description

codecov bot commented Jul 13, 2022 • edited Loading

Codecov Report

dzinad commented Oct 25, 2022

RingerJK commented Oct 25, 2022

dzinad commented Oct 26, 2022

dzinad commented Oct 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RingerJK Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

RingerJK commented Oct 26, 2022 • edited Loading

LukasPaczos commented Oct 26, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasPaczos commented Oct 26, 2022 • edited Loading

Choose a reason for hiding this comment

dzinad commented Oct 26, 2022

RingerJK commented Oct 27, 2022

LukasPaczos commented Oct 27, 2022

RingerJK commented Oct 27, 2022 • edited Loading

Choose a reason for hiding this comment

LukasPaczos left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dzinad commented Oct 28, 2022

LukasPaczos left a comment

Choose a reason for hiding this comment

RingerJK commented Oct 31, 2022

RingerJK commented Jul 4, 2022 •

edited

Loading

codecov bot commented Jul 13, 2022 •

edited

Loading

RingerJK Oct 26, 2022 •

edited

Loading

RingerJK commented Oct 26, 2022 •

edited

Loading

LukasPaczos commented Oct 26, 2022 •

edited

Loading

RingerJK commented Oct 27, 2022 •

edited

Loading