Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

Deprecate MarkerView #9782

Merged
merged 1 commit into from
Sep 27, 2017
Merged

Deprecate MarkerView #9782

merged 1 commit into from
Sep 27, 2017

Conversation

tobrun
Copy link
Member

@tobrun tobrun commented Aug 16, 2017

This PR deprecates the usage of MarkerView and replaces those use-cases with using a SymbolLayer + SymbolGenerator. This allows to benefit from the options that symbolLayer exposes while using any Android SDK View as iconImage.

  • First commit updates naming of an older example (no refs to sprite in public api of the android binding)
  • Second commit show cases SymbolGenerator (see gif below)
  • Third commit deprecates MarkerView API

ezgif com-video-to-gif 11

cc @mapbox/android

@tobrun tobrun added the Android Mapbox Maps SDK for Android label Aug 16, 2017
@tobrun tobrun added this to the android-v5.2.0 milestone Aug 16, 2017
@tobrun tobrun self-assigned this Aug 16, 2017
@tobrun tobrun requested a review from zugaldia August 16, 2017 11:36
* {@link MapboxMap#addImage(String, Bitmap)} and link the id of the image with
* {@link com.mapbox.mapboxsdk.style.layers.PropertyFactory#iconImage(String)} when setting properties to a SymbolLayer
* with {@link com.mapbox.mapboxsdk.style.layers.SymbolLayer#setProperties(PropertyValue[])} or
* {@link com.mapbox.mapboxsdk.style.layers.SymbolLayer#withProperties(PropertyValue[])}.
Copy link
Member

Choose a reason for hiding this comment

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

Could we add a link to an example using the new proposed approach? Explaining the general approach is great but I think that some working code would make the transition easier.

Copy link
Member

Choose a reason for hiding this comment

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

Maybe CustomSymbolLayerActivity or SymbolGeneratorActivity if they're reference implementations for the new approach?

@@ -50,7 +50,8 @@
<string name="activity_query_rendered_features_box_highlight">Highlight features in box</string>
<string name="activity_query_source_features">Query source features</string>
<string name="activity_symbol_layer">Symbols</string>
<string name="activity_add_sprite">Add Custom Sprite</string>
<string name="activity_add_symbol">Add Custom Symbol</string>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe add a note saying "replaces MarkerView approach"?

@1ec5
Copy link
Contributor

1ec5 commented Aug 16, 2017

Can you speak to the performance of SymbolGenerator compared to MarkerViews with 100, 500, 1,000 markers on the map?

@tobrun
Copy link
Member Author

tobrun commented Aug 17, 2017

MarkerViews are Android SDK views synchronized above our rendering surface (currently SurfaceView, optional to switch to TextureView for increased view synchronization performance). What happens for a MarkerView is that the user inflates a View and we will synchronize its position to a LatLng every time the map changes. This synchronization will result in measure and layout phases with every change.

With SymbolGenerator we measure the view once and create a bitmap of it, no performance hit while using the map. One difference though is that MarkerView are inflated on the fly but with SymbolGenerator you will be creating your views/bitmaps up front. I'm not really seeing this as an issue as this is the same ux as with SymbolLayer.

I will look into creating a more complete example to identify any possible issues with SymbolGenerator initialization when measuring a lot of views. Note that while using the map, the performance will boil down to performance of SymbolLayer which will always outscore MarkerView.

@tobrun tobrun closed this Aug 17, 2017
@tobrun tobrun reopened this Aug 17, 2017
@tobrun
Copy link
Member Author

tobrun commented Aug 17, 2017

I updated the example to create a View and SymbolLayer for each Feature in the GeoJSON (37 features):

ezgif com-video-to-gif 13

I would say this example would show a lot of jiggling/degraded performance with MarkerViews.

@tobrun
Copy link
Member Author

tobrun commented Aug 17, 2017

Did another test with this geojson. This resembles ports in the world and consists out of 1081 points.

ezgif com-video-to-gif 14

You can see this is not acceptable in terms of performance (tested on low end device) though the same use-case implemented with textField shows similar issues:

ezgif com-video-to-gif 15

@tobrun
Copy link
Member Author

tobrun commented Aug 17, 2017

for reference here is a gif with 100 and 1000 markerviews:

ezgif com-video-to-gif 17

Adding 1000 resutls in an ANR.

@1ec5
Copy link
Contributor

1ec5 commented Aug 17, 2017

Thanks for the analysis. So would this be a good characterization of the tradeoff?

MarkerViews:

SymbolGenerator:

What about some of the other behaviors listed in #6181?

@tobrun
Copy link
Member Author

tobrun commented Sep 13, 2017

Capturing from internal conversations that we are planning to put the SymbolGenerator code inside a utils plugin instead of the SDK. I'm going to rework this PR to only deprecate MarkerView and point the deprecation notices to use SymbolLayer instead.

@tobrun tobrun force-pushed the tvn-android-sdk-view-symbollayer branch 3 times, most recently from b73b361 to ed2ae27 Compare September 13, 2017 13:07
@tobrun
Copy link
Member Author

tobrun commented Sep 13, 2017

with ed2ae27 the suggestion from #9782 (comment) has been applied.

cc @zugaldia

@tobrun tobrun changed the title Deprecate MarkerView, introduce SymbolGenerator Deprecate MarkerView Sep 13, 2017
@@ -14,7 +14,10 @@
*
* @param <U> Type of the marker view to be composed.
* @param <T> Type of the builder to be used for composing.
* @deprecated Use a {@link com.mapbox.mapboxsdk.style.layers.SymbolLayer} instead. An example of converting Android
* SDK views to be used as a symbol see https://gist.github.com/tobrun/349777a6ef0dfb55245a544344ee197a.
Copy link
Member

Choose a reason for hiding this comment

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

How about we instead include this approach as an activity in the TestApp while we work on making it available as a plugin?

Copy link
Member Author

Choose a reason for hiding this comment

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

I added SymbolGeneratorActivity to the testapp, replaced all references to the gist with a permalink to an Activity in the testapp.

Copy link
Member

Choose a reason for hiding this comment

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

@tobrun 👍

Copy link
Member Author

Choose a reason for hiding this comment

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

If you are on-board with this change, could you re-review?

@tobrun tobrun force-pushed the tvn-android-sdk-view-symbollayer branch 3 times, most recently from ec6ecaf to 7965438 Compare September 18, 2017 13:42
@tobrun tobrun force-pushed the tvn-android-sdk-view-symbollayer branch 3 times, most recently from f31b667 to 68f32bc Compare September 25, 2017 13:12
@tobrun
Copy link
Member Author

tobrun commented Sep 25, 2017

updated the example to use 1 layer for all different used symbols in
68f32bc

@tobrun tobrun force-pushed the tvn-android-sdk-view-symbollayer branch from 68f32bc to 2a9bae5 Compare September 27, 2017 09:25
@tobrun tobrun merged commit 79c9a7f into master Sep 27, 2017
@tobrun tobrun deleted the tvn-android-sdk-view-symbollayer branch September 27, 2017 10:58
@Guardiola31337 Guardiola31337 mentioned this pull request Oct 6, 2017
20 tasks
@Guardiola31337 Guardiola31337 mentioned this pull request Oct 19, 2017
20 tasks
@Guardiola31337 Guardiola31337 mentioned this pull request Oct 26, 2017
20 tasks
@tobrun tobrun mentioned this pull request Nov 3, 2017
21 tasks
This was referenced Nov 14, 2017
@mapbox mapbox deleted a comment from josileudo Jun 25, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Android Mapbox Maps SDK for Android
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants