Add FusedLocationEngine to Test App #1373

kevinkreiser · 2018-10-01T19:26:36Z

I've been testing with the sample app and have seen that the location updates using LocationEngine are coming in between every 2 and 3 seconds on average. In practice sometimes we get no location updates for much longer periods of time. Additionally when sitting still we dont get updates with the conventionally LocationEngine. This leads to a problem where we can't tell if we are sitting still or have no signal when navigating.

I've been able to take a sample application, fork it, and instrument it to see that indeed my device is capable of secondly gps updates (even when sitting still). This work is here: https://github.com/kevinkreiser/android-play-location/blob/master/LocationUpdates/app/src/main/java/com/google/android/gms/location/sample/locationupdates/MainActivity.java

@Guardiola31337 suggested that the fastest way for me to try this approach within the SDK was to simply write an AlternativeLocationEngine that extended the conventional one so that is what I did. So far the results are promising. As with the other test app, I am getting secondly updates.

danesfeder

@kevinkreiser added some cleanup here and updating naming / formatting. Good to go for the test-app for now. Thanks again for running with this 🚀

Guardiola31337

You crushed it again @kevinkreiser 💪

I've left some minor comments. Also noticed that the camera in ComponentNavigationActivity isn't tracking anymore 👀

I've double checked and this is also reproducible in master. Is that working as designed @danesfeder? If not, we should cut a ticket to fix.

Guardiola31337 · 2018-10-08T13:50:59Z

