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

package delivery/ride sharing waypoint demo #641

Merged
merged 18 commits into from
Jan 12, 2018

Conversation

devotaaabel
Copy link
Contributor

This PR is for the example for a Package Delivery/Ride Share service that needs to be able to navigate to multiple destinations. On arrival, the driver is given an alert dialog which gives them the option to navigate to next destination, or to cancel.

ezgif com-optimize 1

A few things I wanted to bring up for discussion:

  • Should the origin/destination markers be removed once the next leg of navigation is started?
  • The choppiness of the MockLocationEngine, starting after the first destination is reached. The first leg of the trip was smooth, but everything after was choppy
  • The pivoting of the camera when transitioning from arriving at a destination to starting the next leg of navigation

cc. @ericrwolfe @danesfeder

@devotaaabel devotaaabel added the ⚠️ DO NOT MERGE PR should not be merged! label Jan 9, 2018
@ericrwolfe
Copy link
Contributor

Nice work! Looping in @bsudekum and @ericdeveloper in on this as well for 👀.

@ericrwolfe
Copy link
Contributor

Should the origin/destination markers be removed once the next leg of navigation is started?

Once the next leg has started, I think we should remove the previous destination marker.

@@ -341,6 +346,11 @@ public void updateLocationLayer(Location location) {
* @param options with containing route / coordinate data
*/
public void startNavigation(NavigationViewOptions options) {
for (Marker marker : markers) {
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, can we extract the removal of these markers into a private method (something like clearMarkers()?

@devotaaabel devotaaabel added ✓ ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Jan 10, 2018
@@ -84,6 +84,8 @@ public void updateShouldSimulateRoute(boolean shouldSimulateRoute) {
* @param route to be mocked
*/
public void updateRoute(DirectionsRoute route) {
// MockLocationEngine is deactivated first to avoid weird behavior with subsequent navigation sessions
deactivateLocationEngine();
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we test with this removed now that we merged in the fix from #646?

Copy link
Contributor

@danesfeder danesfeder left a comment

Choose a reason for hiding this comment

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

Looks great, found some good bugs with NavigationView implementing this example

@devotaaabel devotaaabel merged commit 3454e8e into master Jan 12, 2018
@devotaaabel devotaaabel deleted the devota-waypoint-navigation branch January 12, 2018 15:53
@danesfeder danesfeder mentioned this pull request Jan 23, 2018
14 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants