Skip to content
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

reset step and leg indexes when reroute occurs #52

Merged
merged 1 commit into from
May 23, 2017
Merged

Conversation

cammace
Copy link
Contributor

@cammace cammace commented May 23, 2017

Closes #48

rerouting wouldn't reset the index counts which caused off-route to measure the distance to the wrong step.

@cammace cammace self-assigned this May 23, 2017
@cammace cammace requested a review from zugaldia May 23, 2017 18:00
@cammace cammace merged commit 5949fd0 into master May 23, 2017
@cammace cammace deleted the cam-48-fix branch May 23, 2017 23:43
@cammace cammace added this to the v0.3.0 milestone May 23, 2017
@Grsmto
Copy link
Contributor

Grsmto commented May 24, 2017

Hey @cammace !
Since this has been merged, it seems like the userOffRoute callback is called falsely. Wondering if you're aware of this or not!
RouteUtils.isOffRoute() is returning false but still, callback is triggered.
Also it's called multiple times in a short interval (which from what I read in the code shouldn't happen). I can't figure out why.
I can open an issue/provide more details if needed.
Edit: it seems to be happening just when there is a manoeuvre, which make me believe it comes from this PR.
Edit2: I'm using the MockLocationEngine to develop (I updated it as in this PR).

@cammace
Copy link
Contributor Author

cammace commented May 25, 2017

Hey there @Grsmto, thanks for reporting this. To clarify, you're using the SNAPSHOT build or master? I've actually been removing the RouteUtils methods from the nav SDK and instead directly adding the logic.

How exactly is the MockLocationEngine being used? Unless you are forcing it to go off-route, the callback should never be invoked.

@Grsmto
Copy link
Contributor

Grsmto commented May 25, 2017

Hey! I'm using the SNAPSHOT build afaik (com.mapbox.mapboxsdk:mapbox-android-navigation:0.3.0-SNAPSHOT) and I checked I think I have the changes of this PR in my code.
About my usage of MockLocationEngine I basically followed the implementation of the NavigationActivity.java of the repo (https://github.com/mapbox/mapbox-navigation-android/blob/master/navigation/app/src/main/java/com/mapbox/services/android/navigation/testapp/activity/NavigationActivity.java). It even happens when I disable noise. I'll try to dig more into it maybe it's my implementations that is wrong. But I had the feeling that this happened "suddenly" without code changes, which made me think a dependency had changed (and I saw this PR and some code related hence my deduction, couldn't figure out deeper).
I'll try to isolate the bug to make sure it's not my implementation.
Thanks!

Edit: Ok I forked the project, I'll setup an activity with my implementation and let you know when I reproduce the issue in that environment.

@Grsmto
Copy link
Contributor

Grsmto commented May 25, 2017

Hi @cammace !
I setup a test as promised on my fork: https://github.com/Grsmto/mapbox-navigation-android/tree/offroute-bug-test
I actually figured out that the NavigationActivity was missing the navigation.addOffRouteListener() that's probably why you didn't figure out it was happening :)
I also added a navigation.setSnapToRoute(true); test case as well, cause it seems like it's not working neither.
Hope this help, thanks!
Edit: Checkout the NavigationActivity view. You'll have to be a bit patient sometimes it goes off-route after a few manoeuvres.
Edit2: Just tested now, if I revert the code of this PR, everything works fine (off route and snap point!).

@cammace
Copy link
Contributor Author

cammace commented May 25, 2017

I'll take a look at your test. I haven't been testing offroute with the NavigationActivity but rather the RerouteActivity which seems to be working fine on my end. By default, snapping is set to true but you have to use the location object the callbacks provide to get the snapped location, otherwise, it won't work.

@cammace
Copy link
Contributor Author

cammace commented May 25, 2017

I have fixed the index issue in #56, thanks for reporting and providing a sample to reproduce. The reason snapping to the route isn't working is actually shown in your example, only the location object provided inside OnProgress and AlertLevel are the snapped positions. The Map SDK still gets the true user location. The location plugin in mapbox/mapbox-plugins-android#22 will fix this :)

If you are still running into issues, could you open a ticket so I can look further into it?

@Grsmto
Copy link
Contributor

Grsmto commented May 25, 2017

Hi, awesome!
About the snapped position, I'm not sure cause I did use the provided location object from OnProgress as you can see on this line.
I'll try again and open a separate issue if I keep having the issue.

@cammace cammace mentioned this pull request Jun 5, 2017
10 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants