Add View Annotations Z ordering

[tobrun]

We have been hearing about the need to be able to order Markers. For View based Markers an end developer should be able to indicate to a marker to be placed on top of another marker. This system must integrate with other already shown View elements as CompasView, MyLocationView etc. This could be made part or based on current implementation of MarkerViewAdapter, as a separate ViewOverlay implementation or exposed in Marker itself.

This should solve the issues we have been seeing related to overlapping views.
This issue doesn't cover z-ordering or layering from core in #991

[tobrun]

Instead of calling it layers, the term Overlay is more appropriate. This naming doesn't conflict with the layers we have in core and the view based components will always be on top of the map (overlain on top of the map).

[tobrun]

This should solve the issues we have been seeing related to overlapping views.

If you currently pan an MarkerView towards the CompassView you will see that the MarkerView goes over the CompassView instead of going underneath.

[zugaldia]

Related: #5285 (Android equivalent of #991).

[tobrun]

@ivovandongen has been picking related items up in #5298 & #5294.
We need to validate if those fixes fulfil the requirements of this ticket.

[zugaldia]

@tobrun These are limited scale implementation of this ticket.

In particular, #5294 only brings the selected marker to front, which is a common usecase for the larger need for Z-ordering. iOS recently did the same with #991.

[tobrun]

Moving this of the milestone, what we want for 4.1.0 is in place.
TBD what we want to add in future releases.

[zugaldia]

Moving this of the milestone, what we want for 4.1.0 is in place.

👍

[tobrun]

A user reached out in #5451 requesting the possibility to assign a MarkerView to a layer and manipulate layers completely (eg. hide a layer).

[guruduttstay]

Do we know if there is a plan to add this feature request to Milestone :)

[zugaldia]

Do we know if there is a plan to add this feature request to Milestone :)

@guruduttstay I'd say that we aren't gonna add any more features to 4.1.0 as we are already getting ready for a final release once we published beta3 earlier this week. The good news is that as discussed above, 4.1.0 will bring some partial improvements to this feature via #5294.

[incanus]

I'm going to take a look at this. In the raster iOS SDK, I had implemented a delegate pattern callback for simple comparator-based two-at-a-time sorting of annotations (all of which were native-side views at the time), encapsulated in mapbox/DEPRECATED-mapbox-ios-sdk#491. I will see what sort of API makes sense in Android land for something like this.

[incanus]

To break down the implementation I will likely model this from (found here), the API could work like this:

1. Each frame update, run through the native-side view ordering routine.
2. If the client doesn't implement a custom callback, fall back to a built-in algorithm. This would sort such things as:
   1. Existing views by top-to-bottom of screen order.
   2. Any selected view(s).
   3. Shapes.
   4. Core-side sorting.
3. If the client does implement a custom callback, call out to it for native-side view sorting instead.
   • The callback can consist of a java.util.Collections Comparator interface.

Does this sound like a good design? Will it meet the needs we've outlined?

[incanus]

I guess the broader question here is: do callers want to be able to provide an algorithm for sorting, or to indicate sort upfront at placement time and be hands off afterwards?

[incanus]

If the answer to the above is an algorithm, it looks like with the ViewGroup API that we are using for our mMarkerViewContainer, we can use setChildrenDrawingOrderEnabled(true) and then override getChildDrawingOrder() in order to provide a builtin, or to call out to a user's custom, view sorting operation.

[incanus]

Sounds like an algorithm such as above is a second-stage goal here, first stage being simply passing an index (or, more conveniently, a reference to another marker) which we want the new marker to be atop. Let's start with that first.

[incanus]

I plumbed the requisite argument down through the JNI in 94a5869...fb4e607 then got into C++ Map::addAnnotation() before realizing that this only needs to exist in Java land due to the view bounds checking and view hierarchy adds & deletes in the MarkerViewManager, so I'm working on a simpler implementation now. Stay tuned.

