Skip to content

Instantly share code, notes, and snippets.

@hallahan
Last active August 29, 2015 14:26
Show Gist options
  • Save hallahan/5074f5ac4e2e298e7ba3 to your computer and use it in GitHub Desktop.
Save hallahan/5074f5ac4e2e298e7ba3 to your computer and use it in GitHub Desktop.
Code Review of Markers in Mapbox Android SDK

This is an overview of the current architecture of adding markers to a map. In particular, we are examining:

  1. How images / icons are placed on the canvas.
  2. How collision detection for touching icons work.
  3. How an overlay for markers is structured.
  4. How custom icons as an on device resource can be used (not getting the image from Mapbox servers).
  5. Should we add OSM centric markers to the OSMOverlay or should we revamp ItemizedOverlay to draw markers from a QuadTree rather than a dumb list.

Note that this is using a recent revision of the mapbox android sdk.

Maki Markers

Here is a corresponding marker coming from the Mapbox API:

https://a.tiles.mapbox.com/v4/marker/pin-s-marker-stroked+FF0000.png?access_token=pk.eyJ1IjoiYmxlZWdlIiwiYSI6IkRGLTFPU00ifQ.qJpq3jytAL9A-z_tkNypqg

The Maki Marker Site:

https://www.mapbox.com/maki/

Note that the icons included in the libary are without the enclosing marker, and the corresponding markers do need to be rendered. We may just want to use the icons themselves, however. The rendered marker product can be gotten from the API as seen above.

My thought is that this will be sufficient for icons in OpenMapKit POIs. Let's go with Maki Markers, but modify it so we don't have to get them via an API.

This is a basic example showing adding a ton of markers to the map. Note that we are only seeing 100 markers, and when I bump that up to 10,000, the app grinds to a halt. 1,000 doesn't perform well, but it is operational.

Steps on adding a marker to the map.

Construct Marker object.

public Marker(MapView mv, String aTitle, String aDescription, LatLng aLatLng) {
    super();
    this.mapView = mv;
    if (mv != null) {
        this.context = mv.getContext();
    }
    this.setTitle(aTitle);
    this.setDescription(aDescription);
    this.mLatLng = aLatLng;
    mParentHolder = null;
    mAnchor = DEFAULT_PIN_ANCHOR;

    // Note: Only Load Default Marker (if needed) when getMarker() called.
}

This just gets the object together with the appropriate properties.

Then we call mapView.addMarker(marker); to add the marker to the map.

public Marker addMarker(final Marker marker) {
    if (firstMarker) {
        defaultMarkerList.add(marker);
        setDefaultItemizedOverlay();
    } else {
        if (!getOverlays().contains(defaultMarkerOverlay)) {
            addItemizedOverlay(defaultMarkerOverlay);
        }
        defaultMarkerOverlay.addItem(marker);
    }
    marker.addTo(this);

    firstMarker = false;
    invalidate();
    return marker;
}

For the first marker it sets up a new ItemizedIconOverlay and adds that to the master overlays list.

private void setDefaultItemizedOverlay() {
    defaultMarkerOverlay = new ItemizedIconOverlay(getContext(), defaultMarkerList,
            new ItemizedIconOverlay.OnItemGestureListener<Marker>() {
                public boolean onItemSingleTapUp(final int index, final Marker item) {
                    selectMarker(item);
                    return true;
                }

                public boolean onItemLongPress(final int index, final Marker item) {
                    if (mMapViewListener != null) {
                        mMapViewListener.onLongPressMarker(MapView.this, item);
                    }
                    return true;
                }
            }
    );
    addListener(defaultMarkerOverlay);
    defaultMarkerOverlay.setClusteringEnabled(mIsClusteringEnabled, mOnDrawClusterListener, mMinZoomForClustering);
    addItemizedOverlay(defaultMarkerOverlay);
}
public void addItemizedOverlay(final ItemizedOverlay itemizedOverlay) {
    if (itemizedOverlay instanceof ItemizedIconOverlay) {
        // Make sure Markers are added to MapView
        ItemizedIconOverlay overlay = (ItemizedIconOverlay) itemizedOverlay;
        for (int lc = 0; lc < overlay.size(); lc++) {
            overlay.getItem(lc).addTo(this);
        }
    }

    if (itemizedOverlay.isClusteringEnabled()) {
        addListener(itemizedOverlay);
        itemizedOverlay.onZoom(new ZoomEvent(this, getZoomLevel(), false));
    }

    this.getOverlays().add(itemizedOverlay);
}
public ItemizedIconOverlay(final Context pContext,
                           final List<Marker> pList,
                           final com.mapbox.mapboxsdk.overlay.ItemizedIconOverlay.OnItemGestureListener<Marker> pOnItemGestureListener,
                           boolean sortList) {
    super();
    this.context = pContext;
    this.mItemList = pList;
    this.mOnItemGestureListener = pOnItemGestureListener;
    if (sortList) {
        sortListByLatitude();
    }
    populate();
}

