Initial Mapbox Navigator libandroid-navigation integration by danesfeder · Pull Request #1265 · mapbox/mapbox-navigation-android

danesfeder · 2018-09-05T21:12:18Z

This is the initial integration effort for Mapbox Navigator.

This PR aims to replace the client side snap-to-route and off-route detection.

TODO:

@Guardiola31337 @devotaaabel we need to look into how we test Navigator and all of it's related data (FixLocation, NavigationStatus, etc.) - they are final, so Mockito does not like them.

cc @akitchen @taraniduncan @mcwhittemore

danesfeder · 2018-09-06T13:59:09Z

~~Banner instructions~~

The current banner instruction setup is tightly coupled with the Java BannerInstructions object. This object is not what's returned by Navigator, meaning we would either need to convert the object or create some sort of wrapper. Regardless, this is not going to be as straightforward as the voice instruction update here. So I vote we break this out into a separate PR.

akitchen · 2018-09-06T17:42:31Z

re:
#1265 (comment)

Let's please carve out a separate ticket for that in order to track the specific work that we need to do

akitchen · 2018-09-06T17:43:33Z

@devotaaabel and @danesfeder can you two put your heads together and record the work needed for BannerInstructions in a new ticket?

danesfeder · 2018-09-06T17:49:38Z

@akitchen sure thing #1271

Guardiola31337

Some minor comments - I didn't include what we already discussed re thread changes

Guardiola31337 · 2018-09-14T10:06:04Z

    google()
    jcenter()
    maven { url 'https://plugins.gradle.org/m2' }
+    maven { url 'https://mapbox.bintray.com/mapbox' }


I guess this is not needed. The buildScript block determines which plugins, task classes, and other classes are available for use in the rest of the build script and at the moment maven { url 'https://mapbox.bintray.com/mapbox' } repository is not exposing any of those.

When I remove this I get Failed to resolve: com.mapbox.navigator:mapbox-navigation-native:2.0.0

That's weird. I've tried it locally and it's working 🤔

I can try again, maybe it was some sort of caching issue on my end.

Guardiola31337 · 2018-09-14T10:30:52Z

  private static final int ONE_INDEX = 1;
-
-  private RouteProgress routeProgress;
+  private final RingBuffer<RouteProgress> previousProgressList = new RingBuffer<>(2);


Curious 🤔
Is there any reason for using a RingBuffer here instead of a regular List?

@Guardiola31337 I'm aiming for a first-in first-out queue with a fixed size that replaces its oldest element if full - can we achieve that with a List here?

Does this have to be a list? Couldn't there just be a previousRouteProgress and a currentRouteProgress?

Yeah, what @devotaaabel suggested seems like a simpler solution. Especially here that we're only interested in two values.

Cool, I'll remove it and try to reconfigure this - thanks!

Guardiola31337 · 2018-09-14T10:56:42Z

