Implement Responsive User Location Tracking Mode

[bleege]

The User Location tracking mode (None, Follow, Bearing) was disabled in #1856 as the User Location dot was being updated on the main UI thread. The core logic is there, but it needs to be implemented in a more efficient way so that the UI remains responsive.

/cc @erf

[erf]

Could it be added as an annotation?

[ljbade]

@bleege Is the issue that there is too many events?

[erf]

i made a lowpass filter in #1968 which could be used

[erf]

in addition to some minimal time / angle diff

[erf]

I think you should move out the setDirection call from updateMap to the onCompassChange event.

[bleege]

@bleege Is the issue that there is too many events?

Yes.

[ljbade]

@bleege How does iOS handle that?

[1ec5]

@ljbade, on iOS, MGLMapView doesn’t do anything special. But it seems that Core Location is doing a better job of coalescing location updates than what we’re using on Android: #1676.

[bleege]

But it seems that Core Location is doing a better job of coalescing location updates than what we’re using on Android

Exactly. We need to put a regulator / distance filter on updates to location or specifically that a certain amount of distance / time needs to elapse before updating the dot on the map.

[ljbade]

@bleege I recall the LOST framework has this ability to filter (certainly Google Play services do).

We still need to work in @erf's compass filter for that data.

[ljbade]

I was getting confused between location/GPS events and map position change events.

[ljbade]

In #2409 I'm cleaning up the updateMap() code so this should be unblocked by performance issues now.

[mikemorris]