Notice how that pList in the constructor is the same list as the defaultMarkerList in the MapView, so the list items are not encapsulated. Also notice that this list is sorted by latitude so that the markers further up on the screen are behind the markers lower down.

This sort by latitude is called only once in the constructor. This is bad, because when markers are added later on, they are just appended to this list and not resorted properly. You will see that this is a problem in the MarkersTestFragment.java.

populate then copies the items in mItemList to an internal list that is encapsulated.

protected void populate() {
    final int size = size();
    mAlgorithm.clearItems();
    mInternalItemList.clear();
    mInternalItemList.ensureCapacity(size);
    for (int a = 0; a < size; a++) {
        mInternalItemList.add(createItem(a));
    }
    mAlgorithm.addItems(mInternalItemList);

}

The mInternalItemList is what is used by the drawSafe method.

Populate is being called a lot, and there is a lot of copying happening. Whenver an item is added to the ItemizedIconOverlay, an entire new list is being constructed by a populate call in addItem. This must be solved to make adding markers to the map somewhat efficient.

The actual drawing takes place in the parent class ItemizedOverlay. ItemizedOverlay is a subclass of the awkward SafeDrawOverlay that makes a strange, hacky attempt at correcting the misprojection bug that plagues the library. I'm not going to go into this in this post...

So, draws are called indirectly via drawSafe rather than draw.

@Override
protected void drawSafe(ISafeCanvas canvas, MapView mapView, boolean shadow) {
    if (shadow) {
        return;
    }

    if (mPendingFocusChangedEvent && mOnFocusChangeListener != null) {
        mOnFocusChangeListener.onFocusChanged(this, mFocusedItem);
    }
    mPendingFocusChangedEvent = false;

    final Projection pj = mapView.getProjection();
    final int size = this.mInternalItemList.size() - 1;

    final RectF bounds =
            new RectF(0, 0, mapView.getMeasuredWidth(), mapView.getMeasuredHeight());
    pj.rotateRect(bounds);
    final float mapScale = 1 / mapView.getScale();

    if (!mIsClusteringEnabled || mapView.getZoomLevel() > mMinZoomForClustering) {
        /* Draw in backward cycle, so the items with the least index are on the front. */
        for (int i = size; i >= 0; i--) {
            final Marker item = getItem(i);
            if (item == mFocusedItem) {
                continue;
            }
            onDrawItem(canvas, item, pj, mapView.getMapOrientation(), bounds, mapScale);
        }

        if (mFocusedItem != null) {
            onDrawItem(canvas, mFocusedItem, pj, mapView.getMapOrientation(), bounds, mapScale);
        }

    } else if (mInternalClusterList != null) {
        for (int i = mInternalClusterList.size() - 1; i >= 0; --i) {
            final ClusterMarker clusterMarker = mInternalClusterList.get(i);
            List<Marker> markerList = clusterMarker.getMarkersReadOnly();

            if (markerList.size() > 1) {
//                    if (mOnDrawClusterListener != null) {
//                        Drawable drawable = mOnDrawClusterListener.drawCluster(clusterMarker);
//                        clusterMarker.setMarker(drawable);
//                    }
                onDrawItem(canvas, clusterMarker, pj, mapView.getMapOrientation(), bounds, mapScale);
            } else {
                onDrawItem(canvas, markerList.get(0), pj, mapView.getMapOrientation(), bounds, mapScale);
            }

        }
    }
}

Here, basically a few things are done that aren't important for what we are looking at, and onDrawItem is called to do the actual drawing work for the markers.

protected void onDrawItem(ISafeCanvas canvas, final Marker item, final Projection projection,
                          final float aMapOrientation, final RectF mapBounds, final float mapScale) {
    item.updateDrawingPosition();
    final PointF position = item.getPositionOnMap();
    final Point roundedCoords = new Point((int) position.x, (int) position.y);
    if (!RectF.intersects(mapBounds, item.getDrawingBounds(projection, null))) {
        //dont draw item if offscreen
        return;
    }

    canvas.save();

    canvas.scale(mapScale, mapScale, position.x, position.y);
    final int state =
            (mDrawFocusedItem && (mFocusedItem == item) ? Marker.ITEM_STATE_FOCUSED_MASK : 0);
    final Drawable marker = item.getMarker(state);
    if (marker == null) {
        return;
    }
    final Point point = item.getAnchor();

    // draw it
    if (this.isUsingSafeCanvas()) {
        Overlay.drawAt(canvas.getSafeCanvas(), marker, roundedCoords, point, false,
                aMapOrientation);
    } else {
        canvas.getUnsafeCanvas(new UnsafeCanvasHandler() {
            @Override
            public void onUnsafeCanvas(Canvas canvas) {
                Overlay.drawAt(canvas, marker, roundedCoords, point, false, aMapOrientation);
            }
        });
    }

    canvas.restore();
}