[incanus]

Ok, this seems to work ok, though I want to test some more edge cases around view recycling as this requires the aboveMarkerView to currently be in the view hierarchy.

Using a modification such as this quickie to MarkerViewActivity.java in the test app:

diff --git a/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/annotation/MarkerViewActivity.java b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/annotation/MarkerViewActivity.java
index 0c4eeae..d8bd767 100644
--- a/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/annotation/MarkerViewActivity.java
+++ b/platform/android/MapboxGLAndroidSDKTestApp/src/main/java/com/mapbox/mapboxsdk/testapp/activity/annotation/MarkerViewActivity.java
@@ -111,19 +111,20 @@ public class MarkerViewActivity extends AppCompatActivity {
                         .position(new LatLng(38.902580, -77.050102))
                 );

-                mMapboxMap.addMarker(new TextMarkerViewOptions()
-                        .text("A")
+                MarkerView a = mMapboxMap.addMarker(new TextMarkerViewOptions()
+                        .text("Aardvark")
                         .position(new LatLng(38.889876, -77.008849))
                 );

                 mMapboxMap.addMarker(new TextMarkerViewOptions()
-                        .text("B")
+                        .text("Beethoven")
                         .position(new LatLng(38.907327, -77.041293))
                 );

                 mMapboxMap.addMarker(new TextMarkerViewOptions()
-                        .text("C")
+                        .text("Calumet")
                         .position(new LatLng(38.897642, -77.041980))
+                        .aboveMarkerView(a)
                 );

                 // if you want to customise a ViewMarker you need to extend ViewMarker and provide an adapter implementation

Orders things as A, C, B, despite adding C after B.

[incanus]

I'm having a tough time creating a scenario where this doesn't work. The following scenario worried me at first, but I think it's solid:

1. Two markers, A and B, far apart on the map, with a particular ordering (A under B).
2. Marker B gets removed when far enough out of viewport.
3. When it comes back into relevance, its view is re-added to the hierarchy automatically.
4. Since the reference is there to its aboveMarkerView (i.e. A), A is found in the hierarchy, its index is pulled, and B is re-added, under A if it still exists.
5. If A ever goes away, that's fine since B only specifies A as relevant if it exists.

As far as I can tell with the code and my tests, we are good to go here.

@tobrun @zugaldia Care to weigh in on #5862?

[incanus]

Copying comment from #5862 (comment) since we're abandoning that.

@tobrun and I voiced on this at length and worked through some consideration of both an API design like I've proposed as well as a callback-based sort comparator. The short is that it probably makes sense to skip right to the comparator as fetching it from a callback is a lesser consideration on performance than the actual sorting. Additionally, we could add Parcelable to Marker in order to make the proposed solution work, but the API is still limiting in referring to only one other marker, when marker add order needs to remain flexible.

[tobrun]

I'm trying out the most simplistic ordering possible and that is using ordering based on the used adapter. This solves the follow use-case:

Cars markers should always be shown on top of buildings markers.

Put it other words, a collection of markers should be able to be shown on top or bellow another collection of markers. Currently the different markers need to be implemented with 2 MarkerViewAdapters and I'm thinking of assigning each adapter with it's own FrameLayout (as it now already has it's own view reuse pool). Now an adapter can be added with an index. Where the index is the order in the parent FrameLayout.

To put it in code and images:

addMarkerViewAdapter(new BackgroundViewAdapter(this, 0 /* bottom */);
addMarkerViewAdapter(new TextAdapter(this), 1 /* I'm in front of above */);

addMarkerViewAdapter(new BackgroundViewAdapter(this, 0 /* bottom */);
addMarkerViewAdapter(new TextAdapter(this), 0 /* I'm in below of above */);

[tobrun]

Above comment solves the use-case of ordering different types of MarkerViews, it doesn't encapsulate ordering the same type of MarkerViews. My first attempt will involve using float as z index, this will determine the order how the children are drawn. Since this involves dispatching the draw calls ourselves, we might need to look into create are own ViewGroup and make it possible to do order based on the zIndex value of the MarkerViews.

The difficult part to this system is that views can be recycled when the viewport changes and these need to be restored in correct drawing order. Probably this will result doing the ordering during drawing what could result in a performance hit.

[zugaldia]

Put it other words, a collection of markers should be able to be shown on top or bellow another collection of markers.

I like this approach, even if it's a first iteration. It's consistent with what we do in the Runtime Style API where the z-ordering is attached to the layer, not the annotations. In this case, the adapter (if I understand correctly) would be the equivalent of the layer.

Looking good.

[tobrun]

Been testing out the concept of changing the order marker views are drawn. You can do this by calling setChildrenDrawingOrderEnabled(true) and overriding getChildDrawingOrder(int childCount, int i) on any ViewGroup.

To make this work in MarkerViewManager, we need to maintain a drawing order based on a float. Current thinking is to use the zIndex keyset value of Map<MarkerView, View> for this. With this we also have a parity with Marker from Google. If this solution works out we might need to look into not merging #5238 (comment) since this will be all inclusive solution and could work for all MarkerViews at the same time.

[tobrun]

I got the sorting implemented with a compareTo:

            @Override
            public int compare(MarkerView lhs, MarkerView rhs) {
                int compare = Float.compare(lhs.getzIndex(), rhs.getzIndex());
                if (compare == 0) {
                    return Long.valueOf(lhs.getId()).compareTo(rhs.getId());
                }else{
                    return compare;
                }
            }

This will sort the keys of a TreeMap based on MarkerView zIndex value, if they are equal we compare using id instead. The last remaining tasks is making actually changing the drawing order for this we need to map to original drawing order on our ordered keys.

update:
Had to change compareTo to match the compareTo from Annotation also simplified the code.

    @Override
    public int compareTo(@NonNull Annotation annotation) {
        if (annotation instanceof MarkerView) {
            MarkerView other = (MarkerView) annotation;
            if (zIndex < other.getzIndex()) {
                return 1;
            } else if (zIndex > other.getzIndex()) {
                return -1;
            }
        }
        return super.compareTo(annotation);
    }

[zugaldia]

@tobrun is zIndex == other.getzIndex() a possibility?

[tobrun]

yes, in that case it is calling super.compareTo(annotation) which falls back on the Annotation compareTo which uses comparison on id. In other words, in case we have the same zIndex, we will layer those MarkerViews based on when it was added to the map.

[zugaldia]

@tobrun 👍

[altafrm3d]

Is there something to give Z values to Polylines/Polygons to defines in which order should they show up on MapView? If not, does this ticket cover this feature request?

[hanspeide]

From what I understand this issue tracks z ordering for MarkerViews. Is z ordering for Markers being tracked anywhere?

[zugaldia]

@hanspeide This is a good ticket to read about ordering of annotations, particularly this comment: #1728 (comment).

Runtime styling is now available in the latest version of the SDK and can help with some of the use cases described in this ticket. This activity, for example, shows how to add a layer below another layer using mapboxMap.addLayerBelow().

[hanspeide]

@zugaldia Do layers support adding Markers? If not, what is the alternative?

[zugaldia]

@hanspeide You can add a symbol layer with a marker source. There's a SymbolLayerActivity example in the demo app that shows how to do it. It looks like this:

In general, the demo app is a great resource to get a good sense of what's possible using the current APIs (and it's completely open source).

[tobrun]

MarkerView is going to be deprecated with #9782. We are no longer maintaining this API and will remove it in future versions of the SDK. As noted in the deprecation message. We are advising users to leverage SymbolLayer instead (you can use the sample code in #9782 to convert Android SDK views to an Image to be used as a Symbol). While we no longer maintain the API we are happy to accept any PRs on it.