Milestone #84

cammace · 2017-06-15T18:48:51Z

This PR removes the alert levels in favor of a more robust system called milestones. Milestones are more abstract, use the dsl pattern, offering some of the features listed below:

Easier readability
Besides the default, developers can create their own milestones
Customize what exactly triggers the milestone/alert

An example of creating a milestone's given below. An identifier's provided to later determine what milestone got triggered in the callback and the triggers. In this case, the milestones triggered only when all 3 of the conditions are true.

addMilestone(new StepMilestone.Builder()
      .setIdentifier(NavigationConstants.URGENT_MILESTONE)
      .setTrigger(
        Trigger.all(
          Trigger.gt(TriggerValue.STEP_DISTANCE, 100d),
          Trigger.lt(TriggerValue.DURATION, 15d),
          Trigger.neq(TriggerValue.STEP_INDEX, 0)
        )
      )
      .build()
    );

cc: @mapbox/navigation

1ec5 · 2017-06-19T19:57:20Z

A few questions to better understand the flexibility being provided here, with an eye toward potentially implementing an analogous system on iOS using predicates or callbacks as part of mapbox/mapbox-navigation-ios#224:

How would a developer create a milestone that triggers 1 mile ahead of an exit ramp maneuver but 5 seconds ahead of any other maneuver?
If the developer would like some of these heuristics to depend on application state, do they need to recreate the milestones each time this state changes? Is it possible to mutate a built-in milestone? I can think of two use cases for milestones that depend on application state:
- A user setting in the form of a slider that adjusts the application’s verbosity. Perhaps the setting adjusts automatically based on whether music is playing.
- Milestone durations increase in response to real-time weather data (visibility, precipitation) obtained by the application from a third-party API.
Are there fixed semantics or a fixed ordering to the milestone identifiers provided by NavigationConstants? Is it necessary for a custom milestone’s identifier to fall between 2 and 3 in order to occur between NEW_STEP_MILESTONE and IMMINENT_MILESTONE?
How are you planning to handle transitions around intermediate waypoints? Will the user get ARRIVAL_MILESTONE on each waypoint or only the final one; similarly, will the user get DEPARTURE_MILESTONE multiple times or only once?

cammace · 2017-06-19T20:18:49Z

If I understand correctly, it should look like this.

Milestone milestone = new StepMilestone.Builder()
  .setIdentifier(MY_MILESTONE)
  .setTrigger(Trigger.all(
    Trigger.lte(TriggerProperty.STEP_DISTANCE_TOTAL, 1609.34d), // Mile converted to meters
    Trigger.gt(TriggerProperty.NEXT_STEP_DURATION, 5)// NEXT_STEP_DURATION property doesn't exist yet.
  )).build();

Good point, currently the user would have to build a new milestone. I don't see this being a blocker however since the user can remove the old milestone and quickly build a new one with the updated params.
There's no requirement for the ordering of the milestones, for example, you can have two duplicate milestones and the callback will be triggered twice with the respected milestone identifier. Milestones are completely independent of each other and aren't aware of one another, therefore, you can't specify a specific ordering of the milestones but instead, set the triggers at different times so one milestone always occurs before the other.
A new default milestone will be added which will trigger when the user reaches a waypoint, the departure and arrival milestones should only happen when the route (not leg) event occurs.

1ec5 · 2017-06-19T20:49:56Z

How would a developer create a milestone that triggers 1 mile ahead of an exit ramp maneuver but 5 seconds ahead of any other maneuver?

If I understand correctly, it should look like this. …

How does that distinguish between exit ramp maneuvers and other maneuver types? Or is that out of scope for this design?

cammace · 2017-06-19T21:24:47Z

In the future, we could always add triggers for specific milestones, currently, it is a simple implementation that was designed to match the behavior of the alert levels logic.

zugaldia

This is a fantastic PR, very excited to see this landing soon. Just a few small comments.

zugaldia · 2017-06-20T19:50:26Z

...ain/java/com/mapbox/services/android/navigation/testapp/activity/MockNavigationActivity.java

+    Timber.d("Milestone Event Occurred with id: %d", identifier);
+    switch (identifier) {
+      case NavigationConstants.URGENT_MILESTONE:
+        Toast.makeText(this, "High Milestone", Toast.LENGTH_LONG).show();


Toast text doesn't match the milestone constant.

zugaldia · 2017-06-20T19:50:33Z

...ain/java/com/mapbox/services/android/navigation/testapp/activity/MockNavigationActivity.java

+        Toast.makeText(this, "High Milestone", Toast.LENGTH_LONG).show();
+        break;
+      case NavigationConstants.IMMINENT_MILESTONE:
+        Toast.makeText(this, "Medium Milestone", Toast.LENGTH_LONG).show();


Toast text doesn't match the milestone constant.

zugaldia · 2017-06-20T19:52:30Z

...ain/java/com/mapbox/services/android/navigation/testapp/activity/MockNavigationActivity.java

+      case NavigationConstants.ARRIVAL_MILESTONE:
+        Toast.makeText(this, "Arrival", Toast.LENGTH_LONG).show();
+        break;
+      default:


Can we make the milestone constants above an IntDef enum to make the default case unnecessary? Or is this a possibility? (in which case we'd need to show a Toast too).

I added the default for checkstyle to be happy. Looking back, it makes more sense to show a toast for the cases when the user adds a new milestone but doesn't do anything with it inside the switch statement.

@cammace 👍 let's add a Toast with some generic text then.

Added the toast on line 213, Github's just not showing it in the snippet above.

zugaldia · 2017-06-20T19:56:39Z

...oid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/MapboxNavigation.java

+  private void addDefaultMilestones() {
+    // High milestone
+    addMilestone(new StepMilestone.Builder()
+      .setIdentifier(NavigationConstants.URGENT_MILESTONE)


Comment text doesn't match text in constant.

zugaldia · 2017-06-20T19:57:07Z

...oid-navigation/src/main/java/com/mapbox/services/android/navigation/v5/MapboxNavigation.java

+
+    // Medium milestone
+    addMilestone(new StepMilestone.Builder()
+      .setIdentifier(NavigationConstants.IMMINENT_MILESTONE)


Comment text doesn't match text in constant.

zugaldia · 2017-06-20T20:09:52Z

...-navigation/src/main/java/com/mapbox/services/android/navigation/v5/milestone/Milestone.java

+   * @return true if the milestone trigger's valid, else false
+   * @since 0.4.0
+   */
+  public abstract boolean validate(RouteProgress previousRouteProgress, RouteProgress routeProgress);


What does valid means in this context? That the milestone is properly constructed, or that the milestone is happening? If the latter, I think I'd change validate with something else. Considering it's a boolean, maybe something like milestone.isOccurring?

I like your naming much better 😄

zugaldia · 2017-06-20T20:11:35Z

...-navigation/src/main/java/com/mapbox/services/android/navigation/v5/milestone/Operation.java

+  }
+
+  static boolean notEqual(Number[] valueOne, Number valueTwo) {
+


Nit: extra line.

zugaldia · 2017-06-20T20:12:02Z

...-navigation/src/main/java/com/mapbox/services/android/navigation/v5/milestone/Operation.java

+package com.mapbox.services.android.navigation.v5.milestone;
+
+
+class Operation {


Even if this class isn't public I think it'd benefit from comment code.

zugaldia · 2017-06-20T20:12:39Z

...igation/src/main/java/com/mapbox/services/android/navigation/v5/milestone/StepMilestone.java

+
+  @Override
+  public boolean validate(RouteProgress previousRouteProgress, RouteProgress routeProgress) {
+


Nit: extra line.

zugaldia · 2017-06-20T20:13:38Z

...id-navigation/src/main/java/com/mapbox/services/android/navigation/v5/milestone/Trigger.java

+
+import java.util.Map;
+
+public class Trigger {


Missing Javadoc.

…ilometers) (#84)

Cameron Mace added 2 commits June 13, 2017 16:48

milestone initial architecture added

aad2b18

initial milestone logic setup complete

f1bf6eb

cammace self-assigned this Jun 15, 2017

cammace added enhancement ⚠️ DO NOT MERGE PR should not be merged! labels Jun 15, 2017

cammace added this to the v0.4.0 milestone Jun 15, 2017

Cameron Mace added 4 commits June 15, 2017 17:13

added test and docs

b661eb7

moved milestone test to own package

c093937

added test and a few properties

e535eeb

fixed checkstyle

9b2add8

cammace added ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Jun 16, 2017

cammace requested a review from zugaldia June 19, 2017 21:24

zugaldia requested changes Jun 20, 2017

View reviewed changes

Cameron Mace added 3 commits June 20, 2017 13:50

made review fixes

4a84786

added javadoc

e03af7f

fixed up checkstyle

3d5815b

zugaldia approved these changes Jun 20, 2017

View reviewed changes

cammace merged commit 3c1239b into master Jun 20, 2017

cammace deleted the cam-milestone branch June 20, 2017 22:34

ericrwolfe mentioned this pull request Jun 30, 2017

Customized milestone triggers + notifications mapbox/mapbox-navigation-ios#325

Closed

1ec5 mentioned this pull request Jul 1, 2017

Customizable guidance profiles mapbox/mapbox-navigation-ios#224

Closed

cammace mentioned this pull request Aug 1, 2017

Release 0.4.0 #152

Closed

10 tasks

LukasPaczos pushed a commit that referenced this pull request Jan 27, 2021

increase default predictive caching radius by 1000x (from meters to k…

53cade6

…ilometers) (#84)

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Milestone #84

Milestone #84

cammace commented Jun 15, 2017 •

edited

Loading

1ec5 commented Jun 19, 2017 •

edited

Loading

cammace commented Jun 19, 2017

1ec5 commented Jun 19, 2017

cammace commented Jun 19, 2017

zugaldia left a comment

zugaldia Jun 20, 2017

zugaldia Jun 20, 2017

zugaldia Jun 20, 2017

cammace Jun 20, 2017

zugaldia Jun 20, 2017

cammace Jun 20, 2017

zugaldia Jun 20, 2017

zugaldia Jun 20, 2017

zugaldia Jun 20, 2017

cammace Jun 20, 2017

zugaldia Jun 20, 2017

zugaldia Jun 20, 2017

zugaldia Jun 20, 2017

zugaldia Jun 20, 2017

		}

		static boolean notEqual(Number[] valueOne, Number valueTwo) {

		package com.mapbox.services.android.navigation.v5.milestone;


		class Operation {


		@Override
		public boolean validate(RouteProgress previousRouteProgress, RouteProgress routeProgress) {

Milestone #84

Milestone #84

Conversation

cammace commented Jun 15, 2017 • edited Loading

1ec5 commented Jun 19, 2017 • edited Loading

cammace commented Jun 19, 2017

1ec5 commented Jun 19, 2017

cammace commented Jun 19, 2017

zugaldia left a 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

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

cammace commented Jun 15, 2017 •

edited

Loading

1ec5 commented Jun 19, 2017 •

edited

Loading