PROBLEM: Regardless of if the marker is on the map or not, we attempt to do a draw. Every single marker added to the map is looped through, and a hit detection is done in the onDrawItem via RectF.intersects. This is very inefficient, and we will not be able to handle OSM data sets of a significant size. We want to avoid this and instead give it markers that are in the view port supplied by inquiry from a spatial index, such as a QuadTree.

We updateDrawingPosition of the marker. This calls getMapDrawingBounds of the marker returning the RectF of the bounds of the marker. This value is stored in mMyLocationRect of the marker.

protected RectF getMapDrawingBounds(final Projection projection, RectF reuse) {
    if (reuse == null) {
        reuse = new RectF();
    }
    projection.toMapPixels(mLatLng, mCurMapCoords);
    final int w = getWidth();
    final int h = getHeight();
    final float x = mCurMapCoords.x - mAnchor.x * w;
    final float y = mCurMapCoords.y - mAnchor.y * h;
    reuse.set(x, y, x + w, y + h * 2);
    return reuse;
}

The mAnchor value is PointF(0.5, 1.0) for the markers in the demo app. This makes perfect sense for a marker since its the middle of the bottom. The only time this is set is in the constructor of the marker with a DEFAULT_PIN_ANCHOR constant. You can explicity set the anchor of your marker with the setAnchor method.

Next, we get a PointF position from getPositionOnMap of the marker. This is just returning a mCurMapCoords field in the marker. Notice that this is the same object that was set just before by getPositionOnMap. This is used to scale the canvas based off of the mapScale parameter to onDrawItem. mapScale is typically 1.0 unless there is a pinch zoom gesturing happening with the map.

Next, we get the drawable of the marker, as well as then anchor point. This is in turn used by the Overlay.drawAt convenience method.

protected static synchronized void drawAt(final Canvas canvas, final Drawable drawable,
        final Point origin, final Point offset, final boolean shadow,
        final float aMapOrientation) {
    canvas.save();
    canvas.rotate(-aMapOrientation, origin.x, origin.y);
    canvas.translate(origin.x + offset.x, origin.y + offset.y);
    drawable.draw(canvas);
    Paint paint = new Paint();
    paint.setColor(Color.RED);
    paint.setStrokeWidth(3);
    canvas.drawLine(0, -9, 0, 9, paint);
    canvas.drawLine(-9, 0, 9, 0, paint);
    canvas.drawRect(drawable.getBounds(), paint);
    canvas.restore();
}

This method is getting the canvas, the drawable, the rounded coordinates of the mCurMapCoords, and the anchor point. shadow is ignored, and aMapOrientation is used when the map is twisted off from north. offset is the anchor point that is correctly moving the drawing to the right orientation from the anchor. I'm not sure what the red rectangle is all about, and I don't see it getting drawn, even when I comment out drawing the drawable.

Marker Top Collision Detection

We do the hit test in activateSelectedItems in ItemizedIconOverlay.

private boolean activateSelectedItems(final MotionEvent event,
                                      final MapView mapView,
                                      final ActiveItem task) {
    final Projection projection = mapView.getProjection();
    final float x = event.getX();
    final float y = event.getY();
    for (int i = 0; i < this.mItemList.size(); ++i) {
        final Marker item = getItem(i);
        if (markerHitTest(item, projection, x, y)) {
            if (task.run(i)) {
                this.setFocus(item);
                return true;
            }
        }
    }
    return false;
}

activateSelectedItems is called by the onSingleTapConfirmed and onLongPress events only. Notice that a hit test is being done on all of the markers, regardless of if they are in the view port (very inefficient). Also notice that the test is being done by the markers in mItemList and not mInternalItemList because mInternalItemList is a private field of the super class ItemizedOverlay. It seems a bit odd that we have two separate lists, and one is being referenced by the draws and the other is being referenced by hit detection.

The markerHitTest is getting the bounds of the marker and determining if the point on the view port of the touch event is within.

protected boolean markerHitTest(final Marker pMarker, final Projection pProjection,
                                final float pX, final float pY) {
    RectF rect = pMarker.getHitBounds(pProjection, null);
/*
    RectF rect = pMarker.getDrawingBounds(pProjection, null);
    if (pMarker.isUsingMakiIcon()) {
        //a marker drawing bounds is twice the actual size of the marker
        rect.bottom -= rect.height() / 2;
    }
*/
    return rect.contains(pX, pY);
}

This contains is a simple point in rectangle test implemented by Android Graphic's RectF class.

public boolean contains(float x, float y) {
    return left < right && top < bottom  // check for empty first
            && x >= left && x < right && y >= top && y < bottom;
}

When the hit test actually succeeds, we call setFocus on the ItemizedOverlay with the marker.

public void setFocus(final Marker item) {
    mPendingFocusChangedEvent = item != mFocusedItem;
    mFocusedItem = item;
}

If we have a new item focused, we set mPendingFocusChangedEvent to be true. This is checked next time drawSafe is being called.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment