-
Notifications
You must be signed in to change notification settings - Fork 319
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.
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
Initial Mapbox Navigator libandroid-navigation integration #1265
Conversation
The current banner instruction setup is tightly coupled with the Java |
re: Let's please carve out a separate ticket for that in order to track the specific work that we need to do |
@devotaaabel and @danesfeder can you two put your heads together and record the work needed for BannerInstructions in a new ticket? |
d271612
to
46b43f1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Some minor comments - I didn't include what we already discussed re thread changes
@@ -8,6 +8,7 @@ buildscript { | |||
google() | |||
jcenter() | |||
maven { url 'https://plugins.gradle.org/m2' } | |||
maven { url 'https://mapbox.bintray.com/mapbox' } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I remove this I get Failed to resolve: com.mapbox.navigator:mapbox-navigation-native:2.0.0
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That's weird. I've tried it locally and it's working 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can try again, maybe it was some sort of caching issue on my end.
private static final int ONE_INDEX = 1; | ||
|
||
private RouteProgress routeProgress; | ||
private final RingBuffer<RouteProgress> previousProgressList = new RingBuffer<>(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Curious 🤔
Is there any reason for using a RingBuffer
here instead of a regular List
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have to be a list? Couldn't there just be a previousRouteProgress
and a currentRouteProgress
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, what @devotaaabel suggested seems like a simpler solution. Especially here that we're only interested in two values.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, I'll remove it and try to reconfigure this - thanks!
|
||
class RouteProcessorRunnable implements Runnable { | ||
|
||
private static final int ONE_SECOND = 1000; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
NIT ONE_SECOND_IN_MILLISECONDS
@@ -250,6 +279,9 @@ public RouteProgress build() { | |||
} | |||
|
|||
public static Builder builder() { | |||
return new AutoValue_RouteProgress.Builder(); | |||
return new AutoValue_RouteProgress.Builder() | |||
.inTunnel(false) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess this is not necessary - by default a boolean
is false
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nah, note that we're using a primitive so it can't be null
Line 161 in be0d05f
public abstract boolean inTunnel(); |
I've tested it locally and didn't get any crashes. Am I missing something?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, makes sense. AutoValue
ends up boxing everything in the object form and checking the properties, getting IllegalStateException
s ("Missing required properties:"
) if not initialized explicitly from our side (builder()
). Thanks for clarifying 🙏
46b43f1
to
1772f38
Compare
0d712a5
to
d2aefc2
Compare
@devotaaabel @Guardiola31337 this is updated per our conversation and review comments |
d2aefc2
to
be0d05f
Compare
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:
If this is correct we will still have thread safety issues. Basically, the way 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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks pretty good! A few comments, also I think we should try to keep PRs a little smaller than this.
...main/java/com/mapbox/services/android/navigation/v5/milestone/VoiceInstructionMilestone.java
Show resolved
Hide resolved
...ion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigation.java
Show resolved
Hide resolved
...tion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigator.java
Outdated
Show resolved
Hide resolved
private static final int ONE_INDEX = 1; | ||
|
||
private RouteProgress routeProgress; | ||
private final RingBuffer<RouteProgress> previousProgressList = new RingBuffer<>(2); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this have to be a list? Couldn't there just be a previousRouteProgress
and a currentRouteProgress
?
...oid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/snap/SnapToRoute.java
Show resolved
Hide resolved
|
||
provider.updateSnapEngine(null); | ||
|
||
assertNotNull(provider.retrieveSnapEngine()); | ||
} | ||
|
||
private NavigationEngineFactory buildNavigationEngineFactory() { | ||
return new NavigationEngineFactory(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this really add any value? If you want to initialize with some default data you can do a faker like BannerComponentsFaker
.
c070769
to
6d6e4da
Compare
.../java/com/mapbox/services/android/navigation/v5/milestone/VoiceInstructionMilestoneTest.java
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This looks good so far @danesfeder 💪
I left some final-minor comments to consider before merging here.
} | ||
|
||
void updateLocation(Location location) { | ||
navigator.updateLocation(buildFixLocationFrom(location)); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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);
}
}
...ion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigation.java
Show resolved
Hide resolved
...tion/src/main/java/com/mapbox/services/android/navigation/v5/navigation/MapboxNavigator.java
Outdated
Show resolved
Hide resolved
final boolean checkFasterRoute = checkFasterRoute(options, rawLocation, routeProgress, engineFactory, userOffRoute); | ||
final List<Milestone> milestones = findTriggeredMilestones(navigation, routeProgress); | ||
|
||
workerHandler.postDelayed(this, ONE_SECOND_IN_MILLISECONDS); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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?
...oid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/snap/SnapToRoute.java
Show resolved
Hide resolved
.../java/com/mapbox/services/android/navigation/v5/milestone/VoiceInstructionMilestoneTest.java
Show resolved
Hide resolved
@@ -250,6 +279,8 @@ public RouteProgress build() { | |||
} | |||
|
|||
public static Builder builder() { | |||
return new AutoValue_RouteProgress.Builder(); | |||
return new AutoValue_RouteProgress.Builder() | |||
.currentAnnouncement(EMPTY_ANNOUNCEMENT) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
6d6e4da
to
ab0335f
Compare
Thanks for the continuous improvements 👍. I had a look at this PR and tried to understand it. I was wondering what the |
@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? |
Thanks for the explanation @mcwhittemore. A description of the lib and and API documentation would be great 👍. |
I just thought about this a bit more and was wondering if you could share more details about the plan of the Are there any plans to open source |
@boldtrn - sure. The goal is to merge There is some question about if Thanks for the questions! I hope this helps clear things up a bit. |
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 :) ? |
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:
Banner instructions> Update RouteProgress and BannerInstructionMilestone for banner date from Navigator #1271@Guardiola31337 @devotaaabel we need to look into how we test
Navigator
and all of it's related data (FixLocation
,NavigationStatus
, etc.) - they arefinal
, so Mockito does not like them.cc @akitchen @taraniduncan @mcwhittemore