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

MapboxNavigationNative v9.0.4 #2319

Merged
merged 2 commits into from
Jun 9, 2020
Merged

MapboxNavigationNative v9.0.4 #2319

merged 2 commits into from
Jun 9, 2020

Conversation

1ec5
Copy link
Contributor

@1ec5 1ec5 commented Jan 23, 2020

Upgraded the MapboxNavigationNative dependency from v6.2.1 to v9.0.4. The changes primarily affect location tracking and off-route calculation during turn-by-turn navigation. These are the areas that will need extensive testing before we can merge and release. (Additionally, MapboxNavigationNative’s internal Valhalla dependency, used for offline route calculation, has been upgraded from roughly v3.0.4 to roughly v3.0.9.)

  • Update the dependency
    • Specify the new MapboxNavigationNative version requirement in the Cartfile and podspec
    • Run carthage update to update the copy of MapboxNavigationNative used for development in the mapbox-navigation-ios repository
    • Run cd MapboxCoreNavigationTests/CocoaPodsTest/PodInstall/ && pod update to update the CocoaPods installation tests
  • Build the MapboxNavigation scheme and fix any compiler errors that arise
  • Upgrade again to get the fix for Rerouting doesn't work when Route was calculated with Arabic or Hebrew locale #2336
  • Disable filtering
  • Test the upgraded project:
    • Run the MapboxCoreNavigation scheme’s automated tests
    • Run the Example or Example-CarPlay scheme in an iPhone simulator, enabling the application’s Enable Simulation option
    • Run the Example or Example-CarPlay scheme on an iPhone device without enabling the Enable Simulation option
    • Run the CarPlay example application on a CarPlay device
    • Test-drive the CarPlay example application in a car that has a CarPlay-capable screen

Fixes #2125.

/cc @mapbox/navigation-ios @mapbox/navnative @mapbox/driver-app

@1ec5 1ec5 added this to the v1.0.0 milestone Jan 23, 2020
@1ec5 1ec5 self-assigned this Jan 23, 2020
@1ec5
Copy link
Contributor Author

1ec5 commented Jan 23, 2020

The CocoaPods podspec fails to lint because of compiler warnings about MapboxNavigationNative:

    - WARN  | xcodebuild:  <module-includes>:1:1: warning: umbrella header for module 'MapboxNavigationNative' does not include header 'MBHistoryPlayer.h'
    - WARN  | xcodebuild:  <module-includes>:1:1: warning: umbrella header for module 'MapboxNavigationNative' does not include header 'MBGetRouteCallback.h'
    - WARN  | xcodebuild:  <module-includes>:1:1: warning: umbrella header for module 'MapboxNavigationNative' does not include header 'MBSetRouteCallback.h'
    - WARN  | xcodebuild:  <module-includes>:1:1: warning: umbrella header for module 'MapboxNavigationNative' does not include header 'MapboxNavigationNativeHistoryPlayer.h'
    - WARN  | xcodebuild:  <module-includes>:1:1: warning: umbrella header for module 'MapboxNavigationNative' does not include header 'MBGetTraceAttributesCallback.h'
    - WARN  | xcodebuild:  <module-includes>:1:1: warning: umbrella header for module 'MapboxNavigationNative' does not include header 'MBUpdateSensorDataCallback.h'
    - WARN  | xcodebuild:  <module-includes>:1:1: warning: umbrella header for module 'MapboxNavigationNative' does not include header 'MBConfigureRouterCallback.h'
    - WARN  | xcodebuild:  <module-includes>:1:1: warning: umbrella header for module 'MapboxNavigationNative' does not include header 'MBLocateCallback.h'
    - WARN  | xcodebuild:  <module-includes>:1:1: warning: umbrella header for module 'MapboxNavigationNative' does not include header 'MBIterationResult.h'
    - WARN  | xcodebuild:  <module-includes>:1:1: warning: umbrella header for module 'MapboxNavigationNative' does not include header 'MBGetStatusCallback.h'
    - WARN  | xcodebuild:  <module-includes>:1:1: warning: umbrella header for module 'MapboxNavigationNative' does not include header 'MBUpdateLocationCallback.h'
    - WARN  | xcodebuild:  <module-includes>:1:1: warning: umbrella header for module 'MapboxNavigationNative' does not include header 'MBChangeRouteLegCallback.h'
    - WARN  | xcodebuild:  <module-includes>:1:1: warning: umbrella header for module 'MapboxNavigationNative' does not include header 'MBPushHistoryCallback.h'

[!] MapboxCoreNavigation did not pass validation, due to 13 warnings (but you can use `--allow-warnings` to ignore them).
You can use the `--no-clean` option to inspect any issue.

The CocoaPods build bot isn’t required to pass; it basically tells us whether we need to pass --allow-warnings when releasing.

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 23, 2020

My first impression with this branch in the iOS simulator is that 4× route simulation is noticeably jerky compared to how it used to be. MapboxNavigationNative is probably doing more aggressive filtering that treats these unlikely fast location updates as bogus.

