Enable night style when inside tunnel #1127

vincethecoder · 2018-02-17T13:22:29Z

This detects tunnels on a routeStep and sets the style to DayStyle.
cc @mapbox/navigation-ios

Issue addressed: #559, #1170

NOTE:
When calculating the distance to the tunnel entrance using Polyline's distance(from:to:), there was inconsistent results. This issue is similar to the reported issue here: mapbox/turf-swift#27

In this case, the distance to the tunnel entrance reports lower values as it approaches the entrance. However, in the mid-way of the tunnel commute, it suddenly begins to jump to higher values (greater than the tunnel distance) and then reduces to lower values before it exits the tunnel. The screenshot below highlights these inconsistencies:

…ble dark mode upon entrance and reset to default style mode upon exit.

… lie within the tunnel route.

…ted by styleManager's styles property.

bsudekum · 2018-02-17T17:42:29Z

MapboxCoreNavigation/RouteController.swift

@@ -635,6 +646,10 @@ extension RouteController: CLLocationManagerDelegate {
        updateRouteStepProgress(for: location)
        updateRouteLegProgress(for: location)

+        if routeProgress.currentLegProgress.currentStep.containsTunnel {


Move this if into detectRouteStepInTunnel(for:) and return early if it's true.

bsudekum · 2018-02-17T17:43:27Z

MapboxCoreNavigation/RouteController.swift

+        //   - Current location to exit must be greater than zero meters.
+        //   - Distance to tunnel entrance must be less than tunnel distance.
+        //   - Distance to tunnel exit must also be less than tunnel distance.
+        if (tunnelDistance - distanceToExit) > 0 && distanceToEntrance <= tunnelDistance && distanceToExit <= tunnelDistance {


Do you think it's possible to have only 2 conditions here?

@bsudekum Yes indeed. Good looks 👍

bsudekum · 2018-02-17T17:45:31Z

MapboxCoreNavigation/RouteStep.swift

+    /**
+     Returns the entry and exit intersections of the tunnel on the current route step
+     */
+    var tunnelIntersection: (entry: Intersection, exit: Intersection)? {


tunnelIntersection denotes a single intersection, while in reality, we're calculating the entry and exit of a tunnel here. Could you come up with a more descriptive variable name for this variable?

bsudekum · 2018-02-17T17:46:37Z

MapboxNavigation/NavigationViewController.swift

@@ -561,6 +561,14 @@ extension NavigationViewController: RouteControllerDelegate {
        }
        return advancesToNextLeg
    }
+
+    @objc public func routeController(_ routeController: RouteController, didEnterTunnelAt location: CLLocation?) {
+        if let _ = location {


The fact we're not using location here might clue us in that it may not be necessary to send this param over.

bsudekum · 2018-02-17T17:49:27Z

MapboxCoreNavigation/RouteStep.swift

+     */
+    var tunnelDistance: CLLocationDistance? {
+        guard let tunnelIntersection = tunnelIntersection else { return nil }
+        return tunnelIntersection.entry.location.distance(to: tunnelIntersection.exit.location)


This distance calculation is an absolute point to point distance. Instead, we should be using a function which calculates distance along a line. I believe Polyline is a struct and contains a distance variable. It may be possible to remove this and just use tunnelSlice.distance.

bsudekum · 2018-02-17T17:49:56Z

MapboxCoreNavigation/RouteStep.swift

+        guard let intersections = intersections, containsTunnel else { return nil }
+        for i in 0..<(intersections.count) where intersections.count > 1 {
+            if intersections[i].outletRoadClasses == .tunnel {
+                return (entry: intersections[i], exit: intersections[i+1])


Double check that i + 1 is always within bounds before returning here.

line #77 already does that check.

Isn't line 77 only checking where the number of intersections is greater that 1? Not necessarily that the current index in the loop has a follow on index.

bsudekum · 2018-02-17T17:50:42Z

MapboxNavigation/NavigationViewController.swift

+
+    @objc public func routeController(_ routeController: RouteController, didEnterTunnelAt location: CLLocation?) {
+        if let _ = location {
+            self.styleManager.applyStyle(NightStyle())


Do you think we could first check if the style needs to be updated before continually updating the style on every location update?

@bsudekum I do a safety check before I apply the style in applyStyle(_:) on line #111.

…ing tunnel on route step.

…ers a tunnel.

frederoni · 2018-02-18T01:34:36Z

MapboxNavigation/StyleManager.swift

@@ -96,7 +96,7 @@ open class StyleManager: NSObject {
        resetTimeOfDayTimer()
    }

-    func applyStyle() {
+    func applyStyle(_ newStyle: Style? = nil) {


We should be able to avoid the special case for custom style below if we’d take a StyleType instead of a specific style here.

@frederoni you're right 👍

…igation-ios into tunnel/dark_mode_style

vincethecoder · 2018-02-18T02:49:52Z

@bsudekum and @frederoni Thanks for your feedback 👍

bsudekum · 2018-02-18T17:35:28Z

MapboxNavigation/NavigationViewController.swift

@@ -561,6 +561,14 @@ extension NavigationViewController: RouteControllerDelegate {
        }
        return advancesToNextLeg
    }
+
+    @objc public func didEnterTunnel(_ darkModeEnabled: Bool) {


darkModeEnabled makes it seem like it was already enabled.

bsudekum · 2018-02-18T17:37:01Z

MapboxNavigation/StyleManager.swift

@@ -96,6 +96,21 @@ open class StyleManager: NSObject {
        resetTimeOfDayTimer()
    }

+    func applyStyle(type styleType: StyleType) {


Could this func be combined with the applyStyle below?

Yes, I considered it. However, this func applyStyle(type:) is cleaner, since it requires the styleType. The other func applyStyle scope is getting a bit bigger and complex with more conditions.

UPDATES: I also refactored applyStyle so that we have a function applyStyle(type:) to contain styleType related logic.

bsudekum · 2018-02-18T17:37:47Z

MapboxCoreNavigation/RouteController.swift

+     - parameter darkModeEnabled: The darkModeEnabled flag determines whether a `DarkStyle` should apply.
+     */
+    @objc(didEnterTunnel:)
+    optional func didEnterTunnel(_ darkModeEnabled: Bool)


We should remove the notion of dark mode from RouteController because MapboxCoreNavigation deals with non-UI related functionality.

Makes sense. Will do.

bsudekum · 2018-02-18T17:39:45Z

MapboxCoreNavigation/RouteController.swift

+        // Pre-conditions:
+        //   - Current location to exit must be greater than zero meters.
+        //   - Distance to tunnel entrance must be less than tunnel distance.
+        if (tunnelDistance - distanceToExit) > 0 && distanceToEntrance <= tunnelDistance {


Would something like

if distanceToEntrance <= tunnelDistance && distanceToExit <= tunnelDistance { ...

I only suggest this since it's easier to understand what we're comparing on each side of the && operator.

bsudekum · 2018-02-18T17:41:01Z

MapboxCoreNavigation/RouteController.swift

+        let distanceToEntrance = currentLocation.distance(to: tunnelStartCoordinate)
+        let distanceToExit = currentLocation.distance(to: tunnelEndCoordinate)
+
+        // Capute any location which lies within the tunnel route.


Nit: no need for route in tunnel route. Also, Capute -> Compute.

capture -- misspelt

1ec5

This is looking great – thanks for managing this deluge of feedback and iterating on the approach. 😄 Since this has been a pretty long review, here are a few outstanding pieces of feedback to consider before merging:

1ec5 · 2018-03-06T22:30:39Z

MapboxCoreNavigation/RouteProgress.swift

@@ -417,21 +417,10 @@ open class RouteStepProgress: NSObject {
    }

    /**
-     Returns all the intersection distances on the current step.
+     Returns an array of the calculated distances from the current intersection to the next intersection on the current step.


Nit: this is a property, not a method, so it is set to the array rather than returning the array.

1ec5 · 2018-03-06T22:33:37Z

MapboxNavigation/NavigationViewController.swift

+    /**
+     A Boolean value that determines whether the dark style should be automatically activated when a route controller enters a tunnel.
+     */
+    @objc public var automaticallySwitchStyleForTunnels: Bool = false


Nit: use the declarative form instead of the imperative form when naming a property. In Objective-C, a property’s getter can look identical to an action method:

// Does this line immediately switch the style? [navigationViewController automaticallySwitchStyleForTunnels];

A clearer name would be automaticallySwitchesStyleInsideTunnels or usesNightStyleInsideTunnels.

Honestly, I prefer to use enableNightStyleInsideTunnelFeatureEnabled to hint that variable is a feature flag. Just my 2¢.

1ec5 · 2018-03-06T22:37:32Z

MapboxCoreNavigation/RouteProgress.swift

@@ -388,21 +388,39 @@ open class RouteStepProgress: NSObject {
    /**
     The next intersection the user will travel through.

-     The step must contains `Intersections` for this value not be `nil`.
+     The step must contain `intersectionsIncludingUpcomingManeuverIntersection` for this value not be `nil`.


This is still unclear, because intersectionsIncludingUpcomingManeuverIntersection is a property of RouteStepProgress, not RouteStep.

If intersectionsIncludingUpcomingManeuverIntersection is nil, this property is also nil.

1ec5 · 2018-03-06T22:41:44Z

MapboxNavigation/Constants.swift

-/**
- The minimum distance of a tunnel intersection on a given route step.
- */
-public var MBMinimumTunnelLengthForUsingNightStyle: CLLocationDistance = 100


I think the intent was to address the feedback in #1127 (comment) by moving this constant, but I don’t see where it got moved to.

@1ec5 we're not using this constant at the moment. The tunnel distance check will be done on my upcoming PR for simulation of the tunnel navigation.

…and upcoming intersections, along with their corresponding calculated distances.

…ersections bounds. Modified description text for current and upcoming intersections. Modified variable to indicate when night style should be enabled inside a tunnel.

…ersections bounds.

…ck on intersections bounds to initial state.

bsudekum · 2018-03-08T18:51:32Z

MapboxNavigation/NavigationViewController.swift

@@ -409,7 +409,7 @@ public class NavigationViewController: UIViewController {

        mapViewController?.notifyDidChange(routeProgress: routeProgress, location: location, secondsRemaining: secondsRemaining)

-        if automaticallySwitchStyleForTunnels,
+        if enableNightStyleInsideTunnelFeatureEnabled,


This property has the word enable(d) in it twice. I too am a fan of suggestion - automaticallySwitchesStyleInsideTunnels.

Per our discussion, this variable will be usesNightStyleInsideTunnels.

cc @bsudekum @1ec5

…ersections bounds.

bsudekum · 2018-03-08T18:58:14Z

MapboxCoreNavigation/RouteController.swift

@@ -628,6 +628,10 @@ extension RouteController: CLLocationManagerDelegate {
        let currentStepProgress = routeProgress.currentLegProgress.currentStepProgress
        let currentStep = currentStepProgress.step

+        let intersectionDistances = routeProgress.currentLegProgress.currentStepProgress.intersectionDistances


If the user has set NavigationViewController.automaticallyChangeStyleForTunnel to false, is it necessary to perform these calculations?

@bsudekum FYI

It shouldn't be restricted to automaticallyChangeStyleForTunnel. It should be wrapped within a isDeadReckoningEnabled flag.

This calculation updates the intersectionIndex - fix for issue Intersection index is always 0 #1170. @1ec5 do you need to use this fix for a task unrelated to dead reckoning?

@vincethecoder I don't think this code should be intertwined with isDeadReckoningEnabled. Let's keep these as separate ideas for now.

IMO, the fix here is to move NavigationViewController.automaticallyChangeStyleForTunnel to RouteController and rename it to something more appropriate.

@bsudekum I believe it was mentioned in one of my PR comments to separate the UI-related things in NavigationViewController from RouteController. Now, I'm totally thrown for loop 🤔
cc @1ec5 - do you recall this discussion?

Yes, in #1127 (comment) I recommended keeping UI-related stuff out of Core Navigation. I stand by that recommendation.

…ck on intersections bounds to initial state.

…e route controller.

bsudekum · 2018-03-08T22:03:28Z

MapboxNavigation/NavigationViewController.swift

+                    routeController.isDeadReckoningEnabled = true
+                } else {
+                    styleManager.timeOfDayChanged()
+                    routeController.isDeadReckoningEnabled = false


Why is isDeadReckoningEnabled being mutated?

There is no need to continue mark the intersection as a candidate for dead reckoning.

1ec5 · 2018-03-08T22:28:52Z

MapboxCoreNavigation/RouteController.swift

@@ -942,6 +948,12 @@ extension RouteController: CLLocationManagerDelegate {
        } else {
            routeProgress.currentLegProgress.stepIndex += 1
        }
+
+        if isDeadReckoningEnabled, let coordinates = routeProgress.currentLegProgress.currentStep.coordinates, let intersections = routeProgress.currentLegProgress.currentStep.intersections {


If intersectionDistances is only set when dead reckoning is enabled, then the documentation for that property should say so.

Sure. As soon as we (@bsudekum, @1ec5, myself) gain clarity on whether we are wrapping this inside a isDeadReckoningEnabled flag, I will make a final update on the intersectionDistances property.

This code should not be influenced by dead reckoning. Remove if isDeadReckoningEnabled from this check.

Already removed - post our discussion on slack 😺

… on night style in tunnels.

vincethecoder added 8 commits February 17, 2018 02:42

Added ability to detect commuter is in a tunnel and automatically ena…

d2581fc

…ble dark mode upon entrance and reset to default style mode upon exit.

Minor code refactor for tunnel intersection.

0a67127

Minor grammatical updates to comments on route step tunnel intersection.

52564d6

Minor text updates.

2f67660

Added comments to explain preconditions to capture all locations that…

72fee83

… lie within the tunnel route.

Removed console logs for debug purposes.

1b0e577

Removed resetTimeOfDayTimer method, as styles are automatically adjus…

14c5b8e

…ted by styleManager's styles property.

Updated style manager to apply custom styles.

d764421

vincethecoder added the ✓ ready for review label Feb 17, 2018

Set style to night style when in tunnel.

dc482f7

bsudekum reviewed Feb 17, 2018

View reviewed changes

vincethecoder added 3 commits February 17, 2018 16:30

A few enhancement/refsctor for routestep and routecontroller discover…

1b1152a

…ing tunnel on route step.

Updated logic for setting a custom style.

19ac74d

Modified delegate method to enable tunnel dark mode when commuter ent…

f81319b

…ers a tunnel.

frederoni reviewed Feb 18, 2018

View reviewed changes

vincethecoder and others added 6 commits February 17, 2018 18:26

Added function to apply a specific style type.

aadadf4

Added a break to the for-loop for retrieving the desired style type.

181b428

corrected grammatical error

5198ce4

Removed extra spaces

b2b0186

Merge branch 'tunnel/dark_mode_style' of github.com:mapbox/mapbox-nav…

05ef98f

…igation-ios into tunnel/dark_mode_style

Refactored condition limit to 2

bb89fff

bsudekum reviewed Feb 18, 2018

View reviewed changes

Deleted extra white space.

2acde88

1ec5 approved these changes Mar 6, 2018

View reviewed changes

1ec5 reviewed Mar 6, 2018

View reviewed changes

vincethecoder added 2 commits March 7, 2018 00:36

Minor refactor

241bba6

Updated the change logs with a note regarding new fields for current …

4ad24d0

…and upcoming intersections, along with their corresponding calculated distances.

1ec5 assigned vincethecoder and unassigned 1ec5 Mar 8, 2018

vincethecoder added 3 commits March 8, 2018 13:38

Updates to use a CountableClosedRange on the conditional check on int…

6ef815c

…ersections bounds. Modified description text for current and upcoming intersections. Modified variable to indicate when night style should be enabled inside a tunnel.

Updates to use a CountableClosedRange on the conditional check on int…

f0411b8

…ersections bounds.

Reverted updates to use a CountableClosedRange on the conditional che…

5ecb164

…ck on intersections bounds to initial state.

bsudekum reviewed Mar 8, 2018

View reviewed changes

Updates to use a CountableClosedRange on the conditional check on int…

36a374a

…ersections bounds.

bsudekum reviewed Mar 8, 2018

View reviewed changes

vincethecoder added 2 commits March 8, 2018 14:23

Reverted updates to use a CountableClosedRange on the conditional che…

cb8a46b

…ck on intersections bounds to initial state.

Added dead reckoning flag to restrict intersection calculations on th…

709231e

…e route controller.

vincethecoder changed the title ~~[Tunnel Navigation] - Enable night mode when inside a tunnel~~ Enable night style when inside tunnel Mar 8, 2018

bsudekum reviewed Mar 8, 2018

View reviewed changes

1ec5 reviewed Mar 8, 2018

View reviewed changes

Refactored code into functions and renamed feature flag for switching…

70c5e76

… on night style in tunnels.

bsudekum modified the milestones: v0.15.0, v0.16.x Mar 9, 2018

bsudekum approved these changes Mar 12, 2018

View reviewed changes

Merge branch 'master' into tunnel/dark_mode_style

9c878c3

bsudekum modified the milestone: v0.16.x Mar 12, 2018

vincethecoder merged commit 2acbe46 into master Mar 13, 2018

This was referenced Mar 20, 2018

Intersection index is always 0 #1170

Closed

Update changelog #1244

Merged

bsudekum mentioned this pull request Mar 28, 2018

Enable night mode while in tunnels #559

Closed

vincethecoder deleted the tunnel/dark_mode_style branch May 2, 2018 12:55

Enable night style when inside tunnel #1127

Enable night style when inside tunnel #1127

Conversation

vincethecoder commented Feb 17, 2018 • edited Loading

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

vincethecoder Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincethecoder Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

vincethecoder commented Feb 18, 2018

bsudekum Feb 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincethecoder Feb 18, 2018 • edited Loading

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

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincethecoder Mar 8, 2018 • edited Loading

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

vincethecoder Mar 8, 2018 • edited Loading

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

vincethecoder Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

bsudekum Mar 8, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vincethecoder commented Feb 17, 2018 •

edited

Loading

vincethecoder Feb 18, 2018 •

edited

Loading

vincethecoder Feb 18, 2018 •

edited

Loading

bsudekum Feb 18, 2018 •

edited

Loading

vincethecoder Feb 18, 2018 •

edited

Loading

vincethecoder Mar 8, 2018 •

edited

Loading

vincethecoder Mar 8, 2018 •

edited

Loading

vincethecoder Mar 8, 2018 •

edited

Loading

bsudekum Mar 8, 2018 •

edited

Loading