...x/services/android/navigation/testapp/activity/navigationui/ComponentNavigationActivity.java

  public void onPause() {
    super.onPause();
    mapView.onPause();
+    locationEngine.deactivate();


This is not needed. Actually, it's stopping the location engine when putting the app in the foreground.

For testing purposes I went ahead and fix ☝️ in 5978bd9 cc @kevinkreiser @danesfeder

Guardiola31337 · 2018-10-08T14:02:21Z

...va/com/mapbox/services/android/navigation/testapp/activity/location/FusedLocationEngine.java

+
+  private void createLocationRequest() {
+    locationRequest = new LocationRequest();
+    locationRequest.setInterval(UPDATE_INTERVAL_IN_MILLISECONDS);


setInterval, setFastestInterval and setPriority are hardcoded. We should use set LocationEngine values instead.

Guardiola31337 · 2018-10-08T14:08:28Z

...va/com/mapbox/services/android/navigation/testapp/activity/location/FusedLocationEngine.java

+  private Location lastLocation = null;
+  private final ForwardingLocationCallback forwardingCallback = new ForwardingLocationCallback(this);
+
+  public FusedLocationEngine(@NonNull Activity activity) {


Shouldn't be this a Context instead?

Guardiola31337 · 2018-10-10T10:41:57Z

Also noticed that the camera in ComponentNavigationActivity isn't tracking anymore 👀

I've double checked and this is also reproducible in master. Is that working as designed @danesfeder? If not, we should cut a ticket to fix.

Found the "issue" thanks to @LukasPaczos. Latest changes in the LLP (mapbox/mapbox-plugins-android#544 (comment) and follow up PR mapbox/mapbox-plugins-android#583) + integrating them in the SDK (#1372, #1377 and #1386) introduced some camera gotchas https://www.mapbox.com/android-docs/plugins/overview/location-layer/#following-the-user-location-with-cameramode 👇

Traditional camera transitions will be canceled when any of the camera modes, besides CameraMode#NONE, are engaged. Use LocationLayerPlugin#zoomWhileTracking and LocationLayerPlugin#tiltWhileTracking to manipulate the camera in a tracking state. Use these two in combination with MapboxMap#CancelableCallback to schedule fluid transitions.

When instantiating the location layer plugin for the first time, the map's max/min zoom levels will be set to LocationLayerOptions#MAX_ZOOM_DEFAULT and LocationLayerOptions#MIN_ZOOM_DEFAULT respectively. Adjust the zoom range with the maxZoom() and minZoom() methods in the LocationLayerOptions class.

Going to open a PR that disables the camera tracking while not navigating.

Also, while debugging this I noticed that NavigationMapboxMap#updateCameraTrackingMode requests a @NavigationCamera.TrackingMode. Wouldn't be a good idea to request a @CameraMode.Mode instead - removing the necessity of having to implement @NavigationCamera.TrackingMode and the extra logic in NavigationCamera#setCameraMode from our side? Am I missing something? @danesfeder @LukasPaczos

LukasPaczos · 2018-10-10T10:51:28Z

The idea of exposing and requiring @NavigationCamera.TrackingMode instead of @CameraMode.Mode was to limit available camera mode options to those that are tested and well coordinated with the rest of the navigation UI/UX. Exposing the rest of camera tracking modes would generate another set of paths to maintain and wasn't in the scope of the original PR, but that's definitely an option we can explore going forward.

Guardiola31337 · 2018-10-12T12:11:47Z

Exposing the rest of camera tracking modes would generate another set of paths to maintain and wasn't in the scope of the original PR

Makes sense. Thanks for clarifying @LukasPaczos

but that's definitely an option we can explore going forward.

Yeah, let's revisit this in a follow up PR. It seems like we're duplicating logic unnecessarily. While testing OP I tried to set the camera mode to NONE and I wasn't able to. I know that we already have the updateCamerarTrackingEnabled API to achieve that but I guess we could use the options from LLP and remove that "duplicated" behavior. What do you think @danesfeder @LukasPaczos?

LukasPaczos · 2018-10-12T13:13:32Z

Adding the support for NONE sounds like a great first step to me.

…ates

…igation-native version to kk_animate-SNAPSHOT-5 for testing

…ompared against the system clock for proper navigation/animation

…lue is compared against the system clock for proper navigation/animation" This reverts commit 1a2781e.

…ompared against the system clock for proper navigation/animation

… request a context (instead of an activity) in fused location engine constructor

Guardiola31337 · 2018-10-17T12:03:16Z

Going to go ahead and merge here.

I opened a follow up ticket re. above camera mode discussion 👉 #1431

Thanks @kevinkreiser for running with this one 🙏

kevinkreiser added the ⚠️ DO NOT MERGE PR should not be merged! label Oct 1, 2018

kevinkreiser requested review from Guardiola31337 and danesfeder October 1, 2018 19:26

danesfeder changed the title ~~an alternative location engine that provides more stable location updates~~ Add FusedLocationEngine to Test App Oct 4, 2018

danesfeder force-pushed the kk_high_priority_gps branch from c374999 to c01ed05 Compare October 4, 2018 15:15

danesfeder approved these changes Oct 4, 2018

View reviewed changes

danesfeder force-pushed the kk_high_priority_gps branch from c01ed05 to c0c103e Compare October 4, 2018 15:49

danesfeder added ✓ ready for review and removed ⚠️ DO NOT MERGE PR should not be merged! labels Oct 4, 2018

Guardiola31337 reviewed Oct 8, 2018

View reviewed changes

Guardiola31337 force-pushed the kk_high_priority_gps branch from c0c103e to 6041ae4 Compare October 10, 2018 08:05

Guardiola31337 mentioned this pull request Oct 10, 2018

Fix component navigation activity camera issues #1407

Merged

Guardiola31337 force-pushed the kk_high_priority_gps branch from 6041ae4 to 5978bd9 Compare October 10, 2018 14:35

kevinkreiser and others added 6 commits October 17, 2018 12:30

An alternative location engine that provides more stable location upd…

f2cf9e6

…ates

remove location engine deactivation from on pause and bump mapbox-nav…

d1d27bb

…igation-native version to kk_animate-SNAPSHOT-5 for testing

overwrite location timestamps using the system clock. this value is c…

9b114ba

…ompared against the system clock for proper navigation/animation

Revert "overwrite location timestamps using the system clock. this va…

b75c9c6

…lue is compared against the system clock for proper navigation/animation" This reverts commit 1a2781e.

overwrite location timestamps using the system clock. this value is c…

08fe965

…ompared against the system clock for proper navigation/animation

use location result get locations method for new location updates and…

ea9bbfe

… request a context (instead of an activity) in fused location engine constructor

Guardiola31337 force-pushed the kk_high_priority_gps branch from 3492dd1 to ea9bbfe Compare October 17, 2018 10:56

Guardiola31337 mentioned this pull request Oct 17, 2018

Use @CameraMode.Mode instead of @NavigationCamera.TrackingMode #1431

Closed

Guardiola31337 merged commit 7637e4a into master Oct 17, 2018

Guardiola31337 deleted the kk_high_priority_gps branch October 17, 2018 12:04

danesfeder mentioned this pull request Oct 24, 2018

Release v0.22.0 #1458

Closed

11 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add FusedLocationEngine to Test App #1373

Add FusedLocationEngine to Test App #1373

Uh oh!

kevinkreiser commented Oct 1, 2018 •

edited

Loading

Uh oh!

danesfeder left a comment

Uh oh!

Guardiola31337 left a comment

Uh oh!

Guardiola31337 Oct 8, 2018

Uh oh!

Guardiola31337 Oct 10, 2018

Uh oh!

Guardiola31337 Oct 8, 2018

Uh oh!

Guardiola31337 Oct 8, 2018

Uh oh!

Guardiola31337 commented Oct 10, 2018

Uh oh!

LukasPaczos commented Oct 10, 2018

Uh oh!

Guardiola31337 commented Oct 12, 2018

Uh oh!

LukasPaczos commented Oct 12, 2018

Uh oh!

Guardiola31337 commented Oct 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Add FusedLocationEngine to Test App #1373

Add FusedLocationEngine to Test App #1373

Uh oh!

Conversation

kevinkreiser commented Oct 1, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

danesfeder left a comment

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 left a comment

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 Oct 10, 2018

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 Oct 8, 2018

Choose a reason for hiding this comment

Uh oh!

Guardiola31337 commented Oct 10, 2018

Uh oh!

LukasPaczos commented Oct 10, 2018

Uh oh!

Guardiola31337 commented Oct 12, 2018

Uh oh!

LukasPaczos commented Oct 12, 2018

Uh oh!

Guardiola31337 commented Oct 17, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

kevinkreiser commented Oct 1, 2018 •

edited

Loading