@mskurydin
Copy link
Member

Could you share the recorded history? We will be able to check all the algorithmic stuff.

@1ec5 1ec5 changed the title MapboxNavigationNative v9.0.3 MapboxNavigationNative v9.0.4 Feb 14, 2020
@1ec5
Copy link
Contributor Author

1ec5 commented Feb 14, 2020

After upgrading from v9.0.3 to v9.0.4, one of the tests is failing because twice the expected number of location updates is coming into the navigation service delegate:

Test Case '-[MapboxCoreNavigationTests.NavigationServiceTests testUnimplementedLogging]' started.
2020-02-14 00:38:17.646596-0800 Example[83813:124462] [delegation.EmptyNavigationServiceDelegate] Unimplemented delegate method in EmptyNavigationServiceDelegate: NavigationServiceDelegate.navigationService(_:didUpdate:with:rawLocation:). This message will only be logged once.
/Users/distiller/project/MapboxCoreNavigationTests/NavigationServiceTests.swift:998: error: -[MapboxCoreNavigationTests.NavigationServiceTests testUnimplementedLogging] : XCTAssertEqual failed: ("8") is not equal to ("4") - Expected logs to be populated and expected number of messages sent
Test Case '-[MapboxCoreNavigationTests.NavigationServiceTests testUnimplementedLogging]' failed (1.226 seconds).

@mskurydin
Copy link
Member

@1ec5 could you help understand the problem deeper? The only noticeable change for 9.0.4 was speed calculation while doing the look-ahead (during guided driving).

@JThramer JThramer added the release blocker Needs to be resolved before the release. label Apr 1, 2020
@JThramer JThramer mentioned this pull request Apr 1, 2020
6 tasks
@1ec5 1ec5 modified the milestones: v1.0.0, v0.x next Apr 14, 2020
@1ec5 1ec5 modified the milestones: v0.40.0, v1.0.0 Apr 24, 2020
@1ec5 1ec5 marked this pull request as ready for review June 8, 2020 15:11
@IodaMikeMapbox
Copy link
Contributor

IodaMikeMapbox commented Jun 8, 2020

After upgrading from v9.0.3 to v9.0.4, one of the tests is failing because twice the expected number of location updates is coming into the navigation service delegate:

I investigated the problem, it's related to the our route replay, we simulate the passage of a track with speed multiplier 4 which is roughly similar to have ride at speed 432 km/h. Looks like processing of that speed wasn't supported by v6 which led to not updating progress and we didn't get events of arriving at destination. If we decrease the speedMultiplier to 1 with v6 the sdk will generate three more events which is willArrive, didArrive and shouldPreventReroutesWhenArrivingAt, the total correct number of events is 7.
As for 8 events. There is location filter in version 9 which, if the speedMulitplier is set to 4, led to a rerouting while traveling on a virtual route. Also filtering solved the problem of not arriving at destination. In total, since the rerouting is one more event, the number of events in v9 was 7+1=8.
For now I fixed the test by decreasing the speedMultiplier to 2 in ee5fcb3, but we should rethink our approach with speed multiplier and sync it with android, the android sdk works correctly even with speed multiplier equal to 8.

@1ec5
Copy link
Contributor Author

1ec5 commented Jun 9, 2020

The Xcode_10.2_iOS_12.2_CP_update build failure is being tracked in #2394.

@1ec5 1ec5 merged commit 62fa079 into master Jun 9, 2020
@1ec5 1ec5 deleted the 1ec5-nav-native-v9.0 branch June 9, 2020 15:16
binary "https://www.mapbox.com/ios-sdk/MapboxNavigationNative.json" ~> 6.2.1
github "mapbox/mapbox-directions-swift" ~> 0.31
binary "https://www.mapbox.com/ios-sdk/MapboxNavigationNative.json" ~> 9.0.3
github "mapbox/mapbox-directions-swift" == 1.0.0-alpha.1
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This should’ve remained v0.31. Looks like a rebase incorrectly changed it back to v1.0.0-alpha.1. Fixed in c3dda00.

@@ -1,7 +1,7 @@
binary "https://www.mapbox.com/ios-sdk/MapboxAccounts.json" ~> 2.2
binary "https://www.mapbox.com/ios-sdk/Mapbox-iOS-SDK.json" ~> 5.6
binary "https://www.mapbox.com/ios-sdk/MapboxNavigationNative.json" ~> 6.2.1
github "mapbox/mapbox-directions-swift" ~> 0.31
binary "https://www.mapbox.com/ios-sdk/MapboxNavigationNative.json" ~> 9.0.3
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pinned to v9.0.4 specifically in 5cae87d and aa149ed, because v9.2.1 is available and not backwards-compatible. Compatibility with v9.2.1 would require a bit of work, but we’re doing that anyways as part of upgrading to v13.x.

/cc @mapbox/navnative

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn’t working release blocker Needs to be resolved before the release. topic: instructions topic: location
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No RouteProgress updates when shape format is geoJSON
5 participants