Refactor vanishing point calculation to use EPSG:3857 projection #3661

LukasPaczos · 2020-10-19T14:54:49Z

This PR updates the algorithm that calculates the vanishing point of the route to use EPSG:3857 projection instead of turf measurements. The precision is now spot on, even on extremely long routes, it even greatly outperforms the precision of the over zoomed map tiles.

By dropping all of the required turf calculations, we also significantly improve the performance of the MapRouteLine. After spending a couple of cycles optimizing the code, we're at a point where the biggest penalty comes from calling Layer#nativeSetPaintProperty which we won't be able to rule out from the Nav SDK level.

In the ideal scenario, we're trying to update the lines' gradient expression on each frame of the puck. The gradient expression can become very large for long routes (especially with traffic congestion enabled) and unfortunately, the performance is not good enough for us to fully unlock the potential of the vanishing route line.

That's why I've opted to add a throttling mechanism for the vanishing point updates. It defaults to performing 5 updates per second, which lets the map remain perfectly smooth even for very long routes, while it does not impact the UX at all because our puck hides the imperfections:

You can see that I need to overzoom well beyond zoom level 18 (which the Nav experience is locked to by default) to notice the throttled animation.

That said, the precision is there, so if developers would need a smoother vanishing point animation, they can use NavigationMapRoute.Builder#withVanishingRouteLineUpdateIntervalNano:

/cc @tmpsantos @tobrun for ideas how to optimize native paint property setter, this shouldn't be a blocker though

cafesilencio · 2020-10-19T16:56:27Z

I"m not seeing the NavigationMapRoute.Builder#withVanishingRouteLineUpdateIntervalNano you refer to.

cafesilencio · 2020-10-19T17:31:49Z

I'm seeing a regression of this issue: #3615

tobrun · 2020-10-19T18:07:09Z

Great work @LukasPaczos.

our puck hides the imperfections

Yes, while it shouldn't hold back merging this improvement, it's not a solution that will scale well (or downscales ).

we're at a point where the biggest penalty comes from calling Layer#nativeSetPaintProperty

To be clear, if we can cut down the exeuction time, we would be able to update more frequently? To me it would result in a better UX but the issue of 1 component catching up to another will remain? or are the update in time for next render? do you have any concrete numbers on the cpu time? is this also an issue with a release build?

cafesilencio · 2020-10-19T18:21:57Z

our puck hides the imperfections:

One thing to keep in mind is that at least one of our customers uses a transparent puck.

LukasPaczos · 2020-10-20T09:05:41Z

To be clear, if we can cut down the exeuction time, we would be able to update more frequently?

@tobrun yes, definitely. Right now the native layer property setter is the bottlneck. Is there an opportunity to move more of the native internals of that method to a worker thread?

LukasPaczos · 2020-10-20T09:08:00Z

I"m not seeing the NavigationMapRoute.Builder#withVanishingRouteLineUpdateIntervalNano you refer to.

I didn't push the latest 🤦

LukasPaczos · 2020-10-20T09:51:42Z

To me it would result in a better UX but the issue of 1 component catching up to another will remain?

No, the second gif from the OP represents no throttling in place, the vanishing point and the puck are perfectly in sync.

do you have any concrete numbers on the cpu time?

This is a capture from an unlocked, ~20 second run of a route from Poland to Spain, where the line's gradient is updated on each frame of the location puck's animation for maximum precision. ~60% of the main thread's time is spent in the native paint property setter and the app is stuttering a lot.

Here's the full recording @tobrun: cpu-art-20201020T111444.zip

is this also an issue with a release build?

Nav SDK debug builds already use release builds of the Maps SDK so I wasn't expecting much improvement, but the performance is actually improved in release builds, probably optimizing some of the Nav SDK calculations. I increased the default interval to 16 updates per second which looks good even for a partially transparent puck, although I'd still like to be conservative with the number of updates we execute in the interest of battery consumption. I feel like the impl works great for the default scenario and each customer can fine-tune the update frequency to best match the use-case and devices' capabilities while we can work on other optimizations in parallel.

