Annotation dragging does not respect the layer order about maplibre-plugins-android HOT 11 CLOSED

So just to clarify, when you don't set withDraggable(true) the order is correct?

The drawing order is always correct. The dragging order is not. So if I don't set withDraggable(true), then it is not draggable, and I don't see the dragging problem 😁.

Where is the concept of something being draggable defined?

The Annotation type itself is set to be draggable. But then I am not completely sure where the logic happens that goes from a touch event on the map to actually deciding that this annotation should capture the event.

from maplibre-plugins-android.

I agree that the expected behavior is that dragging puts something at the top. We should probably incorporate idea in the new Annotations API.

In the meantime, as a workaround, I think you may be able to use circle-sort-key even though it is not exposed with CircleOptions with CircleOptions.withData. You'd have to update the matching annotation in a callback when the dragging starts.

from maplibre-plugins-android.

Instead of introducing this new API, should we not just make sure queryRenderedFeatures returns features in the right order?

Yes exactly! That's what I meant with "Or the native map could return the layers in vertical order (instead of adding a compareLayersZIndex function), but that would make it implicit".

But that is going into the C++ native code, right? Are you familiar with that part of the code? I tracked it to here in the render_orchestrator.cpp, but I don't really understand enough to see where the layer vertical order should intervene.

from maplibre-plugins-android.

I'll have a look but I can't make any promises.

from maplibre-plugins-android.

I had another look, and I am starting to think that the C++ part is really per layer. A layer queries the rendered feature for itself.

And the bug is between layers, so that would be on the Android side. I am guessing that somewhere, the move gesture is detected and passed to each layer one at a time. The layer then has an opportunity to "capture" the event (if it has a feature that should use it) or not. And obviously if the layers are not called in the right order, then the dragging may be captured by the wrong layer.

So yeah, I'll have a closer look on the Android side over the weekend!

from maplibre-plugins-android.

Thanks for the clear bug report.

So just to clarify, when you don't set withDraggable(true) the order is correct?

Have you looked at the implementation? Where is the concept of something being draggable defined? The Plugin, the Android SDK or the C++ layer? I guess that's the best place to start debugging.

from maplibre-plugins-android.

Actually I think the query for the annotations is happening here: https://github.com/maplibre/maplibre-plugins-android/blob/main/plugin-annotation/src/main/java/com/mapbox/mapboxsdk/plugins/annotation/DraggableAnnotationController.java#L109

        for (AnnotationManager annotationManager : annotationManagers) {
            if (detector.getPointersCount() == 1) {
                Annotation annotation = annotationManager.queryMapForFeatures(detector.getFocalPoint());
                if (annotation != null && startDragging(annotation, annotationManager)) {
                    return true;
                }
            }
        }

Which calls

    T queryMapForFeatures(@NonNull PointF point) {
        List<Feature> features = mapboxMap.queryRenderedFeatures(point, coreElementProvider.getLayerId());
        if (!features.isEmpty()) {
            long id = features.get(0).getProperty(getAnnotationIdKey()).getAsLong();
            return annotations.get(id);
        }
        return null;
    }

Which seems... like it is taking the first feature (features.get(0)), right? That looks suspicious.

from maplibre-plugins-android.

I think I got it, it's happening in onMoveBegin:

    boolean onMoveBegin(MoveGestureDetector detector) {
        for (AnnotationManager annotationManager : annotationManagers) {
            if (detector.getPointersCount() == 1) {
                Annotation annotation = annotationManager.queryMapForFeatures(detector.getFocalPoint());
                if (annotation != null && startDragging(annotation, annotationManager)) {
                    return true;
                }
            }
        }
        return false;
    }

It goes through all the annotationManagers, and check if that particular annotationManager has an annotation. If it does (and it is draggable), then it returns true (which starts the dragging).

But the problem is that it returns true for the first annotationManager for which it finds an annotation, regardless its zIndex. So instead, I think onMoveBegin should keep a list of all the annotationManager instances that have an annotation at this point, and then assign the drag to the top one.

Not sure how to do it yet, though 😅, as the AnnotationManager elements don't have directly a zIndex, so I need to find a way to compare them ("is this one above or below this other one?").

from maplibre-plugins-android.

@louwers: I think we may need something from the NativeMap 🤔. Right now when adding a layer, it ends up calling something like style.addLayerBelow(), and Style.java calls nativeMap.addLayerBelow(layer, below);.

But the layer itself does not know it's position in the "vertical stack", and has no way to get it as far as I can see.

Would it make sense to have a native function that would help compare two different layers? Something like this:

int compareLayersZIndex(Layer left, Layer right);

Which would call NativeMap and return something like -1 (left is below), 0 (they have the same zIndex) or 1 (right is below)? That way the onMoveBegin() function above could get all the candidate layers, compare them to extract the top one, and drag only the top one.

@fynngodau: I'd like to have your opinion on this, too!

from maplibre-plugins-android.

I am not sure I understand. So you mean that somewhere in the AnnotationManager logic, I should inject custom data (e.g. the zIndex) and then use that to sort the layers in the onMoveBegin code above?

The thing is that circle-sort-key is not enough, because it's not only between circles: it's between layers...

I guess one thing I could do is expose belowLayerId and aboveLayerId in the AnnotationManager, and then from the addAnnotationManager and removeAnnotationManager of DraggableAnnotationController I could reconstruct the vertical stack, and use it to sort the layers in onMoveBegin.

But since the native map below has to know about that vertical stack, I thought maybe it would be better to expose that (with a function like compareLayersZIndex) rather than re-implement the vertical stack logic in the plugin. Or the native map could return the layers in vertical order (instead of adding a compareLayersZIndex function), but that would make it implicit, which is maybe not better).

Now if it is not possible to expose from the native map, then I guess the workaround is to duplicate that state in the plugin 🤔.

from maplibre-plugins-android.

Took a while, but now I understand the problem. The problem is not the rendering order, but that the one that is rendered on top should be the one that is dragged. Instead of introducing this new API, should we not just make sure queryRenderedFeatures returns features in the right order?

By the way I also expect something that is dragged to be put on the top, but that is a separate issue then I suppose.

from maplibre-plugins-android.