Have you tried sampling input/direction (I've used a constant rate of 15-20 times per second, but we could likely go lower) instead of trying to process or filter all events? I've done this in the past for handling user input from a gyroscope and accelerometer more smoothly in JavaScript, where it was previously choking on large bursts of minor change events.

Sample code at https://github.com/mikemorris/gyrocopter/blob/master/gyrocopter.js

[ljbade]

Hmm I might have accidentally removed the user tracking stuff in #2002, but the code is still there in git history so perhaps we can have a go at making it really good this time.

Lets start with just location/GPS (and heading?) tracking and work on compass after.

Should we also hit the UI side of getting our GPS dot looking better?

[ljbade]

Im thinking we eventually want a few modes (start with first two):

• no tracking
• just location tracking
• location tracking and map heading/rotation from GPS direction
• location tracking and map heading/rotation from rate limited compass

Also do we make touch events cancel tracking (separate for rotation and pan events). Or leave that up to the app via the listening events.

[tobrun]

Discussed with @bleege, Important part here is that while zooming the GPS marker 'jitters' and shown GPS location is wrong. If that problem is identified and fixed, I will start looking at optimising the bearing part.

[tobrun]

While zoomed in I'm above the city Leuven and the orange highway

Zooming out, I'm below the city Leuven and highway:

[tobrun]

I guessing that one possible reason for performance is calling these methods.

// Update Location
FrameLayout.LayoutParams lp = new FrameLayout.LayoutParams((int) iconSize, (int) iconSize);
lp.leftMargin = (int) (screenLocation.x - iconSize / 2.0f);
lp.topMargin = (int) (screenLocation.y + iconSize / 2.0f);
mGpsMarker.setLayoutParams(lp);
mGpsMarker.requestLayout();

These can be replaced by setX and setY:

// Update Location
mGpsMarker.setX(screenLocation.x);
mGpsMarker.setY(screenLocation.y);

Setting layoutparams and requesting layout should be considered to be expensive operations. These should not be called in a frequently called update method.

[tobrun]

I'm guessing that wrong location is related to not taking into account the size of GPS marker itself. EDIT: If I make the marker smaller, the location accuracy improves

[tobrun]

I'm glad to report that location bug is resolved! The marker location did not take in account the size of the marker.

[tobrun]

Some things I changed to improve the performance of the frequently called update method:

• avoid creating objects in update method (this is called multiple times while doing gestures)
• move some code to initialisation (some things were set every time, while once would be enough)

[tobrun]

Now I'm noticing a different problem, when zoomed in at maximum, you can see the userdot jump from location to location.

Edit: I tried low pass filtering, seems to be a bit of improvement, doesn't resolve the issue

[bleege]

Mapbox for iOS has this pretty well figured out. We should start this improvement by porting the logic that they used. Details can be found in the links below. For those new to iOS, MGLUserLocation.m and MGLUserLocationAnnotationView.m are going to be the ones to start looking at.

• MGLUserLocation.m
• MGLUserLocationAnnotationView.h
• MGLUserLocationAnnotationView.m
• MGLUserLocation_Private.h

[tobrun]

We are currently using the same thresholds as iOS (1m) but the data we are receiving is just not as accurate. Will do some testing tomorrow by fiddling with LOST configurations. If that doesn't workout I will try out some different strategies average, sampling etc.

[bleege]

Ok, I tracked down the code in iOS for handling this. In iOS didUpdateLocations method is the moral equivalent of onLocationChanged() in Android. Conceptually we should be able to port the same logic and avoid reinventing the wheel.

[bleege]

Oh nevermind me. That's the code for getting the UserDot to not be so jumpy.

[ljbade]

@tobrun - ah I think what happened here was that at some stage we changed the GPS dot image but forgot to update the code! Nice catch.

[tobrun]

I will first take a look reintegrating

location tracking and map heading/rotation from GPS direction location tracking and map heading/rotation from rate limited compass

before optimising the location updates. I feel like this should be streamlined in LOST itself instead fixing it ourselves.

[tobrun]

I'm going to replace following enum:

public enum UserLocationTrackingMode {
        NONE, FOLLOW, FOLLOW_BEARING
}

with integers while using an IntDef annotation:

@IntDef({NONE, FOLLOW, FOLLOW_BEARING})
@Retention(RetentionPolicy.SOURCE)
public @interface UserLocationTrackingMode {}

public static final int TRACKING_NONE = 0x00000000;
public static final int TRACKING_FOLLOW = 0x00000004;
public static final int TRACKING_FOLLOW_BEARING = 0x00000008;

getter/setters will look like this:

@UserLocationTrackingMode
public int getUserLocationTrackingMode() {
   return mUserLocationTrackingMode;
}

public void setUserLocationTrackingMode(@UserLocationTrackingMode int userLocationTrackingMode) {
    this.mUserLocationTrackingMode = userLocationTrackingMode;
}

This is a good example how enums can be replaced with primitive types while still keeping the type safety we love about enums. The reason behind this is explained here. I'm suggesting doing the same for other enums in the project.

cc @ljbade @bleege @zugaldia

[tobrun]

I added location change animation for non locational tracking, I felt that this was needed because this will also impact overal performance of the location marker.

Update: had an issue doing gestures and location change at the same time. Resolved this by checking if the system is 'mDirty' and cancelling any ongoing animations

[tobrun]

If we enable location tracking, we should disable updating the Location marker hence it will always be centered in those cases. (this will remove jitter while zooming/rotating)

edit: for zoom this works, but rotate this doesn't, if user tracking is enabled we need to rotate around center of the map instead of center rotation gesture (Google Maps has same UX)

[tobrun]

Calling next method for animating location change conflicts with output from gestures

setCenterCoordinate(location, true /**animated**/);

Note: this is a different animation than mentioned above for non locational tracking. In this case we are animating the map in to place. in the other we are animating the marker in to place.

[tobrun]

Done:

• reintegrated tracking modes
• added animation for user dot for non tracking mode
• changed the way we handle different types of tracking modes.
  • while tracking I'm not updating the userdot location (stays in center screen)
  • provide a way to handle both a dirty as non dirty system (animations)
• adapted enabling/disabling gestures for different types of tracking modes
• fixed bug showing wrong location
• implemented alternative for enums

TODO:

• read correct compass data from sensors (current impl. gets data from tilting device)
• improve location updates
• enable animation for bearing tracking mode for location update
• enable animation for bearing tracking mode for bearing update
• replace bearing GPS marker icon
• cleanup code

Known bugs:

• tracking mode using compass does not work (see first TODO)
• on configuration change the userdot is not correctly positioned while using a tracking mode
  • MapView is not measured yet while using the width/height to postion GpsMarker

@ljbade : need to ask you some things related to enable animation, will ping Monday afternoon.

cc @bleege

[bleege]

@tobrun I really like the Enum alternative that you found. The key for me was that it still allowed for the ease of use (see below). It may be a bit kludgy to setup but that's on our side and not on developers who use the Mapbox SDK. Still makes you wonder why Google didn't provide better compile time support for Enums instead of introducing an annotation.

mMapView.setUserLocationTrackingMode(MapView.TRACKING_NONE);

[bleege]

While testing the latest from 2049-Responsive-User-Tracking I'm getting an incorrect bearing when using either MapView.TRACKING_FOLLOW_BEARING_GPS or MapView.TRACKING_FOLLOW_BEARING_COMPASS and then rotating the device 360 degrees. In other words arrow doesn't move in the direction I'm facing (see first screenshot). Secondly, when I rotate the app the user dot gets stuck in the upper left hand corner of the Map (see second screenshot).

Consistently Points One Way

Stuck After Rotation

[tobrun]

not switching MapView.TRACKING_FOLLOW_BEARING_GPS when rotating device

this is correct, you will only be updated when location has changed, rotating device will probably not trigger this.

MapView.TRACKING_FOLLOW_BEARING_COMPASS

as indicated this is not yet implemented.

[ljbade]

@bleege @tobrun Interesting about enums. I had always wondered why the Android framework exclusively used static ints instead of enums.

@tobrun Perhaps you should only switch from the circle dot to the arrow after receiving the first location update.

[tobrun]

If location tracking with bearing is enabled.

• Should the map change direction in the way you are heading?
• only the GPS marker?

For the former I need to discuss some things with @ljbade (how to hook into surface view animations while handling gestures?) This is also important for getting velocity/scroller/interpolator things in gestures.

cc @bleege

[tobrun]

Need to optimise data: https://youtu.be/1lJec_vfHjs

[tobrun]

In motion sensor docs they advice using a Rotation vector sensor over current setup:

For example, the rotational vector sensor is ideal if you are developing a game, an augmented reality application, a 2-dimensional or 3-dimensional compass, or a camera stabilisation app.

In most cases, using these sensors is a better choice than using the accelerometer and geomagnetic field sensor or the orientation sensor.

[ljbade]

@tobrun Hmm does that precalculate the accelerometer + compass stuff for us? Might be worth testing.

[ljbade]

@tobrun Hmm does that precalculate the accelerometer + compass stuff for us? Might be worth testing.

[bleege]

If location tracking with bearing is enabled.

• Should the map change direction in the way you are heading?
• only the GPS marker?

@tobrun Yes, the map should change and the GPS marker should indicate that "bearing mode" is on.

[tobrun]

Hmm does that precalculate the accelerometer + compass stuff for us? Might be worth testing.

We still need to calculate. Difference is that it uses a gyroscope to produce more stable azimuth values.

After milestone 2.1.0: We need a game plan to handle different algorithms/devices. Not all Android devices will have the sensors mentioned above (see Compatibility definition Android).

[ljbade]

@tobrun Ah yeah thats true. Might need fallback.. Be nice if all this code was part of LOST: https://github.com/mapzen/LOST/issues/30

[tobrun]

I added a extra user tracking mode that combines the compass and GPS bearing. As @ljbade indicated we should expose them separately but I believe users will expect both singular as a combined version.

[ljbade]

@tobrun Perfect, gives developers complete control over how they want there app to behave.

[tobrun]

I moved some things out of this branch needed for android 2.1.0. Currently moved this to 2.2.0 milestone.

What is required to finish this ticket:

• smooth animation when calling setBearing(degrees, true /* animated */)
  • @incanus indicated that we need to sync. frames. @ljbade could you help me out with this?
• correct image + rotation for bearing

[ljbade]

@tobrun Excellent work!

Yeah I can help you with frame sync, I digged into this a while ago but still havn't come a good solution yet. Part of the problem is that TextureView seems to add a one frame lag, and various methods to sync with that that I have tried so far did not work.

[ljbade]

@tobrun I have been playing with Android systrace to get an idea of what happens in our app.

Below is a screen shot with annotations showing our current process to draw a frame to the screen. To record this I had the map open at location with GPS marker, I then rotated the map, started recording, and tapped the compass to start the reset to north animation. Thus there is no gestures involved in this capture.