Introduce reset function for the navigator

[kmadsen]

Description

Opening this as a draft while working on examples that test out the function.

We are looking to add the ability to teleport the navigator to different locations. For example, simulate a route in new york and then jump to tokyo and simulate a new route - all while keeping the same SDK setup.

A more practical case, would be to simulate a route in your city. And then jump back to the beginning to drive it for real.

• 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

Testing

Notice that without the reset function, the navigator will not reliably reset to the new location selected.

without reset function	with reset function

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
• We might need to update / push api/current.txt files after running $> make core-update-api (Core) / $> make ui-update-api (UI) if there are changes / errors we're 🆗 with (e.g. AddedMethod changes are marked as errors but don't break SemVer) 🚀 If there are SemVer breaking changes add the SEMVER label. See Metalava docs

[kmadsen]

this line is what causes the difference between the videos in the PR description.

The navigator needs to be reset, and then it can start from a new location.

[kmadsen]

Updating the example to use startActivityForResult to showcase the resetTripSession function.

When we start an activity for result, the current activity is never destroyed. Making it so, when the onActivityResult comes back it will be able to update the state of MapboxNavigation.

Before we had the resetTripSession function, it required us to call MapboxNavigation.onDestroy first

[kmadsen]

Digging up some history as we had a similar function in the past.

Introduced a reset function, that would recreate the navigator #2850
That caused a regression in the examples #2769
Fixed the regression with #2905

Notice, the issue was because the reset function was called inside MapboxTripSession.reset()

But we're not calling it here

Worth noting that, our MapboxTripSession private reset function is very different from the Nav Native's resetRideSession.

[kmadsen]

I've also tested the repro steps included in the issue from before: #2769 (comment)

And this change is looking good. It's because we don't want to add the Navigator.resetRideSession call to the MapboxTripSession.reset() function

[kmadsen]

directionsSession is supposed to update tripsSession.route

so our real source of truth is the directionsSession

it's a bit more complicated to wire up tests to have this assumption built in

[LukasPaczos]

Suggested change

	fun reset()
	fun resetRideSession()

Let's call it the same as the native API.

[LukasPaczos]

Do we need to reset the route as well? I'd expect this method to only reset the Navigator's bias (we can work on the method naming as well) and nothing else.

[Guardiola31337]

While reviewing this noticed that we were not resetting the directionsSession.routes as part of onDestroy which allows you to do

mapboxNavigation.onDestroy()
mapboxNavigation.getRoutes()

We considered adding it here which is called when in onDestroy. If we opt to remove it, we should add it after directionsSession.unregisterAllRoutesObservers() when in onDestroy (we can do it in a separate PR though as it doesn't directly relate to the changes added as part of this PR).

cc @kmadsen

[kmadsen]

We agreed in comments that native was going to reset the route, and the SDK callers would need to set route again if they wanted to keep it. But that is actually, not true.

void NavigatorImpl::resetRideSession() {
  BuildRideSessionImpl();
  if (routes_ && ride_session_) {
    try {
      ride_session_->SetRoutes(routes_);
    } catch (const std::exception& ex) {
      LOG_ERROR(std::string{"SetRoute error: "} + ex.what());
    }
  }

cc: @mskurydin @SiarheiFedartsou @averkhaturau

I think @LukasPaczos is right, we don't want to reset the route inside this resetTripSession function. So am moving the route reset back to onDestroy

[LukasPaczos]

LGTM

[Guardiola31337]

👍

[kmadsen]

robo tests ran out of memory. wonder if there is a memory leak

java.lang.OutOfMemoryError: Could not allocate JNI Env
     FATAL EXCEPTION: main
Process: com.mapbox.navigation.examples, PID: 13039
java.lang.OutOfMemoryError: Could not allocate JNI Env
	at java.lang.Thread.nativeCreate(Native Method)
	at java.lang.Thread.start(Thread.java:1063)
	at java.util.concurrent.ThreadPoolExecutor.addWorker(ThreadPoolExecutor.java:921)
	at java.util.concurrent.ThreadPoolExecutor.execute(ThreadPoolExecutor.java:1339)
	at f.f0$a.m()
	at f.s.h()
	at f.s.a()
	at f.f0.s()
	at h.n.E()
	at h.i$b.E()
	at d.b.c.a.a.i.c()
	at d.b.f.e.a.a.a.getRoute()
	at com.mapbox.navigation.route.internal.hybrid.MapboxHybridRouter$d.e()
	at com.mapbox.navigation.route.internal.hybrid.MapboxHybridRouter$d.getRoute()
	at com.mapbox.navigation.route.internal.hybrid.MapboxHybridRouter.getRoute()
	at d.b.f.b.l.a.b.a()
	at d.b.f.b.p.a.i()
	at d.b.f.b.p.a.b()
	at d.b.f.b.a.G()
	at d.b.f.b.a.c()
	at d.b.f.b.a$d.t()
	at d.b.f.b.t.a.g.V()
	at d.b.f.b.t.a.g.J()
	at d.b.f.b.t.a.g$f.l()
	at e.v.k.a.a.i()
	at kotlinx.coroutines.x0.run()
	at android.os.Handler.handleCallback(Handler.java:746)
	at android.os.Handler.dispatchMessage(Handler.java:95)
	at android.os.Looper.loop(Looper.java:148)
	at android.app.ActivityThread.main(ActivityThread.java:5443)
	at java.lang.reflect.Method.invoke(Native Method)
	at com.android.internal.os.ZygoteInit$MethodAndArgsCaller.run(ZygoteInit.java:728)
	at com.android.internal.os.ZygoteInit.main(ZygoteInit.java:618)

[kmadsen]

Still a pain to deobfuscate stack traces, here's the instructions
mapping.txt

But the actual crash is coming from the requesting a route from HTTP

com.mapbox.navigation.route.internal.offboard.MapboxOffboardRouter$getRoute$1 -> d.b.f.e.a.a.a$b:
    com.mapbox.navigation.base.route.Router$Callback $callback -> a
    void onResponse(retrofit2.Call,retrofit2.Response) -> a
    void onFailure(retrofit2.Call,java.lang.Throwable) -> b

okhttp3.RealCall$AsyncCall -> f.f0$a:
    okhttp3.Callback responseCallback -> f
    java.util.concurrent.atomic.AtomicInteger callsPerHost -> g
    okhttp3.RealCall this$0 -> h
    void execute() -> k
    java.util.concurrent.atomic.AtomicInteger callsPerHost() -> l
    void executeOn(java.util.concurrent.ExecutorService) -> m

Syncing with @averkhaturau. We don't believe this is due to this change at this time. The crash is coming from the network call rather than directly from nav-native.

I'll run one more test before merging

[kmadsen]

tl;dr - I'm not finding any reason to believe this pull request is introducing an issue

After running some tests locally, not able to identify a leak from requesting routes. But the "Time to request a route" is very slow - I was able to reproduce this on master and it was possibly due to my own network quality.