+
+class RouteProcessorRunnable implements Runnable {
+
+  private static final int ONE_SECOND = 1000;


NIT ONE_SECOND_IN_MILLISECONDS

Guardiola31337 · 2018-09-14T11:27:57Z

  public static Builder builder() {
-    return new AutoValue_RouteProgress.Builder();
+    return new AutoValue_RouteProgress.Builder()
+      .inTunnel(false)


I guess this is not necessary - by default a boolean is false.

Yeah I understand the default is false - I added it because we aren't marking it as NonNull so AutoValue will throw an exception if we try to build a RouteProgress without the value in the builder.

Nah, note that we're using a primitive so it can't be null

mapbox-navigation-android/libandroid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/routeprogress/RouteProgress.java

Line 161 in be0d05f

public abstract boolean inTunnel();

I've tested it locally and didn't get any crashes. Am I missing something?

@Guardiola31337 did you run the unit tests without it? In the SDK, it should always be set in the processor, so that makes sense why you didn't see any crashes. I can remove it and fix up the data in the tests - I guess we should be accounting for a crash in our unit tests with changes to production code.

Yeah, makes sense. AutoValue ends up boxing everything in the object form and checking the properties, getting IllegalStateExceptions ("Missing required properties:") if not initialized explicitly from our side (builder()). Thanks for clarifying 🙏

danesfeder · 2018-09-14T17:24:42Z

@devotaaabel @Guardiola31337 this is updated per our conversation and review comments

kevinkreiser · 2018-09-14T19:58:01Z

hi @danesfeder @devotaaabel @Guardiola31337 i wanted to make sure i understood properly from earlier. Is this correct? Currently we have two threads using the native object:

main activity calls getRoute and setRoute
background thread calls updateLocation and getStatus

If this is correct we will still have thread safety issues. Basically, the way getStatus works is that it accesses portions of data on the currently loaded route. If you call setRoute (the navigator calls this setDirections) while you are calling getStatus on another thread you could get a crash!

We will be working on the native side to make the navigator thread safe so you dont have to worry about this but in the mean time i would make sure not to call any of the methods from different threads without synchronization.

devotaaabel

Looks pretty good! A few comments, also I think we should try to keep PRs a little smaller than this.

devotaaabel · 2018-09-14T21:07:08Z

  private static final int ONE_INDEX = 1;
-
-  private RouteProgress routeProgress;
+  private final RingBuffer<RouteProgress> previousProgressList = new RingBuffer<>(2);


Does this have to be a list? Couldn't there just be a previousRouteProgress and a currentRouteProgress?

devotaaabel · 2018-09-14T21:37:48Z

  }
+
+  private NavigationEngineFactory buildNavigationEngineFactory() {
+    return new NavigationEngineFactory();


Does this really add any value? If you want to initialize with some default data you can do a faker like BannerComponentsFaker.

…cation

Guardiola31337

This looks good so far @danesfeder 💪

I left some final-minor comments to consider before merging here.

Guardiola31337 · 2018-09-18T08:25:39Z

+  }
+
+  void updateLocation(Location location) {
+    navigator.updateLocation(buildFixLocationFrom(location));


We need to synchronize navigator.updateLocation too (basically all Navigator calls need to be thread-safe). In this case as buildFixLocationFrom is only creating a FixLocation doesn't need to be synchronized (that's us i.e. we're not accessing the Navigator) so we could synchronized only navigator.updateLocation call as follows 👀

void updateLocation(Location location) { FixLocation fixedLocation = buildFixLocationFrom(location); synchronized (this) { navigator.updateLocation(fixedLocation); } }

Guardiola31337 · 2018-09-18T08:50:37Z

+    final boolean checkFasterRoute = checkFasterRoute(options, rawLocation, routeProgress, engineFactory, userOffRoute);
+    final List<Milestone> milestones = findTriggeredMilestones(navigation, routeProgress);
+
+    workerHandler.postDelayed(this, ONE_SECOND_IN_MILLISECONDS);


NIT - I guess "conceptually" it'd be clear if we do this at the end of the process i.e. "process" everything / run all the calculations and finally schedule the next run. What do you think?

Guardiola31337 · 2018-09-18T09:11:01Z

  public static Builder builder() {
-    return new AutoValue_RouteProgress.Builder();
+    return new AutoValue_RouteProgress.Builder()
+      .currentAnnouncement(EMPTY_ANNOUNCEMENT)


Saw that you removed the explicitly inTunnel initialization here. That would mean that there's no a false inTunnel default value anymore (making it a "required" property) so clients using RouteProgress will need to initialize it explicitly (same as we did with these latest changes) if they don't want to get a IllegalStateException. Is that what we want? I'm down with either but I wanted to confirm that we were on the same page.

danesfeder · 2018-09-18T13:50:03Z

This is updated - merging to navigation-native

boldtrn · 2018-09-21T10:16:26Z

Thanks for the continuous improvements 👍. I had a look at this PR and tried to understand it. I was wondering what the Navigator is and how it works? Is it open source? I couldn't find it on Github.

mcwhittemore · 2018-09-21T15:31:53Z

@boldtrn - Mapbox Navigator is a C++ lib that route following and offline navigation. The project is closed source. We've talked internally a few times recently about creating a landing page for this lib to outline the features and provide its API documentation so that contributors to our open source projects can use it. Would that be helpful for you?

boldtrn · 2018-09-24T12:16:47Z

Thanks for the explanation @mcwhittemore. A description of the lib and and API documentation would be great 👍.

boldtrn · 2018-09-24T13:58:42Z

I just thought about this a bit more and was wondering if you could share more details about the plan of the nav-native branch in general? Is it expected to be merged into master at some point or is this an independent project?

Are there any plans to open source Mapbox Navigator in a later stage? I really endorse the open innovation approach of Mapbox.

mcwhittemore · 2018-09-25T15:20:20Z

@boldtrn - sure.

The goal is to merge nav-native into master. This will let Android and iOS share core route following, location filtering and puck projection logic. It will also allow for offline routing.

There is some question about if nav-native becomes open source or not. For the time being the plan is that it will be private, close source. This is because it brings our core routing logic with it. If/When we find a good way to decouple the core navigation logic and the offline routing I think we'll be able to open up at least some of this code.

Thanks for the questions! I hope this helps clear things up a bit.

karussell · 2018-09-27T07:41:49Z

Thanks a lot @danesfeder & others for pushing forward such an amazing open source library. We (as GraphHopper) appreciate that Mapbox invests into the open source community.

@mcwhittemore if this is merged into master this would be an unusual choice in an open source library and could create lots of uncertainty like e.g. the license and rights. Why not add this library for commercial distribution later in the pipeline and use an open source replacement with the functionality from today? For open source "consumers" the value of open source is not only the functionality per se but also that you are able to fix bugs, you can trust it (e.g. no tracking) and it has a clear license how to handle distribution, modification etc. And because of this the "provider" of open source more likely gets contributions, mentions and fixes :).

This will be all unclear with a closed source library. I'm not saying that you should open source this native library but to reduce the chance of forks and other hassle you should really think about how to stay open source in master. We are open to contributions btw.

What can we do that you re-evaluate your current strategy :) ?

danesfeder added ⚠️ DO NOT MERGE PR should not be merged! refactor labels Sep 5, 2018

danesfeder added this to the 0.19.0 milestone Sep 5, 2018

danesfeder self-assigned this Sep 5, 2018

danesfeder requested review from Guardiola31337 and devotaaabel September 5, 2018 21:12

danesfeder changed the title ~~Mapbox Navigator libandroid-navigation integration~~ Initial Mapbox Navigator libandroid-navigation integration Sep 5, 2018

danesfeder changed the base branch from master to navigation-native September 6, 2018 16:30

danesfeder mentioned this pull request Sep 6, 2018

Update RouteProgress and BannerInstructionMilestone for banner date from Navigator #1271

Closed

akitchen mentioned this pull request Sep 7, 2018

User should see themselves progress through tunnel when GPS is weak or nonexistent #835

Closed

6 tasks

danesfeder force-pushed the nav-native branch 4 times, most recently from d271612 to 46b43f1 Compare September 13, 2018 16:17

Guardiola31337 reviewed Sep 14, 2018

View reviewed changes

danesfeder force-pushed the nav-native branch from 46b43f1 to 1772f38 Compare September 14, 2018 17:16

danesfeder added ✓ ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Sep 14, 2018

danesfeder force-pushed the nav-native branch 2 times, most recently from 0d712a5 to d2aefc2 Compare September 14, 2018 17:21

danesfeder force-pushed the nav-native branch from d2aefc2 to be0d05f Compare September 14, 2018 17:25

devotaaabel reviewed Sep 14, 2018

View reviewed changes

danesfeder added 5 commits September 17, 2018 14:29

Nav Native initial setup

4f9ce38

Integrate more of NavigationStatus

fa5b953

Update for polling NavigationStatus system instead of triggered by Lo…

1b325a5

…cation

Add tunnel flag

0b49379

Use runnable Date for snapped location / off route

26995b6

danesfeder force-pushed the nav-native branch 2 times, most recently from c070769 to 6d6e4da Compare September 17, 2018 19:30

devotaaabel approved these changes Sep 17, 2018

View reviewed changes

Comment thread .../java/com/mapbox/services/android/navigation/v5/milestone/VoiceInstructionMilestoneTest.java

Guardiola31337 reviewed Sep 18, 2018

View reviewed changes

Add voice instruction from NavigationStatus

ab0335f

danesfeder force-pushed the nav-native branch from 6d6e4da to ab0335f Compare September 18, 2018 13:29

danesfeder merged commit 40dec59 into navigation-native Sep 18, 2018

danesfeder deleted the nav-native branch September 18, 2018 13:50

danesfeder restored the nav-native branch September 18, 2018 16:19

akitchen removed this from the 0.19.0 milestone Sep 25, 2018

karussell mentioned this pull request Oct 4, 2018

License of closed source library libandroid-navigation #1391

Closed

Guardiola31337 mentioned this pull request Feb 4, 2019

Navigator Module #1719

Closed

Guardiola31337 mentioned this pull request Jul 22, 2019

Navigator Module changelog #1943

Closed


		class RouteProcessorRunnable implements Runnable {

		private static final int ONE_SECOND = 1000;

Conversation

danesfeder commented Sep 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danesfeder commented Sep 6, 2018

Uh oh!

akitchen commented Sep 6, 2018

Uh oh!

akitchen commented Sep 6, 2018

Uh oh!

danesfeder commented Sep 6, 2018

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danesfeder commented Sep 14, 2018

Uh oh!

kevinkreiser commented Sep 14, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

devotaaabel left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

danesfeder commented Sep 18, 2018

Uh oh!

boldtrn commented Sep 21, 2018

Uh oh!

mcwhittemore commented Sep 21, 2018

danesfeder commented Sep 5, 2018 •

edited

Loading

kevinkreiser commented Sep 14, 2018 •

edited

Loading