LukasPaczos · 2020-10-20T11:51:56Z

I'm seeing a regression of this issue: #3615

Not able to reproduce with the latest changes, am I doing it correctly?

cafesilencio · 2020-10-20T15:42:00Z

I'm seeing a regression of this issue: #3615

Not able to reproduce with the latest changes, am I doing it correctly?

I suggest testing with the simulator so that the animation callback doesn't get called and the line updated too quickly to see the issue. I was able reproduce it several times.

LukasPaczos · 2020-10-20T15:53:13Z

@cafesilencio would you be able to retest with the latest if you have a reproducible example handy?

cafesilencio · 2020-10-20T16:05:20Z

After pulling the latest I'm not able to reproduce the issue after trying several times.

LukasPaczos · 2020-10-20T16:09:21Z

Thanks for retesting @cafesilencio! In this case, this is ready for another round.

cafesilencio

Great work.

Guardiola31337

Great work @LukasPaczos

Left some minor comments.

Guardiola31337 · 2020-10-20T21:07:07Z

libnavigation-ui/src/main/java/com/mapbox/navigation/ui/route/MapRouteLine.kt

+            /**
+             * Finds the count of remaining points in the current step.
+             *
+             * TurfMisc.lineSliceAlong places an additional point at index 0 to mark the precise
+             * cut-off point which we can safely ignore.
+             * We'll add the distance from the upcoming point to the current's puck position later.
+             */
+            allRemainingPoints += try {
+                TurfMisc.lineSliceAlong(
+                    LineString.fromLngLats(currentStepProgress.stepPoints ?: emptyList()),
+                    currentStepProgress.distanceTraveled.toDouble(),
+                    currentStepProgress.step?.distance() ?: 0.0,
+                    TurfConstants.UNIT_METERS
+                ).coordinates().drop(1).size
+            } catch (e: TurfException) {
+                0
+            }


What about extracting this block of code into a private function and give it a name based on the comment? This way updateUpcomingRoutePointIndex will be easier to read and understand and comment will become superfluous so it won't be necessary.

All those small pieces of logic have no application outside of this main method's context, they will not be reused anywhere else, and splitting them up would only artificially increase the method count of the library. In my opinion, the comments already split the impl into readable chunks.

The same applies to all comments below.

I'm gonna go ahead and merge, let's address later if needed.

I don't agree. Normally comments are a code smell, they get outdated quickly because code changes but comments don't 😬 so they start lying 🤯

The best comment is a good name for a method or class.

or a variable.

Our future selves would thank us!

Guardiola31337 · 2020-10-20T21:07:39Z

libnavigation-ui/src/main/java/com/mapbox/navigation/ui/route/MapRouteLine.kt

+            /**
+             * Add to the count of remaining points all of the remaining points on the current leg,
+             * after the current step.
+             */
+            val currentLegSteps = completeRoutePoints.nestedList[currentLegProgress.legIndex]
+            allRemainingPoints += if (currentStepProgress.stepIndex < currentLegSteps.size) {
+                currentLegSteps.slice(
+                    currentStepProgress.stepIndex + 1 until currentLegSteps.size - 1
+                ).flatten().size
+            } else {
+                0
+            }


Same as above re: extracting to a private function

Guardiola31337 · 2020-10-20T21:07:46Z

libnavigation-ui/src/main/java/com/mapbox/navigation/ui/route/MapRouteLine.kt

+            /**
+             * Add to the count of remaining points all of the remaining legs.
+             */
+            for (i in currentLegProgress.legIndex + 1 until completeRoutePoints.nestedList.size) {
+                allRemainingPoints += completeRoutePoints.nestedList[i].flatten().size
+            }


Same as above re: extracting to a private function

Guardiola31337 · 2020-10-20T21:07:53Z

libnavigation-ui/src/main/java/com/mapbox/navigation/ui/route/MapRouteLine.kt

