Skip to content

Add ability to override congestion levels for specific road classes.#2741

Merged
MaximAlien merged 18 commits into
mainfrom
maxim/656-route-congestion-consistency
Dec 17, 2020
Merged

Add ability to override congestion levels for specific road classes.#2741
MaximAlien merged 18 commits into
mainfrom
maxim/656-route-congestion-consistency

Conversation

@MaximAlien
Copy link
Copy Markdown
Contributor

@MaximAlien MaximAlien commented Nov 13, 2020

Description

This PR implements the ability to override congestion levels for specific road classes.

Implementation

Since implementation is using Mapbox Directions property RouteStep.segmentIndicesByIntersection it depends on branch vk/478-intersection-geometry. Dependency will be updated after dependent branch is merged to main.

Since Android implementation is using road classes from mapbox_streets_v8 there is slight difference between congestion segments. After iOS is done with implementing same functionality current implementation should re-tested and revised once more.

Update: Most recent version was updated and is currently using MapboxStreetsRoadClass which matches Android implementation.

Screenshots or Gifs

Screenshots show congestion consistency comparison between iOS and Android. In this example .unknown congestion level for .motorway road class is replaced to .low (blue color). Currently there is slight shift of color because of gradient which is used on iOS.

Screen Shot 2020-11-13 at 4 09 31 PM

Screen Shot 2020-11-13 at 4 09 14 PM

@MaximAlien MaximAlien added UI Work related to visual components, Android Auto, Camera, 3D, voice, etc. iOS labels Nov 13, 2020
@MaximAlien MaximAlien added this to the v1.2.0 milestone Nov 13, 2020
@MaximAlien MaximAlien self-assigned this Nov 13, 2020
@MaximAlien MaximAlien requested a review from a team November 14, 2020 00:29
@MaximAlien MaximAlien changed the title Route congestion consistency Add ability to override congestion levels for specific road classes. Nov 14, 2020
@MaximAlien MaximAlien force-pushed the maxim/656-route-congestion-consistency branch 2 times, most recently from 11eb7e3 to ceb649e Compare November 19, 2020 20:59
@1ec5
Copy link
Copy Markdown
Contributor

1ec5 commented Nov 20, 2020

OfflineRoutingTests is failing because of a decoding error that would probably be fixed by mapbox/mapbox-directions-swift#485:

/Users/distiller/project/MapboxCoreNavigationTests/OfflineRoutingTests.swift:33: error: -[MapboxCoreNavigationTests.OfflineRoutingTests testOfflineDirections] : failed - Unexpected Failure: unknown(underlying: Swift.DecodingError.keyNotFound(CodingKeys(stringValue: "iso_3166_1_alpha3", intValue: nil), Swift.DecodingError.Context(codingPath: [CodingKeys(stringValue: "routes", intValue: nil), _JSONKey(stringValue: "Index 0", intValue: 0), CodingKeys(stringValue: "legs", intValue: nil), _JSONKey(stringValue: "Index 0", intValue: 0), CodingKeys(stringValue: "admins", intValue: nil), _JSONKey(stringValue: "Index 0", intValue: 0)], debugDescription: "No value associated with key CodingKeys(stringValue: \"iso_3166_1_alpha3\", intValue: nil) (\"iso_3166_1_alpha3\").", underlyingError: nil)))
/Users/distiller/project/MapboxCoreNavigationTests/OfflineRoutingTests.swift:43: error: -[MapboxCoreNavigationTests.OfflineRoutingTests testOfflineDirections] : Asynchronous wait failed: Exceeded timeout of 2 seconds, with unfulfilled expectations: "Calculate route offline".
/Users/distiller/project/MapboxCoreNavigationTests/OfflineRoutingTests.swift:46: error: -[MapboxCoreNavigationTests.OfflineRoutingTests testOfflineDirections] : failed - No route returned

Comment thread CHANGELOG.md Outdated
Comment thread Cartfile Outdated
Comment thread MapboxNavigation/NavigationMapView.swift Outdated
Comment thread MapboxNavigation/NavigationMapView.swift Outdated
Comment thread MapboxNavigation/NavigationMapView.swift Outdated
Comment thread MapboxNavigation/NavigationMapView.swift Outdated
Comment thread scripts/.gitignore
@MaximAlien
Copy link
Copy Markdown
Contributor Author

OfflineRoutingTests is failing because of a decoding error that would probably be fixed by mapbox/mapbox-directions-swift#485:

