Mapbox-gl-native: Expose a generic Android View Marker API

[tobrun]

Current Marker API on android is fairly limited because Markers are Gl-drawn components. I'm hearing about requests for animating markers or having the ability to do more typical Android SDK visual things with it (animations, selectors etc.).

Concurrent InfoWindows from #3127 and UserLocationView showcased that we are able to sync Android Views with the underlying MapView. The logic is their but we do not expose a generic system.

API proposal:

For native annotations views in the Android Mapbox SDK. I don't see any reason to stray far from the adapter concept what an Android developer uses on daily basis. Combining this with the existing Mapbox API for adding annotations will look like:

 // combined API
 addMarker();
 addMarkers();
 removeMarker();
 removeMarkers();
 selectMarker();

 // new API
 setMarkerViewAdapter();
 setOnMarkerViewClickListener();
 setMarkerViewItemAnimation();

Example API

Integration of the API will follow the same path as the current Marker API.

  // Add country markers
  List<BaseMarkerOptions> countries = new ArrayList<>();
  countries.add(new CountryMarkerOptions().title("China").abbrevName("ch").flagRes(R.drawable.ic_china).position(new LatLng(31.230416, 121.473701)));
  countries.add(new CountryMarkerOptions().title("United States").abbrevName("us").flagRes(R.drawable.ic_us).position(new LatLng(38.907192, -77.036871)));
  countries.add(new CountryMarkerOptions().title("Brazil").abbrevName("br").flagRes(R.drawable.ic_brazil).position(new LatLng(-15.798200, -47.922363)));
  countries.add(new CountryMarkerOptions().title("Germany").abbrevName("de").flagRes(R.drawable.ic_germany).position(new LatLng(52.520007, 13.404954)));
  mapboxMap.addMarkers(countries);

On top of that it will expose some new APIs to make ViewMarkers possible:

  // Add view marker adapter
  mapboxMap.setMarkerViewAdapter(new CountryAdapter(this));

  // Add a view marker click listener
  mapboxMap.setOnMarkerViewClickListener(new MapboxMap.OnMarkerViewClickListener() {
            @Override
            public void onMarkerClick(@NonNull Marker marker, @NonNull View view) {
                Log.d(MapboxConstants.TAG, "Country clicked " + ((CountryMarker) marker).getAbbrevName());
            }
        });

An example of the MarkerViewAdapter shown above:

    private static class CountryAdapter implements MapboxMap.MarkerViewAdapter<CountryMarker> {

        private LayoutInflater inflater;

        public CountryAdapter(@NonNull Context context) {
            this.inflater = LayoutInflater.from(context);
        }

        @Nullable
        @Override
        public View getView(@NonNull CountryMarker marker, @Nullable View convertView, @NonNull ViewGroup parent) {
            ViewHolder viewHolder;
            if (convertView == null) {
                viewHolder = new ViewHolder();
                convertView = inflater.inflate(R.layout.view_custom_marker, parent, false);
                viewHolder.flag = (ImageView) convertView.findViewById(R.id.imageView);
                viewHolder.abbrev = (TextView) convertView.findViewById(R.id.textView);
                convertView.setTag(viewHolder);
            } else {
                viewHolder = (ViewHolder) convertView.getTag();
            }
            viewHolder.flag.setImageResource(marker.getFlagRes());
            viewHolder.abbrev.setText(marker.getAbbrevName());
            return convertView;
        }

        private static class ViewHolder {
            ImageView flag;
            TextView abbrev;
        }
    }

Next steps:

• [x] Basic tracking view on Mapview - @tobrun
• [x] Research limitations + perf. impacts - @tobrun
• [x] Test out Android SDK animations - @tobrun
• [x] Look into different approaches to publicly expose the API - @tobrun
• [x] Introduce basic View Adapter approach - @tobrun
• [x] Use a View reuse pattern in adapter (SimplePool) - @tobrun
• [x] Integrate with selectMarker(Marker m) API - @tobrun
• [x] Integrate with removeMarker(Marker m) API - @tobrun
• [x] Hide old GL marker - @tobrun
• [x] Trigger invalidate on View markers when Map is fully loaded - @tobrun
• [x] Validate is exposed Android View Marker API matches iOS View Annotation API - @tobrun
• [x] Profile execution - @tobrun
• [x] Animation system - @tobrun
• [x] Select animation API - @tobrun
• [x] allow adding multiple adapters - @tobrun
• [x] Flat configurable
• [x] Offset configurable
• [x] Double check changes made to basic Marker API - @tobrun
• [ ] API documentation - @tobrun

@incanus @bleege @zugaldia @cammace @danswick

[zugaldia]

Related: https://github.com/mapbox/mapbox-gl-native/issues/3116

[tobrun]

Related: #1125

[tobrun]

To make this a bit more concrete:

Concurrent InfoWindows from #3127 and UserLocationView showcased that we are able to sync Android Views with the underlying MapView

The syncing is performed in these following lines of code:

        @Override
        public void onSurfaceTextureUpdated(SurfaceTexture surface) {
            mCompassView.update(getDirection());
            mUserLocationView.update();
            for (InfoWindow infoWindow : mInfoWindows) {
                infoWindow.update();
            }
        }

Instead of updating these Views one by one,
I would like to iterate a List<View> an update every entry.
An entry will be updated based on type of interface implemented by the developer.
This interface is responsible for defining update behaviour (eg. following a certain LatLng, expose an update method, ... etc.). We than can also expose add/remove methods for end developers.

[tobrun]

Been doing some tests to see the performance hit when adding new annotations.
All benchmarks have been run on minZoom level with some panning.

• No markers

• 1 marker

• 10 markers

• 20 markers

• 36 markers

• +100 markers

