Adding pulsing circle to LocationComponent options #172

langsmith · 2020-02-12T21:43:16Z

This pr adds a pulsing option to the Maps SDK's LocationComponent. It is a follow up to mapbox/mapbox-gl-native#13942, which was closed because of our transition away from a mono-repo (more info about that at mapbox/mapbox-gl-native#15971).

langsmith · 2020-03-10T21:50:32Z

There's a crash that I'm experiencing when running this pr's test app examples. The examples work for a while (10-40 seconds), but then there's a 💥 . I believe it's related to the Android system being overloaded when the pulsing duration (. pulsingCircleDuration()) is a low number, meaning the animator runs really rapidly.

Some sort of limit or fix needs to be figured out. I'll try to reproduce again on my device and provide more info soon.

cc @Chaoba @zugaldia @malwoodsantoro @jkevincrowley

zugaldia · 2020-03-10T21:56:07Z

There's a crash that I'm experiencing when running this pr's test app examples.

@Chaoba Would you be able to have a look at it?

Chaoba · 2020-03-11T02:19:01Z

@langsmith I have changed SECOND_LOCATION_CIRCLE_PULSE_DURATION_MS to 80 ms and run the test app for a while, not found the crash issue. So, maybe it's something else cause the crash.

Chaoba

Others look good to me.

MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationAnimatorCoordinator.java

langsmith · 2020-03-30T06:30:19Z

Ok @Chaoba @LukasPaczos , this is good for another round of review 🙇

langsmith · 2020-03-30T15:33:58Z

cc @galinelle as FYI about this being another type of LocationComponent type of styling

Chaoba

LGTM

LukasPaczos

This looks awesome @langsmith 👏

Before we merge I have a couple of comments and issues that I found.

First is, that when we change any of the pulsing options, even if the component is disabled, the animation is started:

Second is a leak:
https://gist.github.com/LukasPaczos/e588678654fec0896f059903d5cc818c

MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationComponent.java

MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationComponentOptions.java

MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationLayerController.java

MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/PulseMode.java

.../java/com/mapbox/mapboxsdk/testapp/activity/location/BasicLocationPulsingCircleActivity.java

langsmith · 2020-04-01T16:04:55Z

Second is a leak:
https://gist.github.com/LukasPaczos/e588678654fec0896f059903d5cc818c

This is now the only remaining thing in @LukasPaczos' comments above, that hasn't been fixed

chloekraw · 2020-04-22T06:33:15Z

@langsmith how is this going? Are you blocked on any particular issue?

LukasPaczos · 2020-04-22T14:38:18Z

.../com/mapbox/mapboxsdk/testapp/activity/location/CustomizedLocationPulsingCircleActivity.java

+    if (locationComponent  != null && locationComponent.isLocationComponentActivated()) {
+      enablePulsing = locationComponent.isLocationComponentEnabled();
+    }


We shouldn't try to implement a client-side solution for:

when we change any of the pulsing options, even if the component is disabled, the animation is started

Instead, the LocationComponent#startPulsingLocationCircle should verify the conditions when the style is applied and avoid showing the layer if the component is disabled.

Just made a new commit e7848ab

We shouldn't try to implement a client-side solution

Refactored and removed that part from the CustomizedLocationPulsingCircleActivity e7848ab#diff-7dcce2d0485478de700d188fc32f455aL144-L147

Instead, the LocationComponent#startPulsingLocationCircle should verify the conditions when the style is applied and avoid showing the layer if the component is disabled.

Whenever startPulsingLocationCircle() is run within LocationComponent.java, options.pulseEnabled() is first checked

mapbox-gl-native-android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationComponent.java

Lines 729 to 731 in e7848ab