/Users/distiller/project/MapboxCoreNavigationTests/OfflineRoutingTests.swift:33: error: -[MapboxCoreNavigationTests.OfflineRoutingTests testOfflineDirections] : failed - Unexpected Failure: unknown(underlying: Swift.DecodingError.keyNotFound(CodingKeys(stringValue: "iso_3166_1_alpha3", intValue: nil), Swift.DecodingError.Context(codingPath: [CodingKeys(stringValue: "routes", intValue: nil), _JSONKey(stringValue: "Index 0", intValue: 0), CodingKeys(stringValue: "legs", intValue: nil), _JSONKey(stringValue: "Index 0", intValue: 0), CodingKeys(stringValue: "admins", intValue: nil), _JSONKey(stringValue: "Index 0", intValue: 0)], debugDescription: "No value associated with key CodingKeys(stringValue: \"iso_3166_1_alpha3\", intValue: nil) (\"iso_3166_1_alpha3\").", underlyingError: nil)))
/Users/distiller/project/MapboxCoreNavigationTests/OfflineRoutingTests.swift:43: error: -[MapboxCoreNavigationTests.OfflineRoutingTests testOfflineDirections] : Asynchronous wait failed: Exceeded timeout of 2 seconds, with unfulfilled expectations: "Calculate route offline".
/Users/distiller/project/MapboxCoreNavigationTests/OfflineRoutingTests.swift:46: error: -[MapboxCoreNavigationTests.OfflineRoutingTests testOfflineDirections] : failed - No route returned

Yep, I've seen that. Since current PR already depends on vk/478-intersection-geometry we'll have to wait till both mapbox/mapbox-directions-swift#490 and mapbox/mapbox-directions-swift#485 land in Mapbox Directions.

@MaximAlien MaximAlien added the blocked Blocked by dependency or unclarity. label Dec 4, 2020
@MaximAlien MaximAlien requested a review from a team December 14, 2020 19:20
@MaximAlien MaximAlien force-pushed the maxim/656-route-congestion-consistency branch 2 times, most recently from c512422 to cbc2f49 Compare December 14, 2020 20:11
@MaximAlien MaximAlien removed the blocked Blocked by dependency or unclarity. label Dec 14, 2020
Comment thread MapboxNavigation/DayStyle.swift Outdated
@MaximAlien MaximAlien requested a review from 1ec5 December 14, 2020 23:16
@MaximAlien
Copy link
Copy Markdown
Contributor Author

@1ec5, can you please take a look at this PR once more? RoadClasses option set was replaced with MapboxStreetsRoadClass, so iOS implementation is now in par with Android.

Comment thread CHANGELOG.md Outdated
Comment thread Example/ViewController.swift Outdated
Comment thread MapboxNavigation/NavigationMapView.swift Outdated
Comment thread MapboxNavigation/NavigationMapView.swift Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think RouteLeg.segmentRangesByStep may simplify this computation.

Copy link
Copy Markdown
Contributor Author

@MaximAlien MaximAlien Dec 16, 2020

Choose a reason for hiding this comment

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

Seems that segmentRangesByStep is not detailed enough, as it stores only lower and upper bounds. segmentIndicesByIntersection is more precise in this case, as it stores segment indices for Intersection.

@MaximAlien MaximAlien requested review from a team and 1ec5 December 16, 2020 20:38
@MaximAlien MaximAlien force-pushed the maxim/656-route-congestion-consistency branch from 91a5c64 to 02aaeb0 Compare December 17, 2020 00:43
Comment thread MapboxCoreNavigation/RouteLeg.swift Outdated
Comment thread MapboxCoreNavigation/RouteLeg.swift Outdated
Comment on lines 26 to 27
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

At a glance, it seems like this calculation would be simplified by zipping segmentIndices, but it’s hard to be sure. I’m a bit wary of the manual index arithmetic, but I trust you’ve already tested the multi-leg route edge case. I can’t think of any other edge cases at the moment.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Just tried several approaches with zipping and striding, but seems that it doesn't help to make this code more elegant and simple. I propose to move on with landing this PR so that we can start testing and I can follow-up with any better solutions in future PRs.

@MaximAlien MaximAlien force-pushed the maxim/656-route-congestion-consistency branch from b87b6df to bf436db Compare December 17, 2020 18:49
@MaximAlien MaximAlien merged commit 6ef0a48 into main Dec 17, 2020
@MaximAlien MaximAlien deleted the maxim/656-route-congestion-consistency branch December 17, 2020 19:04
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.

2 participants