[tobrun]

Previous test was using a basic View with only a background. I'm thinking about 3 use cases that needs some extra investigation. Bigger size views, a more complex ViewHierarchy and animated Views.

[tobrun]

Bigger size views

• 20 markers 12px

• 20 markers 24px

• 20 markers 48px
  

Conclusion

There is no clear relation between the size of the View and GPU performance.

[tobrun]

more complex ViewHierarchy

• no extra parent viewgroup

• 1 extra parent viewgroup

• 3 extra parent viewgroup

Conclusion

It adds some extra overhead having a more complex View hierarchy.

[tobrun]

Complex View

• View with color background

• TextView with text and color background
  
• ImageView with icon and color background
  

Conclusion

There is a clear relation between the decrease in performance when showing a more complex view.

[tobrun]

Animations

1 alpha animation with 20 view markers

10 alpha animations with 20 view markers

20 alpha animations with 20 view markers

Conclusion

Even having 1 animated View marker results in dropping frames

[tobrun]

FWIW: a test with same view animation without a MapView also drops in dropping frames occasionally:

[1ec5]

The iOS/OS X analogue to this issue is #1784.

[tobrun]

There is some internal discussion going on how we are going to expose this. On Android we are not committing to a public API just yet and we are following the solution iOS is taking.

Before looking into #4694 / #4384, I'm currently thinking about taking #2835 for a spin and see if it improves any use cases shown above.

[tobrun]

I have been looking at the WIP branch from @boundsj. What I'm noticing, which is different from my initial idea, is that a platform annotation is associated to the same model as a gl annotation. I'm going for a similar approach here. One difference will be the usage of MarkerView naming instead of AnnotationView (Gmaps vs Mapkit naming).

[tobrun]

Been looking into animation support by applying the basic 4 animators Android SDK exposes:

• Translate
• Alpha
• Rotate
• Scale

This resulted in following gif:

When the map is idle this works great, only when you start panning the map, you see the following:

There is an issue with the translate animator because it with not use the current x,y position but will calculate the interpolated values from what you have passed initially. I don't really consider this an issue, since users will be able to use a LatLng animator instead as implemented in AnimatedMarkerActivity.

[tobrun]

Next up is looking into how we are going to expose this as an API.

On top of my head these are the current options:

• setter through MarkerOptions
• setter through Marker
• adapter approach similar to InfoWindowAdapter
• ...

MarkerOptions

The problem with integration into the Marker builder is that the members must be Parceable which an Android SDK View isn't. Currently no going to look any further into this.

Marker

What you see in previous comment is done through Marker.setMarkerView but I'm personally not big fan of this setup because it defeats the purpose of having a builder pattern. It's also not very visible or intuitive for the end developer.

Adapter

I'm currently thinking about applying an adapter pattern. At some point we could implement something as a View pool and adapt Views from that pool to a marker on the screen. This requires that we at some point release an adapted view back to the pool. In other words we should create a system that only shows a View if it's found in current viewport, if not it should be released to the View pool. This would be a big win for performance since we can recycle views instead of constantly inflating new ones+ we are also not required to update out of viewport view annotations. Both these arguments could become requirements when marker clustering lands.

At this point I'm just thinking about providing an interface as public API that at some point could be converted to an adapter approach. The downside for this simplistic approach is that the interface will be called instantly when adding a marker.

Note:
From what I have seen in the iOS code is that they apply a delegation pattern.
I feel the adapter approach would result in a similar experience for the end developer.

[tobrun]

While implementing the Adapter approach I initially provided a flag in MarkerOptions to indicate if a Marker is a ViewMarker (syntax: new MarkerOptions().viewMarker(true)). This will enforce that only ViewMarkers will receive a the adapter callback. Now I'm thinking about removing this because intent for end developer is not clear + smells like a premature optimisation. Might revisit this later.

[zugaldia]

I'm currently thinking about applying an adapter pattern.

+1 for the pattern, it also brings consistency with other elements of the SDK (e.g. InfoWindow).

[tobrun]

When implementing the simplistic adapter approach, I'm using:

      // adapt GL-marker to View-marker
        mapboxMap.setMarkerViewAdapter(new MapboxMap.MarkerViewAdapter() {
            @Nullable
            @Override
            public MarkerView getView(@NonNull Marker marker) {
                if (marker.equals(brazil)) {
                    return createMarkerView("br", R.drawable.ic_brazil);
                } else if (marker.equals(germany)) {
                    return createMarkerView("de", R.drawable.ic_germany);
                } else if (marker.equals(china)) {
                    return createMarkerView("ch", R.drawable.ic_china);
                } else if (marker.equals(us)) {
                    return createMarkerView("us", R.drawable.ic_us);
                }
                return null;
            }
        });

        // add markers
        china = mapboxMap.addMarker(new MarkerOptions().position(new LatLng(31.230416, 121.473701)));
        brazil = mapboxMap.addMarker(new MarkerOptions().position(new LatLng(-15.798200, -47.922363)));
        us = mapboxMap.addMarker(new MarkerOptions().position(new LatLng(38.907192, -77.036871)));
        germany = mapboxMap.addMarker(new MarkerOptions().position(new LatLng(52.520007, 13.404954)));

But this is currently not usable since the MarkerViewAdapter.getView is called inside the addMarker method so I can't compare markers in getView since those markers are non existent at that time.

update: the only good solution to workaround this issue would be to properly adapt views based on the markers shown on screen with getAnnotationsInBounds in conjuction with getVisibleBounds. This would result in calling MarkerViewAdapter.getView at a later time.

[tobrun]

I touched base briefly with @boundsj and he indicated iOS is moving towards the same solution (View reuse). The difficult part is how we are going to be aware of Marker entering and leaving the screen, a couple of possible solutions can be found in #4694 / #4384.