if (options.pulseEnabled()) {

startPulsingLocationCircle();

} else {

isEnabled() is in reference to the overall LocationComponent:

mapbox-gl-native-android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationComponent.java

Lines 742 to 745 in e7848ab

if (isEnabled) {

locationAnimatorCoordinator.startLocationComponentCirclePulsing(options);

locationLayerController.adjustPulsingCircleLayerVisibility(true);

}

LukasPaczos

I'm not able to reproduce the leak anymore. Have you been able to narrow it down and fix it? It might also be directly connected to the solution for https://github.com/mapbox/mapbox-gl-native-android/pull/172/files#r413040504.

When the above is address, we'll need to work on rebasing this on top of the master. Let me know if you need any help with that.

…erface parameter

langsmith · 2020-04-27T05:26:13Z

I'm not able to reproduce the leak anymore

I still get the leak on my physical device 🤷 Maybe it's device specific? Will see what happens on an emulated device.

LukasPaczos

Could you add incompatibility doc entry to:

mapbox-gl-native-android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationComponentActivationOptions.java

Line 138 in 5cee66a

public boolean useSpecializedLocationLayer() {

and

mapbox-gl-native-android/MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationComponentActivationOptions.java

Line 253 in 5cee66a

    
           public Builder useSpecializedLocationLayer(boolean useSpecializedLocationLayer) {

I've spent a fair amount of time trying to track down the leak. The newly added animator seems to be canceled correctly, I'm unable to spot any issues.

The reproduction of the leak has been completely flaky for me, sometimes I've been able to reproduce it many time in the row, then it stopped being reproducible and I had to reinstall the app. And the cycle repeats. I've never been able to reproduce with the debugger attached. Any ideas @mapbox/maps-android?

I've also found an unrelated performance issue - https://github.com/mapbox/mapbox-gl-native/issues/16439.

MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationComponent.java

MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationComponentOptions.java

…/LocationComponentOptions.java Co-Authored-By: Łukasz Paczos <lukasz.paczos@mapbox.com>

…/LocationComponent.java Co-Authored-By: Łukasz Paczos <lukasz.paczos@mapbox.com>

…/LocationComponentOptions.java Co-Authored-By: Łukasz Paczos <lukasz.paczos@mapbox.com>

langsmith · 2020-04-27T15:08:25Z

incompatibility doc entries added ✔️

LukasPaczos

Great work @langsmith!

langsmith added the 🚧 WIP issue or pull request is a work in progress label Feb 12, 2020

langsmith force-pushed the ls-adding-pulsing-locationcomponent-circle branch from 5808c0f to c6e3460 Compare March 10, 2020 21:59

zugaldia requested a review from Chaoba March 11, 2020 17:08

Chaoba reviewed Mar 12, 2020

View reviewed changes

MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location/LocationAnimatorCoordinator.java Outdated Show resolved Hide resolved

langsmith force-pushed the ls-adding-pulsing-locationcomponent-circle branch 3 times, most recently from 682a0b5 to cc91ef1 Compare March 27, 2020 06:10

langsmith self-assigned this Mar 29, 2020

langsmith added the feature 🍏 New feature or request label Mar 29, 2020

langsmith force-pushed the ls-adding-pulsing-locationcomponent-circle branch 2 times, most recently from 34e6599 to 47e961b Compare March 30, 2020 05:56

langsmith marked this pull request as ready for review March 30, 2020 06:29

langsmith removed the 🚧 WIP issue or pull request is a work in progress label Mar 30, 2020

zugaldia requested review from Chaoba and LukasPaczos March 30, 2020 19:49

Chaoba approved these changes Mar 31, 2020

View reviewed changes

LukasPaczos suggested changes Mar 31, 2020

View reviewed changes

langsmith force-pushed the ls-adding-pulsing-locationcomponent-circle branch 2 times, most recently from 40767b5 to 0d2ec13 Compare April 1, 2020 03:28

chloekraw added this to the release-water milestone Apr 6, 2020

langsmith force-pushed the ls-adding-pulsing-locationcomponent-circle branch from f099ffd to 5ccf6d2 Compare April 7, 2020 00:32

LukasPaczos reviewed Apr 22, 2020

View reviewed changes

Langston Smith added 4 commits April 22, 2020 15:58

refactoring to swap interpolator string paramete for interpolator int…

6a4f038

…erface parameter

LocationLayerControllerTest adjustments

4b93fc6

removing unneeded test

2096cbd

more rebase adjustments

02e5dc0

langsmith force-pushed the ls-adding-pulsing-locationcomponent-circle branch from 5ccf6d2 to 02e5dc0 Compare April 22, 2020 23:46

Langston Smith added 3 commits April 22, 2020 18:32

[location] added pulsing locationcomponent circle

206fc07

refactoring to swap interpolator string paramete for interpolator int…

0ea0ac6

…erface parameter

consolidating pulse ui update methods

dfd9c63

langsmith force-pushed the ls-adding-pulsing-locationcomponent-circle branch from f2fa043 to dfd9c63 Compare April 23, 2020 01:43

Langston Smith added 2 commits April 23, 2020 09:33

gl-native vendor bump

0b3bc9f

refactor to check for activation with locationcomponent

e7848ab

LukasPaczos reviewed Apr 27, 2020

View reviewed changes

Langston Smith and others added 6 commits April 27, 2020 06:47

Update MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location…

7a681b5

…/LocationComponentOptions.java Co-Authored-By: Łukasz Paczos <lukasz.paczos@mapbox.com>

Update MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location…

6008acd

…/LocationComponent.java Co-Authored-By: Łukasz Paczos <lukasz.paczos@mapbox.com>

Update MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location…

cabcb4a

…/LocationComponentOptions.java Co-Authored-By: Łukasz Paczos <lukasz.paczos@mapbox.com>

Update MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location…

12e411c

…/LocationComponentOptions.java Co-Authored-By: Łukasz Paczos <lukasz.paczos@mapbox.com>

Update MapboxGLAndroidSDK/src/main/java/com/mapbox/mapboxsdk/location…

0082d69

…/LocationComponentOptions.java Co-Authored-By: Łukasz Paczos <lukasz.paczos@mapbox.com>

Added incompatibility docs

3c051b6

Langston Smith and others added 2 commits April 27, 2020 08:13

Added pulse alpha range explanation

ed5d513

Do not wrap pulsing circle animator with an animator set to avoid leaks

07ec52f

LukasPaczos approved these changes Apr 30, 2020

View reviewed changes

LukasPaczos merged commit 9bfe77d into master Apr 30, 2020

LukasPaczos deleted the ls-adding-pulsing-locationcomponent-circle branch April 30, 2020 10:28

This was referenced Apr 30, 2020

[android] Adding pulsing circle to LocationComponent options mapbox/mapbox-gl-native#13942

Closed

Add pulsing LocationComponent example mapbox/mapbox-android-demo#1336

Closed

langsmith mentioned this pull request May 11, 2020

Added pulsing LocationComponent examples mapbox/mapbox-android-demo#1344

Merged

langsmith mentioned this pull request May 20, 2020

Added a value animator to the vanishing route line to improve the vis… mapbox/mapbox-navigation-android#3010

Merged

10 tasks

langsmith mentioned this pull request Jun 26, 2020

Restore pulsing location example mapbox/mapbox-android-demo#1060

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Adding pulsing circle to LocationComponent options #172

Adding pulsing circle to LocationComponent options #172

langsmith commented Feb 12, 2020

langsmith commented Mar 10, 2020

zugaldia commented Mar 10, 2020

Chaoba commented Mar 11, 2020

Chaoba left a comment

langsmith commented Mar 30, 2020

langsmith commented Mar 30, 2020

Chaoba left a comment

LukasPaczos left a comment

langsmith commented Apr 1, 2020

chloekraw commented Apr 22, 2020

LukasPaczos Apr 22, 2020

langsmith Apr 23, 2020

LukasPaczos left a comment

langsmith commented Apr 27, 2020

LukasPaczos left a comment

langsmith commented Apr 27, 2020

LukasPaczos left a comment

	if (options.pulseEnabled()) {
	startPulsingLocationCircle();
	} else {

	if (isEnabled) {
	locationAnimatorCoordinator.startLocationComponentCirclePulsing(options);
	locationLayerController.adjustPulsingCircleLayerVisibility(true);
	}

Adding pulsing circle to LocationComponent options #172

Adding pulsing circle to LocationComponent options #172

Conversation

langsmith commented Feb 12, 2020

langsmith commented Mar 10, 2020

zugaldia commented Mar 10, 2020

Chaoba commented Mar 11, 2020

Chaoba left a comment

Choose a reason for hiding this comment

langsmith commented Mar 30, 2020

langsmith commented Mar 30, 2020

Chaoba left a comment

Choose a reason for hiding this comment

LukasPaczos left a comment

Choose a reason for hiding this comment

langsmith commented Apr 1, 2020

chloekraw commented Apr 22, 2020

LukasPaczos Apr 22, 2020

Choose a reason for hiding this comment

langsmith Apr 23, 2020

Choose a reason for hiding this comment

LukasPaczos left a comment

Choose a reason for hiding this comment

langsmith commented Apr 27, 2020

LukasPaczos left a comment

Choose a reason for hiding this comment

langsmith commented Apr 27, 2020

LukasPaczos left a comment

Choose a reason for hiding this comment