Update ETA text color to match congestion on route by bsudekum · Pull Request #403 · mapbox/mapbox-navigation-ios

bsudekum · 2017-07-20T18:23:02Z

todo:

update according to remaining segments on route
use color in route line

1ec5 · 2017-07-20T19:42:49Z

MapboxNavigation/RouteTableViewController.swift

Is it possible for an unknown to appear in a response that otherwise has known congestion values? Would it be better to try and account for the unknown slot than to bail?

1ec5 · 2017-07-20T19:50:23Z

Examples/Swift/ViewController.swift

Also add a comment explaining why .expectedTravelTime appears here.

bsudekum · 2017-07-22T16:49:43Z

MapboxNavigation/RouteTableViewController.swift

+            // From this, we can estimate what coordinates in the route remain.
+            // This color change does not need to be 100% accurate, 
+            // just a rough estimation of remaining route congestion.
+            let estimatedCoordinatesRemaining = Int(floor(Double(coordinates.count) * routeProgress.fractionTraveled))


@1ec5 how do you feel about this logic?

Hard to say. The number of coordinates in a given step has no relation to its distance. For example, a short, windy stretch of road may have many more coordinates than a longer, straighter stretch of road.

(Note, I'm looking at the overall route here but your point is still valid.)

The code has changed significantly to account for only the remaining portion of the route.

1ec5 · 2017-07-23T05:52:04Z

MapboxNavigation/RouteTableViewController.swift

+
+            guard let coordinates = routeProgress.route.coordinates else { return }
+
+            // To keep this cheap, use the fraction traveled along the current route.


It would be very convenient to know which segments correspond to a given step, just as we know the geometry along an individual step. When initializing NavigationViewController with a route or when a new route comes in, let’s build a dictionary mapping steps to their segments, which we can reuse each time this method is called.

@1ec5 how would this help this PR? We're not really too interested in the given step, rather the entire route.

Aren’t we considering the total of expected travel times along all the remaining segments of the route? We could recompute that every time this method is called, which you’ve studiously avoided, but one optimization would be to essentially index the segments by step so that it would be cheaper to get all the remaining segments starting from the current location. So when this method is called, we’d sum the expected travel times on all the remaining segments on the current step plus all the remaining steps.

bsudekum · 2017-07-27T00:17:16Z

MapboxCoreNavigation/RouteProgress.swift

+            for step in leg.steps {
+                if let lastCoord = step.coordinates?.last, let coordinates = route.coordinates, let segmentCongestionLevels = leg.segmentCongestionLevels, let expectedSegmentTravelTimes = leg.expectedSegmentTravelTimes  {
+                    guard let nextIndex = coordinates.index(where: {
+                        $0.latitude == lastCoord.latitude && $0.longitude == lastCoord.longitude


@1ec5 @frederoni It looks like this is only truthy once; on the first step. Could there be a rounding error?

Yeah, comparing doubles is bound to fail. Instead, let's take advantage of the fact that nextIndex is always coordinateIndex + step.coordinates.count to avoid a somewhat expensive call to index(where:).

bsudekum · 2017-07-27T16:38:57Z

@1ec5 @frederoni this is ready for review

1ec5 · 2017-07-27T16:56:10Z

MapboxNavigation/RouteTableViewController.swift

-            let remaingCongestionSegmentTimes = legSegments + currentStepCongestionTimes
+            let segementsForStep = routeProgress.congestionTravelTimesSegmentsByStep[routeProgress.legIndex][routeProgress.currentLegProgress.stepIndex]
+
+            guard coordinatesRemainingOnStep <= segementsForStep.count else { return }


Aren’t there always fewer coordinates remaining on the step than the total number of coordinates on the step?

1ec5 · 2017-07-27T16:56:22Z

MapboxNavigation/RouteTableViewController.swift

-            let currentStepCongestionTimes = routeProgress.congestionTravelTimesSegmentsByStep[routeProgress.legIndex][routeProgress.currentLegProgress.stepIndex].suffix(estimatedCoordinatesRemaining)
-            let legSegments = Array(Array(routeProgress.congestionTravelTimesSegmentsByStep.joined()).joined())
-            let remaingCongestionSegmentTimes = legSegments + currentStepCongestionTimes
+            let segementsForStep = routeProgress.congestionTravelTimesSegmentsByStep[routeProgress.legIndex][routeProgress.currentLegProgress.stepIndex]


Typo: s/segements/segments/.

1ec5 · 2017-07-27T16:58:48Z

MapboxCoreNavigation/RouteProgress.swift

+    /**
+     If the route contains both `segmentCongestionLevels` and `expectedSegmentTravelTimes`, this array will contain an array of arrays of tuples of `segmentCongestionLevels` and `expectedSegmentTravelTimes`.
+     */
+    public var congestionTravelTimesSegmentsByStep: [[[(CongestionLevel, TimeInterval)]]] = []


Factor out (CongestionLevel, TimeInterval) into a typealias. Maybe call it TimedCongestionLevel or CongestedTimeInterval. 😛 Then use the rename this property and congestionTravelTimesSegmentsByLeg accordingly.

By the way, note that this property won’t bridge to Objective-C. I think that’s OK though; the only expected user of this property is MapboxNavigation.

1ec5 · 2017-07-27T17:02:02Z

MapboxCoreNavigation/RouteProgress.swift

        super.init()
        currentLegProgress = RouteLegProgress(leg: currentLeg, stepIndex: 0, alertLevel: alertLevel)
+
+        var coordinateIndex = 0


maneuverCoordinateIndex would be more descriptive. A documentation comment like this would help as well (you can give local variables documentation comments too):

An index into the route’s coordinates and congestionTravelTimesSegmentsByStep that corresponds to a step’s maneuver location.

1ec5 · 2017-07-27T17:04:56Z

MapboxCoreNavigation/RouteProgress.swift

+                    let stepCoordinatesCount = Int(step.coordinateCount) + coordinateIndex - 1
+
+                    let congestionSegment = Array(segmentCongestionLevels[coordinateIndex-stepIndex..<stepCoordinatesCount - stepIndex])
+                    let travelTimeSegment = Array(expectedSegmentTravelTimes[coordinateIndex-stepIndex..<stepCoordinatesCount - stepIndex])


stepSegmentCongestionLevels and stepSegmentTravelTimes

1ec5 · 2017-07-27T17:13:14Z

MapboxNavigation/Style.swift


+    /**
+     Sets an alternate color used throughout the UI to denote low traffic congestion. Not used to style the route line.
+     */


Given that this color is used to style text, not the route line, let’s call this property lowTrafficTextColor.

(It would be more grammatical to call the other properties lowTrafficColor, moderateTrafficColor, etc., but I guess that can be a separate PR.)

1ec5 · 2017-07-27T17:14:41Z

MapboxNavigation/RouteTableViewController.swift

+
+            let segementsForStep = routeProgress.congestionTravelTimesSegmentsByStep[routeProgress.legIndex][routeProgress.currentLegProgress.stepIndex]
+
+            guard coordinatesRemainingOnStep <= segementsForStep.count else { return }


I wonder if we should assert that this is the case.

1ec5 · 2017-07-27T17:17:39Z

MapboxNavigation/RouteTableViewController.swift

+
+            guard coordinatesRemainingOnStep <= segementsForStep.count else { return }
+
+            let currentStepCongestionTimes = segementsForStep.suffix(from: coordinatesRemainingOnStep)


This could be useful functionality to put on RouteStepProgress.

1ec5 · 2017-07-27T17:47:46Z

MapboxNavigation/RouteTableViewController.swift

        } else {
            headerView.timeRemaining.text = dateComponentsFormatter.string(from: routeProgress.durationRemaining)
+
+            let coordinatesRemainingOnStep = Int(floor((Double(routeProgress.currentLegProgress.currentStepProgress.step.coordinateCount)) * routeProgress.currentLegProgress.currentStepProgress.fractionTraveled))


countOfCoordinatesLeftOnStep so it’s clear that the property is a number, not an array of coordinates.

1ec5 · 2017-07-27T17:51:34Z

MapboxNavigation/RouteTableViewController.swift

+            guard coordinatesRemainingOnStep <= segementsForStep.count else { return }
+
+            let currentStepCongestionTimes = segementsForStep.suffix(from: coordinatesRemainingOnStep)
+            let remainingStepSegments = Array(routeProgress.congestionTravelTimesSegmentsByStep[routeProgress.legIndex].suffix(from: routeProgress.currentLegProgress.stepIndex)).joined()


If I’m not mistaken, this could still be a huge array unless we find a way to flatten or precompute the expected travel time buckets for later steps. Can we add another dictionary to RouteProgress that would make it possible to change this code to something like the following sketch?

let timedCongestionLevelsLeftOnStep: [TimedCongestionLevel] = stepTimedCongestionLevels.suffix(from: countOfCoordinatesLeftOnStep) // var travelTimesByCongestionLevelByStep: [[[CongestionLevel: TimeInterval]]] let timesLeftOnLegByCongestionLevel: [[CongestionLevel: TimeInterval]] = routeProgress.travelTimesByCongestionLevelByStep[routeProgress.legIndex].suffix(from: legProgress.stepIndex) // `sum(_:)` is really a call to `reduce(_:_:)` // The for loop before is OK too – just wrote it this way for clarity. let lowTimeLeftOnStep: TimeInterval = sum(timedCongestionLevelsLeftOnStep.filter { $0.0.low }.map { $0.1 }) let lowTimeLeftOnLeg: TimeInterval = sum(timesLeftOnLegByCongestionLevel.map { $0.low }) let remainingLowTime = lowTimeLeftOnStep + lowTimeLeftOnLeg

bsudekum · 2017-07-31T23:56:21Z

Updated this, but I'm still off by 1 somewhere. The remaining step congestion times do not add up quite yet.

bsudekum · 2017-08-15T16:03:09Z

Got this working 🎉 . @1ec5 ready for another round of review.

1ec5 · 2017-08-17T00:29:21Z

MapboxCoreNavigation/RouteProgress.swift

     */
    public var currentLegProgress: RouteLegProgress!

+    public typealias TimedCongestionLevel = (CongestionLevel, TimeInterval)


Since this typealias is public and the method arguments that use it are also public, the typealias needs documentation.

1ec5 · 2017-08-17T00:31:51Z

MapboxNavigation/RouteTableViewController.swift

This will crash if congestionTimesPerStep[routeProgress.legIndex] has only two items and we’re on step 1 (the second step), right?

1ec5 · 2017-08-17T00:35:01Z

MapboxCoreNavigation/RouteProgress.swift

+                for (stepIndex, step) in leg.steps.enumerated() {
+                    let stepCoordinatesCount = Int(step.coordinateCount) + maneuverCoordinateIndex - 1
+
+                    let congestionSegment = Array(segmentCongestionLevels[maneuverCoordinateIndex-stepIndex..<stepCoordinatesCount - stepIndex])


Can you answer this question?

1ec5 · 2017-08-17T00:35:21Z

MapboxCoreNavigation/RouteProgress.swift

+                    let stepCoordinatesCount = Int(step.coordinateCount) + maneuverCoordinateIndex - 1
+
+                    let congestionSegment = Array(segmentCongestionLevels[maneuverCoordinateIndex-stepIndex..<stepCoordinatesCount - stepIndex])
+                    let travelTimeSegment = Array(expectedSegmentTravelTimes[maneuverCoordinateIndex-stepIndex..<stepCoordinatesCount - stepIndex])


#403 (comment)

1ec5 · 2017-08-17T00:35:36Z

MapboxCoreNavigation/RouteProgress.swift

+                    let congestionSegment = Array(segmentCongestionLevels[maneuverCoordinateIndex-stepIndex..<stepCoordinatesCount - stepIndex])
+                    let travelTimeSegment = Array(expectedSegmentTravelTimes[maneuverCoordinateIndex-stepIndex..<stepCoordinatesCount - stepIndex])
+
+                    let zippedCongestionTimes = Array(zip(congestionSegment, travelTimeSegment))


#403 (comment)

bsudekum · 2017-08-22T23:19:56Z

This is ready for review if @frederoni or @1ec5 want to check this out.

1ec5 · 2017-08-23T05:51:02Z

MapboxCoreNavigation/RouteProgress.swift

    public var currentLegProgress: RouteLegProgress!

+    /**
+     Tupel containing a `CongestionLevel` and a corresponding `TimeInterval` representing the expected travel time for this segment.


Typo: “tupel”.

1ec5 · 2017-08-23T05:57:26Z

MapboxCoreNavigation/RouteProgress.swift

+    public typealias TimedCongestionLevel = (CongestionLevel, TimeInterval)
+
+    /**
+     If the route contains both `segmentCongestionLevels` and `expectedSegmentTravelTimes`, this array will contain an array of arrays of tuples of `segmentCongestionLevels` and `expectedSegmentTravelTimes`.


this array will contain an array of arrays of tuples of segmentCongestionLevels and expectedSegmentTravelTimes

This isn’t very helpful; Quick Help could’ve told the developer that. On the other hand, it would be helpful to phrase the comment like:

…this property is set to a deeply nested array of TimeCongestionLevels per segment per step per leg.

1ec5 previously approved these changes Jul 20, 2017

View reviewed changes

Bobby Sudekum added 2 commits July 22, 2017 11:48

Update ETA color to match congestion on route

512c58d

Update

1f28e88

bsudekum force-pushed the eta-color branch from 5c4e84a to 1f28e88 Compare July 22, 2017 16:49

bsudekum commented Jul 22, 2017

View reviewed changes

1ec5 reviewed Jul 23, 2017

View reviewed changes

Bobby Sudekum added 2 commits July 24, 2017 13:33

Update

ae4b0a1

Update

1a8a575

bsudekum commented Jul 27, 2017

View reviewed changes

fix

7fb405f

1ec5 reviewed Jul 27, 2017

View reviewed changes

ericrwolfe modified the milestone: v0.7.0-1 Jul 27, 2017

Bobby Sudekum added 3 commits July 27, 2017 16:42

update

c13f614

Merge branch 'master' into eta-color

985e0a6

fix

de25216

Bobby Sudekum added 2 commits August 1, 2017 11:37

Merge branch 'master' into eta-color

fba4743

fix?

1b83987

ericrwolfe modified the milestones: v0.7.0-1, v0.7.0-2 Aug 9, 2017

Bobby Sudekum added 2 commits August 15, 2017 08:26

Merge branch 'master' into eta-color

2815c06

Fix

19321ca

bsudekum added the ✓ ready for review label Aug 15, 2017

ericrwolfe removed this from the v0.7.0-2 milestone Aug 16, 2017

ericrwolfe modified the milestones: v0.7.0-3, v0.7.0-2 Aug 16, 2017

1ec5 reviewed Aug 17, 2017

View reviewed changes

Bobby Sudekum added 6 commits August 17, 2017 10:43

Fix

d597c9c

remove departure step

0f8b3bf

remove -1

e3faaa3

Merge branch 'master' into eta-color

4e2acde

1

2ef4d7d

Merge branch 'master' into eta-color

6873cac

1ec5 approved these changes Aug 23, 2017

View reviewed changes

Bobby Sudekum added 2 commits August 23, 2017 09:16

Doc

0a32278

come on test

309fd7e

bsudekum merged commit 5f38563 into master Aug 23, 2017

bsudekum deleted the eta-color branch August 23, 2017 16:40

1ec5 mentioned this pull request Jun 28, 2021

Route traffic summary mapbox/mapbox-navigation-android#4559

Open


		guard let coordinates = routeProgress.route.coordinates else { return }

		// To keep this cheap, use the fraction traveled along the current route.


		let segementsForStep = routeProgress.congestionTravelTimesSegmentsByStep[routeProgress.legIndex][routeProgress.currentLegProgress.stepIndex]

		guard coordinatesRemainingOnStep <= segementsForStep.count else { return }


		guard coordinatesRemainingOnStep <= segementsForStep.count else { return }

		let currentStepCongestionTimes = segementsForStep.suffix(from: coordinatesRemainingOnStep)

Comments

Conversation

bsudekum commented Jul 20, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsudekum commented Jul 27, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsudekum commented Jul 31, 2017

Uh oh!

bsudekum commented Aug 15, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

1ec5 Aug 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bsudekum commented Aug 22, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

bsudekum commented Jul 20, 2017 •

edited

Loading

1ec5 Aug 17, 2017 •

edited

Loading