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

Update vanishing point calculation with a location based implementation #2737

Merged
merged 15 commits into from
Dec 5, 2020

Conversation

ShanMa1991
Copy link
Contributor

@ShanMa1991 ShanMa1991 commented Nov 12, 2020

Description

This pr is to solve the inconsistency of route progress, puck and vanishing route line during the navigation, when speed is fast or when zooming in. This is pr is mainly following the the Android implementation as #3422 and 3661.

  • fix 647
  • fix the constant gap between the puck and the vanishing route line point
  • change the Changelog
  • update the calculation of the fractionTraveled calculation method to be synchronized with puck

Implementation

Instead of using the sum of previous route steps’ distance as the traveled distance, and calculating the fractionTraveled based on the distance fraction, this pr stores the coordinates and route line index along the route. The distance is calculated by the coordinates instead of the line string. The fractionTraveled is based on the route line index and the distance, which improve the accuracy. It also keeps the location and fractionTraveled of route line synchronized with puck to avoid the inconsistency.

Add a NSTimer (Timer) to update the vanishing route line, slicing the fractionTraveled into small pieces and updating each of them during a small time interval. It allows the smoothly transition during each 1 second update of the route progress.

Screenshots or Gifs

From the .gif below, we could see that the vanishing route line is at the same location of the puck view, and also move smoothly when zoom in and at high speed

fixGif

/cc @mapbox/navigation-ios

@ShanMa1991 ShanMa1991 added UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. iOS labels Nov 12, 2020
@ShanMa1991 ShanMa1991 added this to the v1.2.0 milestone Nov 12, 2020
@ShanMa1991 ShanMa1991 self-assigned this Nov 12, 2020
@ShanMa1991
Copy link
Contributor Author

The problem so far mainly comes from two parts during navigation tests.

First, the method of line-slice-along methods defined in Turf.js used in Android team is different from LineString.trimmed(from:distance:) in Turf-swift. The Turf-swift method asks for a location and a distance as parameter, while the Turf.js asks for two distance as parameter. It caused some problems in dealing with the overlapping coordinates points and raise the index problem. Especially in the updateUpcomingRoutePointIndex(routeProgress:), it would fail to update the primaryRouteRemainingDistancesIndex correctly due to the index problem. I'll keep working on the bug fixing to solve this problem.

Second, for the routeProgress update, the NavigationMapView should receive the notification of the routeProgress change to re-calculate the primaryRouteRemainingDistancesIndex and then the fractionTraveled based on the coordinates and the location. I'll keep working on this part to keep track of the routeProgress and the calculation.

@ShanMa1991 ShanMa1991 force-pushed the shan-components-consistency-647 branch from 65e2b36 to 88128c9 Compare November 17, 2020 21:53
@ShanMa1991 ShanMa1991 marked this pull request as ready for review November 17, 2020 21:57
@ShanMa1991
Copy link
Contributor Author

The constant location gap between the vanishing route line point and the puck may be caused by the calling of the func updateUpcomingRoutePointIndex(routeProgress: ) and the calling of the func updateTraveledRouteLine(point:). They are called after the updating the routeProgress and the puck location. It may have a sequence problem and the caller problem during the navigation. I'll keep working on that to fix it.

@cafesilencio
Copy link

One thing I would suggest is looking at MapRouteLineTest.kt on the Android side and using the unit tests against your implementation to see if you get the same results. The routes we test with can be found here.

@ShanMa1991
Copy link
Contributor Author

After updating the callers to receive the routeProgress, tested it with really long route simulation and different, The vanishing route line is synchronized with puck. And there're are some improvements to do:

  1. The simulated location manager provides routeProgress with location update every 1 second. The updateRoute with vanishing route line update is every 1 second as well. So the end point of the vanishing routeline is jumping every 1 second from A to B. While the puck view is smoothly animated transition from A to B during this 1 second. So from vision effect, that the puck view is driving and sliding but the route line is jumping during each 1 second.

To solve this problem, the vanishing route line could add the animating effect to allow the smoothly transition during the 1 second, similar toas the puck view.

  1. Add the tests similar to MapRouteLineTest.kt , to check the correctness of the fractionTraveled

@LukasPaczos
Copy link

To solve this problem, the vanishing route line could add the animating effect to allow the smoothly transition during the 1 second

Is the animation of the puck constant and completely predictable? On the Android side, the puck's transition animation is variable, so we opted to expose a listener from the Maps SDK that's invoked whenever the transition animation of the puck ticks. On each of those ticks, we can precisely update the vanishing point.

@cafesilencio
Copy link

To add to what Lukas has written, on the Android side we add a listener called OnIndicatorPositionChangedListener to the map location component. This was recently added to the Maps SDK. I assume it was added on the IOS version. See also here. The call is consumed here.

During the puck's animation this is called with every point the puck is moving to. It is called many times per second. So the vanishing point adjusts a very small amount along with the puck's movements. I haven't had a chance to review your code yet so maybe you're already implementing this. If so, disregard this. But in case it's not clear how to get a smooth vanishing point transition, this is how it's done.

@ShanMa1991
Copy link
Contributor Author

Thanks for the suggestion above! I'll try the methods as provided to see how to get a smooth vanishing point transition synchronized with the puck animation.

Copy link
Contributor

@MaximAlien MaximAlien left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR. Please check initial iteration of my comments. As discussed earlier let's concentrate of following tasks:

  1. Since after updating calculation algorithm user course view is in sync with vanishing route line let's try to cover implemented functionality with tests (minor fixes).
  2. NavigationMapView becomes pretty large, do you think we can extract added functionality into separate entity or NavigationMapView extension?
  3. Find out whether Maps SDK for iOS provides callback similar to onIndicatorPositionChanged as on Android.

MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
/**
Calculates the distance between 2 points using [EPSG:3857 projection](https://epsg.io/3857).
*/
private func calculateDistance(point1: CLLocationCoordinate2D, point2: CLLocationCoordinate2D) -> Double {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think these calculation methods should be tested either directly of indirectly.

MapboxNavigation/RouteMapViewController.swift Outdated Show resolved Hide resolved
@ShanMa1991
Copy link
Contributor Author

Right now the func updateRoute(_ routeProgress:) only receive route progress every one second, and it limited the vanishing line update in frequency. I'll use the func updateTraveledRouteLine(point:) to receive location change without time constraint and update route line. The func updateUpcomingRoutePointIndex(routeProgress:) will be responsible to receive route progress change every second to provide the primaryRouteRemainingDistancesIndex whenever it's needed to be used in calculating fractionTraveled. So in this way, the route line layer could receive updates from location without the 1 second frequency constraint.

After that, we could work on providing the location change to the update the vanishing route line. No matter from the puck view location, or from the animation and frame rate.

@ShanMa1991
Copy link
Contributor Author

Because the MapSDK on iOS side doesn't have the onIndicatorPositionChanged as on Android. So during the 1 second, the func updateRoute() could not be triggered at a higher frequency as the Android to update the routeLayer.lineGradient. So in the next step, to keep the puck and the vanishing route line synced during the 1 second when zoom in, I'll try increase the frame rate to update the vanishing route line at a similar frequency as the puck, and use the animation rate of the puck to sliced the fractionTraveled into small pieces to update the routeLayer.lineGradient gradually. Because the animation of the puck is trackable with a constant rate.

Besides, in the NavigationMapViewTests, it still needs a correct and expected fractionTraveled to testify the calculated result.

@ShanMa1991
Copy link
Contributor Author

The NSTimer could not repeat the function at a constant time interval and may call the action multiple times. It cause the lineGradient calculation problem, which is based on even distributed time interval. I'll work on to fix this problem.

@ShanMa1991 ShanMa1991 force-pushed the shan-components-consistency-647 branch from c4b6af4 to bccb8d5 Compare December 1, 2020 19:02
@ShanMa1991
Copy link
Contributor Author

Now the problem is that the user puck view is updated evenly during every second, but the vanishing route line is updated with the Timer. But the Timer called the time interval not at an evenly distributed rate. So the changing puck view and the changing vanishing route line is still not synchronous when zoom pretty in.

So the next step is to update the Timer event at a constant time interval to allow the two synced. Maybe try CADisplayLink

@ShanMa1991
Copy link
Contributor Author

By updating the Timer, the vanishing route line bow could implement the smoothly transition as above above . But because the time interval of the Timer is not evenly distributed, when zoom pretty in, the puck view and the vanishing route line may still sometimes not at the same location. And the route line casing layer also sometimes has a flash under layer.

MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
MapboxNavigationTests/Fixtures/vanish_point_test.json Outdated Show resolved Hide resolved
MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
MapboxNavigation/NavigationMapView.swift Outdated Show resolved Hide resolved
@ShanMa1991 ShanMa1991 force-pushed the shan-components-consistency-647 branch from 4fdcbac to 39195a9 Compare December 3, 2020 18:18
@MaximAlien
Copy link
Contributor

You're making good progress. There are still some items I think it'd be good to take care of in scope of this PR if possible:

  1. Right after starting navigation puck and route line are not in perfect sync:
    2

  2. It seems that there is regression in route refresh (I'm not 100% sure that this PR causes this, but we should attempt to fix it). To increase route refresh updates you can set RouteControllerProactiveReroutingInterval = 10.0 before starting navigation.
    1

Comment on lines 110 to 114
if coordinates.isEmpty {
return nil
} else {
return calculateGranularDistances(coordinates)
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What about more traditional:

Suggested change
if coordinates.isEmpty {
return nil
} else {
return calculateGranularDistances(coordinates)
}
if !coordinates.isEmpty {
return calculateGranularDistances(coordinates)
}
return nil

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion, I'll fix it

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the suggestion. At the start of the navigation, because the fractionTraveled hasn't be updated, there's no available lineGradient to update. I think it can be solved by a fixer to use the routeProgress.fractionTraveled instead or use speed to update the lineGradient. I'll work on this problem to fix it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The refresh problem and the start of the navigation problem are both caused by the navigationMapView.show(_ routes: [Route], legIndex: Int = 0). inside the function, it defines the mainRouteLayer and parentLayer with a lineGradient that forms from a 0.0 fractionTraveled.

So at the start of the navigation, before it receives the routeProgress to call func updateRoute(_ routeProgress:), it shows the vanishing route line with the stop at the start point. It causes the gap between the puck view and not perfectly synced.

When refresh, the func navigationService(_ service: NavigationService, didRefresh routeProgress: RouteProgress) also call the navigationMapView.show(_ routes: [Route], legIndex: Int = 0) first to make the vanishing route line stop at the start point. Then it calls the func updateRoute(_ routeProgress:) to update the vanishing route line.

To fix the problem, inside the navigationMapView.show(_ routes: [Route], legIndex: Int = 0), it should replace the default 0.0 value with the fractionTraveled. And prepare a resonnable value of the fractionTraveled to be used.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I create a new ticket to handle this problem in #2757

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In the new commit, the refresh problem is solved with using the old fractionTraveled to form the two layers for vanishing route line. Then it receive updates from route progress, and slight change from the old fractionTraveled stop.

CHANGELOG.md Outdated Show resolved Hide resolved
@ShanMa1991 ShanMa1991 merged commit db8e9a5 into main Dec 5, 2020
@ShanMa1991 ShanMa1991 deleted the shan-components-consistency-647 branch December 5, 2020 01:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
UI Work related to visual components, Android Auto, Camera, 3D, voice, etc.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants