-
-
Notifications
You must be signed in to change notification settings - Fork 51
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
Fix snap-to-route crash #106
Conversation
List<RouteLeg> upcomingLegs = routeProgress.directionsRoute().legs() | ||
.subList(routeProgress.legIndex() + 1, routeProgress.directionsRoute().legs().size()); | ||
for (RouteLeg leg : upcomingLegs) { | ||
if (leg.steps() != null && leg.steps().size() >= 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.
Wouldn't it be easier to use a for(i= routeProgress.legIndex() + 1 ; ...)? That way we don't need sublists.
In general since returning null is acceptable, I am not sure if we shouldn't just return null if the next leg has no steps or < 2 steps? But I guess both approaches would be fine as this seems to be an edge case either way.
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.
Wouldn't it be easier to use a for(i= routeProgress.legIndex() + 1 ; ...)? That way we don't need sublists.
Sure. Make sense!
In general since returning null is acceptable, I am not sure if we shouldn't just return null if the next leg has no steps
If we return NULL
the bearing is set to 0
, this means the puck and camera is turn around to zero position, and will turn back for next step.
This also happens on the last position. Maybe we should store the latest bearing and re-use them instead of return a 0
bearing. In this case we don't need the loop-stuff.
What do you think?
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.
How about we use the actual bearing of the location? Or yeah, we could use the last bearing, I think this should be fine for most cases as well.
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 like the idea of fallback to the real location. But this is often not route matched and we have also the issue #63. So I would implement the last bearing usage for now, but I think we can improve this later after some other topics to the 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.
Thanks for working on this. IMHO sublists might be a bit of a resource waster here, as this function is called frequently.
I removed the loop now. If the necessary points not available, the logic is fallback to the last calculated bearing. If them also not available it will fallback to the real location bearing. |
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.
Thanks this change looks good to me 👍
I discovered that legs that contains only a single step will cause a
IndexOutOfBoundsException
crash.Cause was only a wrong if-condition. But to prevent a wrong bearing of
0
, I adjust the fetching of the upcoming step.