+            /**
+             * When we know the number of remaining points and the number of all points,
+             * calculate the index of the upcoming point.
+             */
+            val allPoints = completeRoutePoints.flatList.size
+            primaryRouteRemainingDistancesIndex = allPoints - allRemainingPoints - 1
+        } ?: run { primaryRouteRemainingDistancesIndex = null }


Same as above re: extracting to a private function

Guardiola31337 · 2020-10-20T21:10:46Z

libnavigation-ui/src/main/java/com/mapbox/navigation/ui/route/MapRouteLine.kt

+            /**
+             * Take the remaining distance from the upcoming point on the route and extends it
+             * by the exact position of the puck.
+             */
+            val remainingDistance =
+                traveledIndex.distanceRemaining + calculateDistance(upcomingPoint, point)


What about using a different naming (based on the comment) for the explaining variable? This way updateTraveledRouteLine will be easier to read and understand and comment will become superfluous so it won't be necessary.

Guardiola31337 · 2020-10-20T21:11:29Z

libnavigation-ui/src/main/java/com/mapbox/navigation/ui/route/MapRouteLine.kt

+            /**
+             * Calculate the percentage of the route traveled and update the expression.
+             */
+            if (granularDistances.distance >= remainingDistance) {
+                val offset = (1.0 - remainingDistance / granularDistances.distance)
+                if (offset >= 0) {
+                    val expression = getExpressionAtOffset(offset)
+                    hideCasingLineAtOffset(offset)
+                    hideRouteLineAtOffset(offset)
+                    decorateRouteLine(expression)
+                }


What about extracting this block of code into a private function and give it a name based on the comment? This way updateTraveledRouteLine will be easier to read and understand and comment will become superfluous so it won't be necessary.

Guardiola31337 · 2020-10-20T21:16:54Z

libnavigation-ui/src/main/java/com/mapbox/navigation/ui/route/MapRouteLine.kt

+            for (i in (points.size - 1) downTo 1) {
+                val curr = points[i]
+                val prev = points[i - 1]
+                distance += calculateDistance(curr, prev)
+                indexArray.append(i - 1, RouteLineDistancesIndex(prev, distance))
            }
+            indexArray.append(
+                points.size - 1,
+                RouteLineDistancesIndex(points[points.size - 1], 0.0)
+            )


Same as above re: extracting to private functions so it's easier to read and understand

Guardiola31337 · 2020-10-20T21:17:44Z

libnavigation-ui/src/main/java/com/mapbox/navigation/ui/route/MapRouteLine.kt

+        /**
+         * Calculates the distance between 2 points using
+         * [EPSG:3857 projection](https://epsg.io/3857).
+         * Info in [mapbox-gl-js/issues/9998](https://github.com/mapbox/mapbox-gl-js/issues/9998).
+         */
+        fun calculateDistance(point1: Point, point2: Point): Double {
+            val d = doubleArrayOf(
+                (projectX(point1.longitude()) - projectX(point2.longitude())),
+                (projectY(point1.latitude()) - projectY(point2.latitude()))
            )
+            return sqrt(d[0] * d[0] + d[1] * d[1])
+        }


Should we expose this from mapbox-java?

Interesting idea, let's track in mapbox/mapbox-java#1208 👍

LukasPaczos added the UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. label Oct 19, 2020

LukasPaczos added this to the v1.2.0 milestone Oct 19, 2020

LukasPaczos requested a review from a team October 19, 2020 14:54

LukasPaczos force-pushed the lp-vanishing-precision branch from 46530b1 to e560e0e Compare October 20, 2020 09:07

LukasPaczos force-pushed the lp-vanishing-precision branch from e560e0e to 6147ddb Compare October 20, 2020 09:49

LukasPaczos force-pushed the lp-vanishing-precision branch 2 times, most recently from 69cdb8c to cae460c Compare October 20, 2020 11:38

Guardiola31337 mentioned this pull request Oct 20, 2020

Include Codecov reports to Github PR checks #3663

Merged

8 tasks

LukasPaczos force-pushed the lp-vanishing-precision branch 3 times, most recently from e0b5d88 to 3b69582 Compare October 20, 2020 13:06

[ui] refactor vanishing point calculation to use EPSG:3857 projection

c016f9d

LukasPaczos force-pushed the lp-vanishing-precision branch from 3b69582 to c016f9d Compare October 20, 2020 13:23

cafesilencio approved these changes Oct 20, 2020

View reviewed changes

Guardiola31337 reviewed Oct 20, 2020

View reviewed changes

LukasPaczos mentioned this pull request Oct 21, 2020

Expose EPSG:3857 projection calculation utils mapbox/mapbox-java#1208

Open

LukasPaczos merged commit a41c9f7 into master Oct 21, 2020

LukasPaczos deleted the lp-vanishing-precision branch October 21, 2020 10:55

Guardiola31337 mentioned this pull request Oct 21, 2020

Bump mapboxSdkServices dependencies version to 5.8.0-beta.2 #3668

Merged

10 tasks

LukasPaczos mentioned this pull request Nov 6, 2020

Introduce NavigationMapboxMap.Builder and expose vanishing point update interval #3745

Merged

ShanMa1991 mentioned this pull request Nov 12, 2020

Update vanishing point calculation with a location based implementation mapbox/mapbox-navigation-ios#2737

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor vanishing point calculation to use EPSG:3857 projection #3661

Refactor vanishing point calculation to use EPSG:3857 projection #3661

LukasPaczos commented Oct 19, 2020 •

edited

Loading

cafesilencio commented Oct 19, 2020

cafesilencio commented Oct 19, 2020

tobrun commented Oct 19, 2020

cafesilencio commented Oct 19, 2020

LukasPaczos commented Oct 20, 2020

LukasPaczos commented Oct 20, 2020

LukasPaczos commented Oct 20, 2020 •

edited

Loading

LukasPaczos commented Oct 20, 2020

cafesilencio commented Oct 20, 2020 •

edited

Loading

LukasPaczos commented Oct 20, 2020

cafesilencio commented Oct 20, 2020

LukasPaczos commented Oct 20, 2020

cafesilencio left a comment

Guardiola31337 left a comment

Guardiola31337 Oct 20, 2020

LukasPaczos Oct 21, 2020

LukasPaczos Oct 21, 2020

Guardiola31337 Oct 21, 2020

Guardiola31337 Oct 20, 2020

Guardiola31337 Oct 20, 2020

Guardiola31337 Oct 20, 2020

Guardiola31337 Oct 20, 2020

Guardiola31337 Oct 20, 2020

Guardiola31337 Oct 20, 2020

Guardiola31337 Oct 20, 2020

LukasPaczos Oct 21, 2020

Refactor vanishing point calculation to use EPSG:3857 projection #3661

Refactor vanishing point calculation to use EPSG:3857 projection #3661

Conversation

LukasPaczos commented Oct 19, 2020 • edited Loading

cafesilencio commented Oct 19, 2020

cafesilencio commented Oct 19, 2020

tobrun commented Oct 19, 2020

cafesilencio commented Oct 19, 2020

LukasPaczos commented Oct 20, 2020

LukasPaczos commented Oct 20, 2020

LukasPaczos commented Oct 20, 2020 • edited Loading

LukasPaczos commented Oct 20, 2020

cafesilencio commented Oct 20, 2020 • edited Loading

LukasPaczos commented Oct 20, 2020

cafesilencio commented Oct 20, 2020

LukasPaczos commented Oct 20, 2020

cafesilencio left a comment

Choose a reason for hiding this comment

Guardiola31337 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

LukasPaczos commented Oct 19, 2020 •

edited

Loading

LukasPaczos commented Oct 20, 2020 •

edited

Loading

cafesilencio commented Oct 20, 2020 •

edited

Loading