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.

Sign up for GitHub

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

Jump to bottom

Custom Rerouting #3472

Merged
merged 8 commits into from
Nov 29, 2021
Merged

Custom Rerouting #3472

merged 8 commits into from
Nov 29, 2021

Conversation

Udumft
Copy link
Contributor

@Udumft Udumft commented Oct 13, 2021

Resolves #3464

This PR modifies RouterDelegate.router(_:willRerouteFrom:) method to allow user to select if he want's to customize the output route.

@Udumft Udumft added op-ex Refactoring, Tech Debt or any other operational excellence work. topic: directions Core Work related to core navigation and integrations. labels Oct 13, 2021
@Udumft Udumft added this to the v2.1-beta milestone Oct 13, 2021
@Udumft Udumft self-assigned this Oct 13, 2021
Udumft
Udumft commented Oct 13, 2021
View reviewed changes
Sources/MapboxCoreNavigation/RouterDelegate.swift Outdated
*/
func router(_ router: Router, willRerouteFrom location: CLLocation)
func router(_ router: Router, willRerouteFrom location: CLLocation) -> ReroutingRequest
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to stick to the return value instead of providing a callback to be called by user. It is less convenient to use it this way when there are multithreaded calculations on the User side, but it is safer in terns that user will be forced to return a value in any case. Otherwise, user could just ignore/forget to call the callback and it'll jam the rerouting.

@Udumft Udumft force-pushed the vk/1114-dangerous-maneuver branch 2 times, most recently from c3a038f to 99d51ef Compare November 5, 2021 14:57
@mapbox-github-ci-issues-3
Copy link

Breaking Changes in MapboxCoreNavigation

Breaking API Changes

MapboxNavigationService

  • removed method: router(_:willRerouteFrom:) in MapboxNavigationService

NavigationServiceDelegate

  • removed method: navigationService(_:willRerouteFrom:) in NavigationServiceDelegate
  • removed method: navigationService(_:willRerouteFrom:) in NavigationServiceDelegate

@mapbox-github-ci-issues-3
Copy link

Breaking Changes in MapboxNavigation

Breaking API Changes

NavigationViewControllerDelegate

  • removed method: navigationViewController(_:willRerouteFrom:) in NavigationViewControllerDelegate
  • removed method: navigationViewController(_:willRerouteFrom:) in NavigationViewControllerDelegate

NavigationViewController

  • removed method: navigationService(_:willRerouteFrom:) in NavigationViewController

@Udumft Udumft marked this pull request as ready for review November 8, 2021 12:25
@Udumft Udumft requested a review from a team November 8, 2021 12:25
@Udumft
Copy link
Contributor Author

Udumft commented Nov 8, 2021

Originally it was labeled for 2.1, but since there are some breaking changes, could we move it to 2.2 maybe?

@MaximAlien
Copy link
Contributor

@Udumft, could you please rebase this PR onto main as there are few conflicts now? Dependencies you're using here were updated as well in main directly in #3590.

@MaximAlien MaximAlien modified the milestones: v2.1-beta, v2.1-rc Nov 12, 2021
@MaximAlien
Copy link
Contributor

Moved to v2.1-rc for now as v2.1-beta is no longer relevant.

@mapbox-github-ci-issues-2
Copy link

Breaking Changes in MapboxCoreNavigation

Breaking API Changes

NavigationServiceDelegate

  • removed method: navigationService(_:willRerouteFrom:) in NavigationServiceDelegate
  • removed method: navigationService(_:willRerouteFrom:) in NavigationServiceDelegate

@mapbox-github-ci-issues-2
Copy link

Breaking Changes in MapboxNavigation

Breaking API Changes

NavigationViewControllerDelegate

  • removed method: navigationViewController(_:willRerouteFrom:) in NavigationViewControllerDelegate
  • removed method: navigationViewController(_:willRerouteFrom:) in NavigationViewControllerDelegate

@Udumft
Copy link
Contributor Author

Udumft commented Nov 15, 2021

Since this PR introduces a breaking change while we are at RC stage, I switch it to target 2.2 version

@Udumft Udumft modified the milestones: v2.1-rc, v2.2 Nov 15, 2021
azarovalex
azarovalex reviewed Nov 18, 2021
View reviewed changes
Sources/MapboxCoreNavigation/RouteController.swift Outdated
case .failure(let error):
return calculationCompletion(session, .failure(error))
case .success(let response):
guard let mostSimilarIndex = response.routes?.index(mostSimilarTo: progress.route) else {
Copy link
Contributor

@azarovalex azarovalex Nov 18, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Some customers pointed out that this algorithm can sometimes select the route with the biggest ETA, should we somehow allow them to select one of the routes using their own algorithm?
Currently, it's a little bit misleading that we call willReroute callback not before the reroute itself, but before we start to fetch possible reroutes.
Maybe we should rename willReroute to willFetchReroutes and add a new willReroute callback where an application can select the best route.🤔

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A bit ugly, but user can achieve this if they just remove "slower" routes from reroutingResult.

We could (and actually will) revisit rerouting logic once we incorporate NavNative's rerouting implementation.

@Udumft
vk-1114-dangerous-maneuver: added a unit test for overriding reroutin…
358a4b9 
…g radius
@Udumft
vk-3464-custom-reroute: modified Router delegate method to allow prov…
986fd94 
…iding custom route for rerouting. Modified underlying logic to reflect the change; Unit tests added/updated.
@Udumft
vk-3464-custom-reroute: removed duplicated test
e840ab6
@Udumft
vk-3464-custom-reroute: reworked delegate methods to allow new featur…
e2ab288 
…e in non-breaking fashion: added new delegate method to ask for reroute, removed modification of 'willReroute' method; tests updated; CHANGELOG updated.
@Udumft
vk-3464-custom-reroute: TestHelper compilation fixed.
5b25bd3
@mapbox-github-ci-issues-2
Copy link

No breaking changes detected in MapboxCoreNavigation

@mapbox-github-ci-issues-2
Copy link

No breaking changes detected in MapboxNavigation

@mapbox-github-ci-issues-4
Copy link

No breaking changes detected in MapboxCoreNavigation

@mapbox-github-ci-issues-4
Copy link

No breaking changes detected in MapboxNavigation

@Udumft Udumft requested a review from a team November 26, 2021 08:25
azarovalex
azarovalex approved these changes Nov 26, 2021
View reviewed changes
Copy link
Contributor

@azarovalex azarovalex left a comment

Choose a reason for hiding this comment

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

lgtm

@Udumft Udumft merged commit 2e22445 into vk/1114-dangerous-maneuver Nov 29, 2021
@Udumft Udumft deleted the vk/3464-custom-reroute branch November 29, 2021 08:09
Udumft added a commit that referenced this pull request Nov 29, 2021
@Udumft
Custom Rerouting (#3472)
f79f1e2 
vk-3464-custom-reroute: reworked delegate methods to allow new feature in non-breaking fashion: added new delegate method to ask for reroute, tests updated; CHANGELOG updated.
Udumft added a commit that referenced this pull request Dec 7, 2021
@Udumft
Custom Rerouting (#3472)
6120852 
vk-3464-custom-reroute: reworked delegate methods to allow new feature in non-breaking fashion: added new delegate method to ask for reroute, tests updated; CHANGELOG updated.
@Udumft Udumft mentioned this pull request Dec 13, 2021
Closed
@1ec5
Copy link
Contributor

1ec5 commented Mar 7, 2022

These changes were backed out in #3668.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@azarovalex azarovalex azarovalex approved these changes

Assignees

@Udumft Udumft

Labels
Core Work related to core navigation and integrations. op-ex Refactoring, Tech Debt or any other operational excellence work. topic: directions
Projects
None yet
Milestone
v2.2
Development

Successfully merging this pull request may close these issues.
4 participants
@Udumft @MaximAlien @1ec5 @azarovalex