[tobrun]

I'm back running at this. Been looking at progress on iOS side in https://github.com/mapbox/mapbox-gl-native/pull/4801/commits/4db0d0f82352c453ff47fae6fcf6b66f078404ec and seeing that they are updating the view annotations as part of a map change event:

case mbgl::MapChangeRegionIsChanging:
    ....
           [self updateAnnotationViews];
    ....            
}            

More details about the updateAnnotationsViews method can be found here

Next steps for android is getting a similar system in place that will track which markers are currently visible in the viewport. On android this could give some performance overhead since we need to pass through jni to get the view annotations in bounds. Once we know which are found in the viewport and which aren't we can look into applying the view reuse/adapter approach.

[1ec5]

Note that you need to update in response to MapChangeRegionDidChange in addition to MapChangeRegionIsChanging. The last MapChangeRegionIsChanging notification in an animation is suppressed in favor of the MapChangeRegionDidChange notification.

[tobrun]

With above implemented it's behaving in similar way as onSurfaceUpdate callback as seen in this log, Next items to tackle is testing out rate limiting the discovery of new markers and implementing a View Pool to reuse View objects. This pool will be used as the base for our Adapter implementation.

[tobrun]

On top of exposing a MarkerViewAdapter, I'm also thinking about exposing an abstract BaseMarkerVIewAdapter.

[zugaldia]

On top of exposing a MarkerViewAdapter, I'm also thinking about exposing an abstract BaseMarkerVIewAdapter

For the dev to implement its own, or for other annotations to share?

[tobrun]

@zugaldia for the dev to implement his own. I'm still figuring out the what and who related to public API. Main idea is that the user can add markers through maboxMap.addMarker(markerOptions) and that he can specify an adapter that exposes something as:

    public interface MarkerViewAdapter<U extends Marker> {

        @Nullable
        View getView(@NonNull U marker, @Nullable View convertView, @NonNull ViewGroup parent);
    }

The view returned above is the one that will be shown as the Marker.

[tobrun]

An example from above would look like:

    @Override
    public void onMapReady(MapboxMap mapboxMap) {
        List<BaseMarkerOptions> countries = new ArrayList<>();
        countries.add(new CountryMarkerOptions().abbrevName("ch").flagRes(R.drawable.ic_china).position(new LatLng(31.230416, 121.473701));
        countries.add(new CountryMarkerOptions().abbrevName("us").flagRes(R.drawable.ic_us).position(new LatLng(38.907192, -77.036871));
        countries.add(new CountryMarkerOptions().abbrevName("br").flagRes(R.drawable.ic_brazil).position(new LatLng(-15.798200, -47.922363));
        countries.add(new CountryMarkerOptions().abbrevName("de").flagRes(R.drawable.ic_germany).position(new LatLng(52.520007, 13.404954));
        mapboxMap.addMarkers(countries);

        mapboxMap.setMarkerViewAdapter(new CountryAdapter(this));
    }

    private static class CountryAdapter implements MapboxMap.MarkerViewAdapter<CountryMarker> {

        private LayoutInflater inflater;

        public CountryAdapter(@NonNull Context context) {
            this.inflater = LayoutInflater.from(context);
        }

        @Nullable
        @Override
        public View getView(@NonNull CountryMarker marker, @Nullable View convertView, @NonNull ViewGroup parent) {
            ViewHolder viewHolder;
            if (convertView == null) {
                viewHolder = new ViewHolder();
                convertView = inflater.inflate(R.layout.view_custom_marker, parent, false);
                viewHolder.flag = (ImageView) convertView.findViewById(R.id.imageView);
                viewHolder.abbrev = (TextView) convertView.findViewById(R.id.textView);
                convertView.setTag(viewHolder);
            } else {
                viewHolder = (ViewHolder) convertView.getTag();
            }
            viewHolder.flag.setImageResource(marker.getFlagRes());
            viewHolder.abbrev.setText(marker.getAbbrevName());
            return convertView;
        }

        private static class ViewHolder {
            ImageView flag;
            TextView abbrev;
        }
    }

[tobrun]

In the commits above I migrated to a LongSparseArray as it's an optimised version of the Map interface on the Android platform. I'm using the Marker id as key and the related view is value. I'm currently able to instantiate the views through the adapter interface when markers show up in the viewport. Next steps is being able to remove them when panning out the viewport + introducing view reusability.

[tobrun]

Here is the output when zooming out on a map and discovering new markers along the way:

5-04 04:10:04.831 9421-9421/ V/MapboxMap: Amount of annotations: 2
05-04 04:10:04.866 9421-9421/ V/MapboxMap: SurfaceTexture has been updated
05-04 04:10:04.907 9421-9421/ V/MapboxMap: SurfaceTexture has been updated
05-04 04:10:09.583 9421-9421/ V/MapboxMap: Already added 1
05-04 04:10:09.583 9421-9421/ V/MapboxMap: Already added 2
05-04 04:10:09.583 9421-9421/ V/MapboxMap: Adding 3
05-04 04:10:09.583 9421-9421/ V/MapboxMap: Amount of annotations: 3
05-04 04:10:09.622 9421-9421/ V/MapboxMap: SurfaceTexture has been updated
05-04 04:10:09.625 9421-9421/ V/MapboxMap: Already added 1
05-04 04:10:09.625 9421-9421/ V/MapboxMap: Already added 2
05-04 04:10:09.625 9421-9421/ V/MapboxMap: Already added 3
05-04 04:10:09.625 9421-9421/ V/MapboxMap: Amount of annotations: 3
05-04 04:10:09.631 9421-9421/ V/MapboxMap: Already added 1
05-04 04:10:09.631 9421-9421/ V/MapboxMap: Already added 2
05-04 04:10:09.631 9421-9421/ V/MapboxMap: Already added 3
05-04 04:10:09.631 9421-9421/ V/MapboxMap: Amount of annotations: 3
05-04 04:10:09.670 9421-9421/ V/MapboxMap: SurfaceTexture has been updated
05-04 04:10:09.717 9421-9421/ V/MapboxMap: SurfaceTexture has been updated
05-04 04:10:09.724 9421-9421/ V/MapboxMap: Already added 1
05-04 04:10:09.724 9421-9421/ V/MapboxMap: Already added 2
05-04 04:10:09.724 9421-9421/ V/MapboxMap: Already added 3
05-04 04:10:09.724 9421-9421/ V/MapboxMap: Amount of annotations: 3
05-04 04:10:09.777 9421-9421/ V/MapboxMap: SurfaceTexture has been updated
05-04 04:10:09.873 9421-9421/ V/MapboxMap: SurfaceTexture has been updated
05-04 04:10:09.904 9421-9421/ V/MapboxMap: Already added 1
05-04 04:10:09.904 9421-9421/ V/MapboxMap: Already added 2
05-04 04:10:09.904 9421-9421/ V/MapboxMap: Already added 3
05-04 04:10:09.904 9421-9421/ V/MapboxMap: Amount of annotations: 3
05-04 04:10:09.917 9421-9421/ V/MapboxMap: SurfaceTexture has been updated
05-04 04:10:09.950 9421-9421/ V/MapboxMap: Already added 1
05-04 04:10:09.950 9421-9421/ V/MapboxMap: Already added 2
05-04 04:10:09.950 9421-9421/ V/MapboxMap: Already added 3
05-04 04:10:09.950 9421-9421/ V/MapboxMap: Amount of annotations: 3
05-04 04:10:09.986 9421-9421/ V/MapboxMap: SurfaceTexture has been updated
05-04 04:10:09.991 9421-9421/ V/MapboxMap: Already added 1
05-04 04:10:09.991 9421-9421/ V/MapboxMap: Already added 2
05-04 04:10:09.991 9421-9421/ V/MapboxMap: Already added 3
05-04 04:10:09.991 9421-9421/ V/MapboxMap: Amount of annotations: 3
05-04 04:10:10.024 9421-9421/ V/MapboxMap: SurfaceTexture has been updated
05-04 04:10:11.023 9421-9421/ V/MapboxMap: SurfaceTexture has been updated
05-04 04:10:11.175 9421-9421/ V/MapboxMap: Adding 0
05-04 04:10:11.175 9421-9421/ V/MapboxMap: Already added 1
05-04 04:10:11.175 9421-9421/ V/MapboxMap: Already added 2
05-04 04:10:11.175 9421-9421/ V/MapboxMap: Already added 3
05-04 04:10:11.175 9421-9421/ V/MapboxMap: Amount of annotations: 4

I'm going to see if I can implement above logic on a workerthread to free the uithread for rendering.

update: the map feels a bit more responsive after introducing a AsyncTask but we are still executing this almost every second while panning. Rate limiting the executions will be the right thing to do for discoverability.

update: I added rate limiting to these bounds calculations, currently it will calculate the different bounds in rates of 300ms.

[tobrun]

Next up is the view reuse. The basic idea is that Views who are no longer shown in the viewport will be released to a SimplePool<View>. When we require having to show new view markers we will poll the pool to reuse that old View instead.

update: The following logs indicate we are reusing the view through the pool. First convertview are null at initialisation, after panning a view marker out of the viewport and back in we can see that the view has been reused.

05-04 05:39:32.207 32454-309/ V/MapboxMap: Adding 1 with convertView null
05-04 05:39:32.207 32454-309/ V/MapboxMap: Adding 2 with convertView null
05-04 05:39:32.207 32454-309/ V/MapboxMap: Adding 3 with convertView null
05-04 05:39:32.208 32454-309/ V/MapboxMap: Amount of annotations: 0
05-04 05:39:32.247 32454-32454/ V/MapboxMap: SurfaceTexture has been updated
05-04 05:39:33.107 32454-338/ V/MapboxMap: Already added 1
05-04 05:39:33.107 32454-338/ V/MapboxMap: Already added 2
05-04 05:39:33.107 32454-338/ V/MapboxMap: Already added 3
05-04 05:39:33.107 32454-338/ V/MapboxMap: Amount of annotations: 3
05-04 05:39:33.144 32454-32454/ V/MapboxMap: SurfaceTexture has been updated
05-04 05:39:33.428 32454-32485/ V/MapboxMap: Adding 0 with convertView null
05-04 05:39:33.428 32454-32485/ V/MapboxMap: Already added 1
05-04 05:39:33.428 32454-32485/ V/MapboxMap: Already added 2
05-04 05:39:33.428 32454-32485/ V/MapboxMap: Already added 3
05-04 05:39:33.428 32454-32485/ V/MapboxMap: Amount of annotations: 3
05-04 05:39:33.466 32454-32454/ V/MapboxMap: SurfaceTexture has been updated
05-04 05:39:33.841 32454-32687/ V/MapboxMap: Already added 0
05-04 05:39:33.841 32454-32687/ V/MapboxMap: Already added 1
05-04 05:39:33.841 32454-32687/ V/MapboxMap: Already added 2
05-04 05:39:33.841 32454-32687/ V/MapboxMap: Already added 3
05-04 05:39:33.841 32454-32687/ V/MapboxMap: Amount of annotations: 4
05-04 05:39:33.884 32454-32454/ V/MapboxMap: SurfaceTexture has been updated
05-04 05:39:34.159 32454-32710/ V/MapboxMap: Already added 1
05-04 05:39:34.159 32454-32710/ V/MapboxMap: Already added 2
05-04 05:39:34.159 32454-32710/ V/MapboxMap: Already added 3
05-04 05:39:34.159 32454-32710/ V/MapboxMap: Removing 0 : android.support.v4.util.Pools$SimplePool@70925a5
05-04 05:39:34.159 32454-32710/ V/MapboxMap: Amount of annotations: 3
05-04 05:39:34.165 32454-32454/ V/MapboxMap: SurfaceTexture has been updated
05-04 05:39:34.262 32454-32454/ V/MapboxMap: SurfaceTexture has been updated
05-04 05:39:34.331 32454-32454/ V/MapboxMap: SurfaceTexture has been updated
05-04 05:39:34.390 32454-32454/ V/MapboxMap: SurfaceTexture has been updated
05-04 05:39:34.461 32454-32454/ V/MapboxMap: SurfaceTexture has been updated
05-04 05:39:34.493 32454-309/ V/MapboxMap: Adding 0 with convertView android.widget.RelativeLayout{afb7d7a V.E...... ........ 0,0-168,168}

[tobrun]

there seems to be something off with my id management..
after panning a bit back and forth I'm seeing this:

update after debugging a bit, I'm seeing that the MapView child count can grow from 9 to 13. This explains a bit what we are seeing but this should not be possible.

[tobrun]

One thing I'm noticing is that calling getAnnotationsInBounds for the whole world view map results in not correctly returned ID's. I'm thinking there is a date line issue there.

[tobrun]

I'm also thinking that https://github.com/mapbox/mapbox-gl-native/issues/3276#issuecomment-216896158 is caused by having a multiple callback system in my Asynctask. I'm going to optimise my Asynctask to return a model object containing state information on which markers need to be added/removed.

update: single result callback is looking like:

    void setViewMarkersBoundsTaskResult(MapView.MarkerInBoundsTask.Result result) {
        Map<Marker, View> outBoundsMarker = result.getOutBounds();
        View convertView;
        for (Map.Entry<Marker, View> outBoundsEntry : outBoundsMarker.entrySet()) {
            convertView = outBoundsEntry.getValue();
            if (convertView != null) {
                convertView.setVisibility(View.GONE);
                viewSimplePool.release(convertView);
                mMarkerViews.remove(outBoundsEntry.getKey().getId());
            }
        }

        List<Marker> inBoundsMarkers = result.getInBounds();
        for (Marker marker : inBoundsMarkers) {
            convertView = viewSimplePool.acquire();
            View adaptedView = mMarkerViewAdapter.getView(marker, convertView, mMapView);
            mMarkerViews.append(marker.getId(), adaptedView);
            if (convertView == null) {
                mMapView.addView(adaptedView);
            } else {
                convertView.setVisibility(View.VISIBLE);
            }
        }
    }

update: now running into problems related to concurrent asynctask being executed. Only one asynctask may be run at the same time, this should be managed in combination with rate limited updates.

update:
above in place resolves the IllegalStateExcepetion related to releasing a View object to the pool

[tobrun]

I'm noticing that when a view comes into bounds and made visible you can see it animating in that place. This is because we are setting visibility before updating it's position. I'm now going to move the visibility part after updating the position and see if that delivers a better ux.

update: now they aren't _flying_ into place anymore

[tobrun]

A basic implementation of the adapter approach landed last week. Now we are going to start tacking the next item on the list: selectMarker. Their are a couple of things to be covered by this:

• [x] a clicked ViewMarker becomes a selected Marker
• [x] onClick needs to be invoked when a marker becomes selected
• [x] look into ViewReuse - onClick integration (research needed)

update: I think it's valid to immediately start looking into the Android SDK Adapter solution to this problem when it comes to view reuse. Defining an OnClickListener on adapted object will not survive view reuse and will results in wrongly set callbacks. In the Android SDK they have resolved this by managing the OnClickListener themselves and expose an OnItemClickListener instead.

[tobrun]

removeMarker is now also implemented. When we call remove marker the adapted View will be hidden, released to the SimplePool and it's entry in the LongSparseArray will be removed.

[tobrun]

Next item on the list is trigger invalidate when map is fully loaded. Currently when you start up the View Marker API Activity no View Markers are shown until the user starts interacting with the map. Need to look into invalidating them during map loading. I'm seeing 2 possibilities:

• add logic to current reloadMarkers
• hook into other mapchange event

update: due to async behaviour and being able to invalidate when a new adapter has been set. I'm adding this as a step in the setMarkerViewAdapter(). This will trigger a lookup to determine which annotations are in bounds.

[tobrun]

I implemented Hide old GL marker and started to look at profiling the code and see if we can find some performance bottlenecks. First step taken was to convert the BulkMarkerActivity to work with ViewMarkers.

I'm currently thinking the AsyncTask is causing more overhead than it's actually beneficial. In cf97b2e I have disabled and should be revisited with profiling.

[tobrun]

First steps taken to profile the code is using Android SDK tools:

• TraceView
• Systrace

Note that this only results in profiling the java code of this project.
Will need to look into the NDK profiling support to see what is possible there.

[tobrun]

TraceView

When interacting (panning) with the map we are seeing the following pattern:

The recurring blue bar is most notabel and contains the following definition:

Looking into the details this is the render method from NativeMapView and it takes almost up to 10 ms to execute. We will need to look into C++ debugging to know what is executed in the render method. This behaviour is not related to ViewMarkers, the next 2 methods showcase the ViewMarker API:

• InvalidateViewMarkers:

• Conclusion: Most time is spent on Log and setting the Async result of discovering annotations that are in bounds. Need to check if having/not-having an AsyncTask changes this.
  
  
  
  • OnSurfaceTextureUpdated:
  
  

• Conclusion:
  
  
  
  • almost half the time (40%) is spent on toScreenLocation to translate between pixels on the screen and map coordinates.
  
  
  • setting the X value of the View Marker is (25%), strange enough is that setting Y value is only 6.8%
  
  

[tobrun]

Adding a note to look into hardware layering the ViewMarker and look into using something instead of setX/setY that is more performant (these methods now invoke a measure on MapView which is not needed).

[tobrun]

I touched base with @boundsj a couple of days back and talked about the animations we are going to expose. For now we are going to focus on select/deselect animation and not focus on add/remove annotation. It seems that the Android API will need to limit the user a bit more than on iOS since it will require setting these up via setViewMarkerItemAnimator() instead of animations the view directly. The need for having a separate API comes from the Adapter approach requirement.

[tobrun]

An example of the select/deselect animation mentioned above:

API:

void setMarkerViewItemAnimation(@AnimatorRes int animationInRes,@AnimatorRes int);

update: while above works in typical use cases. We need to optimise this for view reuse. An issue now is that if you leave a marker open and pan it out of the viewport and afterwards pan it back in. You will see that another marker has the animated state and the initial view is not animated anymore. This is due to view-reuse where a view place can be recycled to be used for another marker.

[tobrun]

There are cases where view recycling is not correctly applied.

Steps to reproduce:

• open BulkMarkerActivity with 1000 markers at once (viewcache grows to 1000)
• zoom in to a spot on the map (viewcache stays 1000 large)
• pan to other places on the map (viewcache grows a bit bigger, eg. 1045 after couple swipes)

[tobrun]

Related to animations, I still need to work on Animation system item. This system needs to be able to hold state about the animation of a reused marker view. In meantime I added some show/hide alpha animations. At this point these will not be user configurable but this could change in future releases. (I mainly not exposing this now since iOS is also not committing to this API at this time).

[1ec5]

I wouldn’t worry too much about what the iOS SDK is going to expose, because iOS relies on implicit (declarative) animation. If I’m not mistaken, as things stand, an iOS application could animate the annotation dropping from the sky or growing out of the map without any additional API on our end.

[boundsj]

I think that is correct @1ec5 -- any arbitrary animations should be possible.

I'm about to make a commit that adds a prepareForReuse API to the new iOS MGLAnnitationView. This method is helpful for, among other things, managing animations of reused views. If the application developer uses a view model / view pattern (i.e. annotation and annotation view), animations can be cleaned up just before an annotation view is reused (since the SDK calls prepareForReuse on each view just as it comes out of the queue) and restarted (based on view model state) when the reused view is shown in the map again. A simpler alternative that may work in some cases is for the app developer to simply opt out of the reuse queue (by not supplying a reuse identifier).

[tobrun]

Thanks for the 👀 and the suggestions @1ec5 @boundsj.

Related to require subclassing a specific View:

I have been looking at a similar setup as what you expose on iOS. I tried introducing an Android equivalent of MGLAnnotationView. I initially didn't like the idea of forcing our users to extend a certain View class.

For example this is the code to show a simple text as View Marker:

    public class TextViewMarker extends ViewMarker {


        public TextViewMarker(Context context) {
            super(context);
            init(context, null);
        }


        public TextViewMarker(Context context, AttributeSet attrs) {
            super(context, attrs);
            init(context, attrs);
        }


        public TextViewMarker(Context context, AttributeSet attrs, int defStyleAttr) {
            super(context, attrs, defStyleAttr);
            init(context, attrs);
        }


        private void init(Context context, AttributeSet o) {
            LayoutInflater.from(context).inflate(android.R.layout.simple_list_item_1, this);
        }


        public void setText(String text){
            TextView textView = (TextView) findViewById(R.id.text);
            textView.setText(text);
        }
    }

That class then should be used in the Adapter with a return set to ViewMarker:

        @Nullable
        @Override
        public ViewMarker getView(@NonNull CountryMarker marker, @Nullable ViewMarker convertView, @NonNull ViewGroup parent) {
            ViewHolder viewHolder;
            if (convertView == null) {
                viewHolder = new ViewHolder();
                convertView = inflater.inflate(R.layout.view_custom_marker, parent, false);
                viewHolder.flag = (ImageView) convertView.findViewById(R.id.imageView);
                viewHolder.abbrev = (TextView) convertView.findViewById(R.id.textView);
                convertView.setTag(viewHolder);
            } else {
                viewHolder = (ViewHolder) convertView.getTag();
            }
            viewHolder.flag.setImageResource(marker.getFlagRes());
            viewHolder.abbrev.setText(marker.getAbbrevName());
            return convertView;
        }

While it's a common practice on Android to write custom views ourselves. It feels a bit unnatural when a library requires you to extend a certain View while combining that with an Adapter approach. This will cascade in requesting our end developers to write some extra code.

---

Related animations:

animations can be cleaned up just before an annotation view is reused (since the SDK calls prepareForReuse on each view just as it comes out of the queue) and restarted

I also tried this approach but was having issues with Animator base class not exposing a reverse method. If this was the case. I could reference the set animator through .animate() and perform the reversed animation but that would still limit us having 1 animation. Introducing a model containing the animations/state would solve this but it will not be part of the same model as on iOS since we are limited by the Marker/MarkerOptions setup and don't have a subclass of own Android SDK View class that is able to talk to a model when animations are set.

Limitations

The mentioned items above limits the end developer more as on iOS. For example now all view markers must have the same animations or some what the same layout. To resolve this I'm thinking to allow setting multiple ViewMarkerAdapters on one map so an end developer is not restricted to having one interaction for all view markers but can define as many different ones as he wants.

I'm still figuring/trying some things out and it might be that I will convert the Android solution to be completely the same as iOS. Current solution is based on something that feels common to Android developers and exposed limitations above are imo limitations that they will perceive as normal for view reuse on Android.

cc @bleege @zugaldia @cammace

[tobrun]

To allow developers to achieve the same functionality and thus meeting the same business requirements. I have added having multiple adapters to the todo list. This will result in changing the way how animations are set and will couple them together with adapter itself.

update:

Here is an example with having 2 view marker adapters:

To finish integration on this:

• [x] decouple ViewPool for each instance of ViewMarkerAdapter
• [x] change animation handling setup
• [ ] add marker type to OnMarkerClickListener

[tobrun]

I'm back to profiling the code and determining performance bottle necks: I'm first looking into OnSurfaceTextureUpdateListener, This method is called each time we render a frame.

Most time was spend on toScreenLocation which translates a LatLng to a pixel on screen.

The other method to look into is invalidating view markers on screen:

Time there was mostly spend on inflating views. This is normal since this is the reason why we introduced View reuse in the first place. Doing another trace where all the views are already inflated and doing some panning. I verified that no calls are done for layout inflation.

[tobrun]

I have been thinking a lot about which API Android should expose.

Their are currently two options available:

• the adapter approach as shown in this issue
• the same API as iOS is exposing in #1784 / https://github.com/mapbox/mapbox-gl-native/pull/4801

While their isn't a big difference in public API or underlying logic. Their is a concrete model object representing the view state and a concrete link with to the current Marker object.

I'm going to rework our current approach to match iOS and identify improvements/problems.

[bleege]

FWIW, my initial gut reaction is to support the more "Android way" of doing things API wise as that's the platform that this being built for and what most developers are going to expect. That said I don't know enough of the details here to make an informed opinion. @tobrun Can you pick an example and write some pseudocode of what the API could look like if it was written "iOS Style" and what it'd look like "Android style". That'll make things more concrete for everyone to more easily wrap their heads around this questions.

[tobrun]

@bleege
Agreed that we want an API that feels Android like but I didn't want to implement something completely different than iOS. A lot of users want to implement an application on both platforms and both SDKs should be able to handle the same use cases/business requirements in a similar fashion.

At this point the difference between current progress and the one proposed in OP are:

• MarkerOptions has a flag indicating if a Marker is a VBA
• Introduced MarkerViewSettings which is a model that stores
  
  
  
  • state (https://github.com/mapbox/mapbox-gl-native/issues/3276#issuecomment-218554748)
  
  
  • configuration
  
  

The latter item is important as it decouples the View Marker from a GL Marker. This will enables us to implement features in View marker that are not yet existent in GL markers (eg. offset). I'm currently figuring out how this model fits in the bigger picture and where it should be configured for end developers.

[1ec5]

For reference, the architecture on iOS is driven by the need for different annotations to have completely different characteristics. From the outset, we've relied on the delegate pattern (equivalent to the adapter pattern) for customizing GL annotations, but that quickly highlighted the need to customize individual annotations. Each delegate method needs to switch on the annotation's reuse identifier (akin to a marker ID), leading to quite a bit of boilerplate code.

By putting the majority of style properties on the view itself, we centralize the logic so that the developer only has to switch on the reuseIdentifier in order to associate a view with an annotation. By analogy, MapView's marker adapter would have only one method that returns a view.

This approach on iOS allows for a cleaner MVC separation, but it does come with the downside of requiring the developer to subclass a specific view class. We could relax that restriction by factoring these style properties into a protocol (aka interface in Java) and having MGLAnnotationView conform to that protocol. Then if the developer wants to supply a completely custom view, they'd be responsible for implementing these properties. This is exactly the approach we've taken with the annotation model objects: you can instantiate an MGLPointAnnotation or supply your own class that conforms to MGLAnnotation. For now, in the absence of an MGLAnnotationViewProtocol, we expect that anyone who wants to supply a non-standard view like a UISwitch can embed it in an MGLAnnotationView.

To me, it isn't principally important for the two SDKs to match API-for-API, but @tobrun has brought up many of the same considerations that led us to move away from an adapter-like approach on iOS.

[tobrun]

@1ec5 this clears up a lot of questions I had on the iOS implementation! Thank you :bow:

[tobrun]

I have started adding the features that weren't possible (yet) with GL annotations:

Here you can see flat in action on 🇺🇸 and the :green_zero_marker:

[tobrun]

I'm now pushing on my last architectural refactor. With that in place we can merge it to master. The only open item to resolve is inheritance of generic MarkerView.

[bleege]

While working on a related Marker issue I noticed that BaseMarkerViewOptions has a method called flat(boolean flat) in it that doesn't appear to have any connection with Core GL for adding Markers to the Map. I see this is part of the Google Maps API's MarkerOptions so that's likely why it was implemented. Regardless, I wanted to see what it'd do so I added a counter example to it's test in the TestApp's MarkerViewActivity and it didn't appear to do anything. If that's the case we should reconsider shipping this method (it's new to 4.1.0) as it's one thing to be complete with the Google Maps API but it's another thing to ship a method that doesn't have any function. Maybe I'm missing something though. Thoughts?

_Test Code_

_Results_

[1ec5]

The flat property is intended to make the marker lie flat against a tilted map rather than standing up.

[bleege]

@1ec5 Thanks for clarifying! I re-ran the test and that's what it does.

[1ec5]

As it happens, that transformation is incorrect: see #5218.

[p-fischer]

Hi @tobrun @bleege !
I'm looking into the Mapbox Android API. I'm strongly interested in using it along with MarkerViews. However I'm still having a bit of trouble in understanding the implementation requirements. Can you point me to a documentation/tutorial/example that shows the usage?
The country marker example has helped me quite a bite so far. Is the full source code available? I'm especially interested in seeing the implementation of CountryMarker and CountryMarkerOptions.
I'm thankful for any help. Cheers

[bleege]

@p-fischer Thanks for considering Mapbox! Markers are have undergone a big change in the 4.1.0 code base so confusion isn't surprising. 😄 We won't have official documentation / examples of this ready on mapbox.com until we ship 4.1.0 Final (likely in the next week or so). In the meantime though I'd recommend exploring the code in the TestApp as it's the Android app that we use internally to build these API into the SDK. As such it's always the most up to date reference materials that we have. We're currently working on the release-android-v4.1.0 branch so the TestApp code that's going to be most useful for you can be found here:

TestApp Home Page
https://github.com/mapbox/mapbox-gl-native/tree/release-android-v4.1.0/platform/android/MapboxGLAndroidSDKTestApp

Annotation (which Markers are part of) Examples
https://github.com/mapbox/mapbox-gl-native/tree/release-android-v4.1.0/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/annotation

Marker InfoWindow Examples
https://github.com/mapbox/mapbox-gl-native/tree/release-android-v4.1.0/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/infowindow

As always, we produce nightly builds (called SNAPSHOTS) that anyone can use to get the latest updated code from the day! Instructions on how to install them can be found on the Mapbox Android SDK Web site.

[p-fischer]

Thanks @bleege , I finally could get a marker view to be displayed :)

The issues I'm now having are:

1. Markers are not drawn immediately. Only when I pan the map, or if I call invalidateViewMarkers on the MarkerViewManager.
2. The Marker pop ups in the top left corner for a short while before moving to its actual position (might be related to 1?!)
3. Panning the map is disabled when touching the map on a marker view. This is possible on a regular marker. I think the touch event should be delegated to the MapView if not consumed.
   4.The TestApp crashed as soon as I selected an Activity holding a map. I got an exception saying:

[...]
android.view.InflateException: Binary XML file line #14: Error inflating class com.mapbox.mapboxsdk.maps.MapView
[...]
Caused by: java.lang.UnsatisfiedLinkError: dalvik.system.PathClassLoader[DexPathList[[zip file "/data/app/com.mapbox.mapboxsdk.testapp-1/base.apk"],nativeLibraryDirectories=[/vendor/lib, /system/lib]]] couldn't find "libmapbox-gl.so"

[bleege]

@p-fischer Glad that you're up and running! If you can share some of the code that's causing these problems for you we'd be happy to help you figure them out.

[p-fischer]

Hi @bleege , I basically copied the classes CountryAdapter, CountryMarker, CountryMarkerOptions, CountryMarkerView, and CountryMarkerViewOptions from the TestApp, you pointed me to, and put a map view in a fragment in my app.
I'd like to get the TestApp running (and not crashing) to check the behavior observed in my app (1. - 3.) in the TestApp. Unfortunately, the TestApp crashes with the exception mentioned in 4. Do you have any idea what the exception means?

[Schumi09]

Hi, depending on what you are trying to achieve it is meanwhile also possible to use some default image adapter and all you have to do is

MarkerViewOptions options = new MarkerViewOptions();
options.icon(iconFactory.fromResource(R.drawable.default_marker));
options.flat(true);
options.position(new LatLng(51.963611, 7.613142));
options.rotation(90.0f);
map.addMarker(options);

so you could also set up a more complex Drawable that you can use as Icon then.

[bleege]

@p-fischer Looking at the log I think the first place to start is to make sure that you're building the proper ABI of Core GL that your device is expecting. This is the libmapbox-gl.so part of the error log. We've got instructions for how to set up your local development environment and specifically running the TestApp that show how to build this part of the Mapbox Android SDK. If you can take a look at those and confirm that things are setup as needed we can then keep looking into problem.

Setup Project Locally Instructions
https://github.com/mapbox/mapbox-gl-native/blob/master/platform/android/CONTRIBUTING_OSX.md

Running The TestApp Instructions
https://github.com/mapbox/mapbox-gl-native/blob/master/platform/android/CONTRIBUTING_OSX.md#running-the-testapp

[bleege]

@Schumi09 Thanks for showing that as a way to get the default marker to load in the current iteration. So everyone know we're planning on fixing that in #5276.

[p-fischer]

@bleege I got the TestApp running, thanks. However, I copied the TestApp Module and referenced the SDK the SNAPSHOT as a dependency. For lazy people like me, it might be handy to have the Test App in a separate repository referencing the SDK only through a dependency ;)

I did observe that issue that the map can not be panned starting touch on a marker view on the Test App as well. I think that should be possible because you want the marker view behavior to be consistent with the behavior of the ordinary markers.

[bleege]

@p-fischer Glad that you were able to get things running. I agree it'd be helpful to have the TestApp in a separate repository but it'd also have to continue living in it's current spot as it's how day to day development has to be done because the SDK is an Android Library. FWIW, we're starting to add more examples to the Demo App if you're interested in these types of things. https://github.com/mapbox/mapbox-android-demo

I did observe that issue that the map can not be panned starting touch on a marker view on the Test App as well. I think that should be possible because you want the marker view behavior to be consistent with the behavior of the ordinary markers.

Yep, I was able to recreate this too in the TestApp. As the gif shows below I tried panning underneath the Mapbox logo and it worked and then I tried panning by touching the Mapbox logo and it didn't. Meanwhile this same process worked when I tried panning on the red GL based marker. At first glance it sounds like the gestures are being consumed at the View Annotation level and not being forwarded on to the MapView. We'll keep looking into this as you're correct that the behavior should be consistent with the GL markers.