[ios, macos] Add selection support to MGLMultiPoint annotations. #9984

fabian-guerra · 2017-09-13T02:34:21Z

WIP fixes #2082

I modified the initial implementation from #9755 to support callouts and reuse our current API.

Next steps:

Test current annotations API with MGLMultiPoint support.
Try a custom MGLMultiPoint callout.
Test with MGLPointAnnotations.
Add macOS support.
Update changelogs.

This is how it looks.

1ec5

If you don’t mind, please make the same changes to the macOS SDK’s implementation of MGLMapView so the two implementations don’t diverge any further. Thanks!

1ec5 · 2017-09-13T16:32:30Z

platform/ios/src/MGLMapView.mm

@@ -285,6 +289,7 @@ @implementation MGLMapView

    MGLAnnotationTagContextMap _annotationContextsByAnnotationTag;
    MGLAnnotationObjectTagMap _annotationTagsByAnnotation;
+    MGLShapeAnnotationObjectLayerIDMap _shapeAnnotationLayerIDsByAnnotation;


_annotationTagsByAnnotation already associates each shape annotation with an annotation tag. Do we need a separate ivar to track the layer IDs used by mbgl? Seems like we can prefix the annotation tag on the fly as needed.

1ec5 · 2017-09-13T16:33:19Z

platform/ios/src/MGLMapView.mm

@@ -152,6 +152,10 @@ typedef NS_ENUM(NSUInteger, MGLUserTrackingState) {
 /// Mapping from an annotation object to an annotation tag.
 typedef std::map<id<MGLAnnotation>, MGLAnnotationTag> MGLAnnotationObjectTagMap;

+typedef std::map<id<MGLAnnotation>, std::string> MGLShapeAnnotationObjectLayerIDMap;
+
+static NSString *const  MGLLayerIDShapeAnnotation = @"com.mapbox.annotations.shape.";


Let’s call this constant MGLShapeAnnotationLayerIdentifierPrefix, by analogy with MGLAnnotationSpritePrefix.

1ec5 · 2017-09-13T16:38:48Z

platform/ios/src/MGLMapView.mm

@@ -3646,6 +3667,11 @@ - (MGLAnnotationTag)annotationTagAtPoint:(CGPoint)point persistingResults:(BOOL)
            CGRect annotationRect;

            MGLAnnotationView *annotationView = annotationContext.annotationView;
+
+            if (queryingShapeAnnotations) {
+                return false;


If queryingShapeAnnotations is true, then we still end up running this lambda once for each entry in nearbyAnnotations, which can be expensive. Bring this queryingShapeAnnotations check outside the lambda so that the lambda doesn’t run at all.

1ec5 · 2017-09-13T18:40:05Z

platform/ios/src/MGLMapView.mm

+        { CGRectGetMaxX(rect), CGRectGetMaxY(rect) },
+    };
+
+    std::vector<MGLAnnotationTag> nearbyAnnotations;


This appears to be a list of all the annotation tags, not just the nearby ones.

It contains nearby ones. You can look at line 3799:

return nearbyAnnotations = _mbglMap->queryShapeAnnotations(screenBox, { optionalLayerIDs });

Otherwise returns an empty vector.

What I meant was that this vector contains all the annotations within the specified rect. It just happens that this method is called with a rect that represents the area near the tap gesture, but that isn’t required by this method. Perhaps annotationsInRect would be more self-explanatory.

1ec5 · 2017-09-13T18:41:34Z

platform/ios/src/MGLMapView.mm

@@ -3960,6 +4010,13 @@ - (CGRect)positioningRectForCalloutForAnnotationWithTag:(MGLAnnotationTag)annota
    {
        return CGRectZero;
    }
+
+    if ([annotation isKindOfClass:[MGLMultiPoint class]]) {
+        CLLocationCoordinate2D origin = annotation.coordinate;


If the coordinate lies outside the current viewport, we should move it somewhere within the current viewport. This might require us to clip the geometry to the viewport (in a temporary new shape object) before calculating its centroid.

Do we have an API for clipping?

turf-difference returns the difference between two polygons (in this case, the shape and the visible coordinate rect). But we don’t have a Swift port at the ready. mbgl may be using some clipping utilities that we can take advantage of.

I'm going to cut a ticket for this. I will add a new API for clipping.

The reason I brought this up is that the callout might end up being completely invisible, making the user think the map did nothing in response to the tap. But it does make sense to treat the clipping behavior as tail work.

It just occurred to me that the best fix for this issue would be to automatically pan to the selected shape annotation’s centroid, to show as much of the shape as possible, before showing the callout. We could fix #3249 at the same time.

Also @frederoni pointed that it may be useful to set the callout anchor to where the user tapped in case the centroid is off screen. What do you think?

Anchoring the callout at the tap location would also be reasonable. If I’m not mistaken, that’s how polygon selection works in Leaflet, so there’s precedent for it. #3249 would still be desirable, but we can do that separately.

1ec5 · 2017-09-23T01:02:36Z

platform/ios/src/MGLMapView.mm

+        { CGRectGetMaxX(rect), CGRectGetMaxY(rect) },
+    };
+
+    std::vector<MGLAnnotationTag> nearbyAnnotations;


What I meant was that this vector contains all the annotations within the specified rect. It just happens that this method is called with a rect that represents the area near the tap gesture, but that isn’t required by this method. Perhaps annotationsInRect would be more self-explanatory.

1ec5 · 2017-09-23T01:03:06Z

platform/ios/src/MGLMapView.mm

@@ -152,6 +152,8 @@ typedef NS_ENUM(NSUInteger, MGLUserTrackingState) {
 /// Mapping from an annotation object to an annotation tag.
 typedef std::map<id<MGLAnnotation>, MGLAnnotationTag> MGLAnnotationObjectTagMap;

+static NSString *const  MGLShapeAnnotationLayerIdentifierPrefix = @"com.mapbox.annotations.shape.";


This constant should have a documentation comment.

1ec5 · 2017-09-23T01:04:58Z

platform/ios/src/MGLMapView.mm

+            auto end = std::remove_if(nearbyAnnotations.begin(), nearbyAnnotations.end(),
+                                      [&](const MGLAnnotationTag annotationTag)
+                                      {
+                                          id <MGLAnnotation> annotation = [self annotationWithTag:annotationTag];


The indentation is a bit extreme here. I realize Xcode did this automatically, but would you mind indenting this lambda by only one tab beyond the auto instead of aligning with the opening parenthesis of the remove_if() call? (In this situation, I’d probably use the ⇧⌥⌘V shortcut to preserve indentation then indent manually.) Alternatively, consider breaking the lambda out into a separate local variable, which will make this code easier to read.

1ec5 · 2017-09-23T01:09:15Z

platform/ios/src/MGLMapView.mm

@@ -3960,6 +4010,13 @@ - (CGRect)positioningRectForCalloutForAnnotationWithTag:(MGLAnnotationTag)annota
    {
        return CGRectZero;
    }
+
+    if ([annotation isKindOfClass:[MGLMultiPoint class]]) {
+        CLLocationCoordinate2D origin = annotation.coordinate;


The reason I brought this up is that the callout might end up being completely invisible, making the user think the map did nothing in response to the tap. But it does make sense to treat the clipping behavior as tail work.

1ec5 · 2017-09-23T01:15:09Z

platform/ios/src/MGLMapView.mm

+        NSString *layerID;
+        for (const auto &annotation : _annotationTagsByAnnotation) {
+            layerID = [NSString stringWithFormat:@"%@%u", MGLShapeAnnotationLayerIdentifierPrefix, annotation.second];
+            layerIDs.push_back(layerID.UTF8String);


The dedicated queryShapeAnnotations() method should only ever return features in the shape layers; it shouldn’t be necessary for callers to build their own lists of layer IDs. Push this logic down to map.cpp, and pull this string out into mbgl::AnnotationManager so the code in map.cpp can reuse it.

1ec5 · 2017-10-12T19:29:32Z

platform/ios/src/MGLMapView.mm

@@ -3246,6 +3246,11 @@ - (void)removeStyleClass:(NSString *)styleClass
    }

    std::vector<MGLAnnotationTag> annotationTags = [self annotationTagsInRect:rect];
+
+    if (!annotationTags.size()) {
+        annotationTags = [self shapeAnnotationTagsInRect:rect];


If a point annotation and a shape annotation are both visible, this method should return both. That’s what I’d expect after reading this public method’s documentation, anyways.

1ec5 · 2017-10-12T19:30:36Z

platform/ios/src/MGLMapView.mm

+                NSAssert(annotation, @"Unknown annotation found nearby tap");
+                if ( ! annotation)
+                {
+                  return true;


Nit: funky indentation.

1ec5 · 2017-10-12T19:39:42Z

src/mbgl/renderer/renderer.cpp

+    if (layerIDs.size()) {
+        shapeLayerIDs.reserve(layerIDs.size());
+        for (const auto &layerID : layerIDs) {
+            shapeLayerIDs.push_back(AnnotationManager::ShapeLayerID + layerID);


mbgl::Renderer::Impl should have a method queryShapeAnnotations() that figures out all the shape layer IDs on its own. mbgl::Renderer::Impl has a layerImpls member that lists all the style layers; iterate over that vector to get each layer that has AnnotationManager::ShapeLayerID in its ID. In the end, -[MGLMapView shapeAnnotationTagsInRect:] shouldn’t have to build a layerIDs vector at all.

1ec5

The iOS/macOS side of this PR is looking better, although I still have concerns about the behavior where -visibleAnnotationsInRect: (a public method that can be used for more than tap selection) only returns shape annotations if no point annotations are nearby.

As for the mbgl side, I’d appreciate a second pair of eyes from @asheemmamoowala.

1ec5 · 2017-10-13T21:15:53Z

platform/ios/src/MGLMapView.mm

+    std::vector<MGLAnnotationTag> shapeAnnotationTags = [self shapeAnnotationTagsInRect:rect];
+
+    if (!shapeAnnotationTags.size()) {
+        annotationTags.insert(annotationTags.end(), shapeAnnotationTags.begin(), shapeAnnotationTags.end());


My point in #9984 (comment) stands, but moreover, I think this code won’t do anything: if shapeAnnotationTags is empty, it adds the contents of shapeAnnotationTags to annotationTags.

@1ec5 You're right I made the corresponding changes.

1ec5 · 2017-10-13T21:22:00Z

src/mbgl/renderer/renderer_impl.cpp

+std::vector<Feature> Renderer::Impl::queryShapeAnnotations(const ScreenLineString& geometry) const {
+    std::vector<const RenderLayer*> layers;
+    RenderedQueryOptions options;
+    options.layerIDs = {{ AnnotationManager::ShapeLayerID }};


There isn’t ever a layer with this ID, is there? Does mbgl use options.layerIDs for anything past this point?

asheemmamoowala · 2017-10-14T00:06:57Z

src/mbgl/renderer/renderer_impl.cpp

@@ -655,6 +655,52 @@ std::vector<Feature> Renderer::Impl::queryRenderedFeatures(const ScreenLineStrin

    return result;
 }
+
+std::vector<Feature> Renderer::Impl::queryShapeAnnotations(const ScreenLineString& geometry) const {
+    std::vector<const RenderLayer*> layers;


nit var name: shapeAnnotationLayers

asheemmamoowala · 2017-10-14T00:17:02Z

src/mbgl/renderer/renderer.cpp

@@ -52,6 +52,17 @@ std::vector<Feature> Renderer::queryRenderedFeatures(const ScreenBox& box, const
            options
    );
 }
+
+std::vector<Feature> Renderer::queryRenderedSourceFeatures(const ScreenBox& box) const {


This method could be removed and it's logic encapsulated in Renderer::queryShapeAnnotations

Technically both methods should return different annotation types with different parameters. What I did is refactor the logic that was common to both queryPointAnnotations and queryShapeAnnotations.

I don't follow, queryPointAnnotations calls queryRenderedFeatures.
Additionally, this method only calls queryShapeAnnotations, but has a name that implies it is a generic query for a source.

asheemmamoowala · 2017-10-14T00:18:33Z

src/mbgl/renderer/renderer_impl.cpp

+                layers.emplace_back(layer);
+            }
+        }
+    }


The rest of this method is identical to Renderer::Impl::queryRenderedFeatures. Can you refactor the two methods to share the querying?

asheemmamoowala · 2017-10-17T21:07:22Z

src/mbgl/renderer/renderer_impl.cpp

    std::unordered_set<std::string> sourceIDs;
    for (const RenderLayer* layer : layers) {
        sourceIDs.emplace(layer->baseImpl->source);
    }
-
+    


nit: Tabs on empty lines

Renderer::queryPointAnnotations calls queryRenderedFeatures specifying the layer it should query. In this case point annotations are rendered in a single layer. You can pass all the layer ID's for the shape annotations; it was already originally designed this way as you can read in this comment we decided to change that to let the rendered figures out all the shape layer IDs on its own.

Which name do you think it would be more explicit than queryShapeAnnotations?

I think queryRenderedSourceFeatures could be renamed to queryShapeAnnotationFeatures or better yet collapsed into queryShapeAnnotations

asheemmamoowala · 2017-10-17T21:10:57Z

src/mbgl/renderer/renderer.cpp

@@ -52,6 +52,17 @@ std::vector<Feature> Renderer::queryRenderedFeatures(const ScreenBox& box, const
            options
    );
 }
+
+std::vector<Feature> Renderer::queryRenderedSourceFeatures(const ScreenBox& box) const {


I don't follow, queryPointAnnotations calls queryRenderedFeatures.
Additionally, this method only calls queryShapeAnnotations, but has a name that implies it is a generic query for a source.

1ec5 · 2017-10-18T19:12:24Z

platform/ios/src/MGLMapView.mm

@@ -3762,61 +3768,69 @@ - (MGLAnnotationTag)annotationTagAtPoint:(CGPoint)point persistingResults:(BOOL)
    queryRect = CGRectInset(queryRect, -MGLAnnotationImagePaddingForHitTest,
                            -MGLAnnotationImagePaddingForHitTest);
    std::vector<MGLAnnotationTag> nearbyAnnotations = [self annotationTagsInRect:queryRect];
+    BOOL queryingShapeAnnotations = NO;
+
+    if (!nearbyAnnotations.size()) {


If there is both a point annotation and a shape annotation beneath the passed-in point, this method should return the point annotation the first time this method is called and the shape annotation the second time, as long as persist is set to YES. That way the shape annotation remains accessible even when there is an overlapping point annotation.

/ref #9984 (comment)

I’m OK with this detail being tail work, by the way. Overall, this PR is looking good, especially after the last few changes in core.

lawrencelm · 2018-01-02T20:37:43Z

is there any example on how to use this to tap on a polyline for objective-C? I took a look at the pull request but couldn't figure out how to use it

fabian-guerra · 2018-01-02T20:46:53Z

Hi, @lawrencelm. The behavior is similar to annotations try:

MGLPolyline *line = [MGLPolyline polylineWithCoordinates:lineCoordinates count:4];
line.title = @"My MGLPolyline";

Tap the line.

lawrencelm · 2018-01-02T20:51:14Z

thanks! does this also work for MGLPolylineFeature?

lawrencelm · 2018-01-02T21:31:50Z

I tried adding a .title and then tapping on the polyline and it didn't work. I also tried changing from MGLPolylineFeature to MGLPolyline, and adding it in different ways ([self.mapView addOverlay] and [self.mapView addAnnotation]).

This is my code:

                    CLLocationCoordinate2D *altRouteCoordinates = malloc(altRoute.coordinateCount * sizeof(CLLocationCoordinate2D));
                    [altRoute getCoordinates:altRouteCoordinates];
                    MGLPolylineFeature *altPolyline = [MGLPolylineFeature polylineWithCoordinates:altRouteCoordinates count:altRoute.coordinateCount];
                    MGLShapeSource *altSource = [[MGLShapeSource alloc] initWithIdentifier:[NSString stringWithFormat:@"route-source-%i",r] shape:altPolyline options:nil];
                    
                    MGLLineStyleLayer *altLine = [[MGLLineStyleLayer alloc] initWithIdentifier:[NSString stringWithFormat:@"route-style-%i",r] source:altSource];
                    
                    altLine.lineJoin = [MGLStyleValue valueWithRawValue:[NSValue valueWithMGLLineJoin:MGLLineJoinRound]];
                    altLine.lineCap = [MGLStyleValue valueWithRawValue:[NSValue valueWithMGLLineCap:MGLLineCapRound]];
                    altLine.lineColor = [MGLStyleValue valueWithRawValue:[UIColor grayColor]];
                    // Use a style function to smoothly adjust the line width from 3pt to 30pt between zoom levels 14 and 18. The `interpolationBase` parameter allows the values to interpolate along an exponential curve.
                    altLine.lineWidth = [MGLStyleValue valueWithInterpolationMode:MGLInterpolationModeExponential
                                                                        cameraStops:@{
                                                                                      @14: [MGLStyleValue valueWithRawValue:@3],
                                                                                      @18: [MGLStyleValue valueWithRawValue:@30]
                                                                                      }
                                                                            options:@{MGLStyleFunctionOptionDefaultValue:@1.5}];
                    //[altPolyline addTarget:self action:@selector(switchRoutes) forControlEvents:UIControlEventTouchUpInside];
                    altPolyline.title = @"Tap me";
                    
                    [self.mapView.style addSource:altSource];
                    [self.mapView.style addLayer:altLine];

fabian-guerra · 2018-01-02T21:42:32Z

Try

MGLPolyline *line = [MGLPolyline polylineWithCoordinates:lineCoordinates count:4];
line.title = @"My MGLPolyline";
[self.mapView addAnnotation:line];

In the code you posted I don't see where you add altPolyline to the map.

Also please open a new issue if shape selection is not working.

lawrencelm · 2018-01-02T22:00:55Z

Works now! I was making a silly mistake where I was deleting annotations. Thanks a ton!

1ec5 · 2018-01-06T06:52:07Z

does this also work for MGLPolylineFeature?

No, if you use the runtime styling API (as in a source and a layer), you’d have to implement interactivity manually and also manage the current selection. #6515 would alleviate a little of that.

fabian-guerra added annotations Annotations on iOS and macOS or markers on Android iOS Mapbox Maps SDK for iOS labels Sep 13, 2017

fabian-guerra added this to the ios-v3.6.3 milestone Sep 13, 2017

fabian-guerra self-assigned this Sep 13, 2017

fabian-guerra requested review from boundsj and 1ec5 September 13, 2017 02:34

1ec5 reviewed Sep 13, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-shape-callout-2082 branch from 174da96 to 7152903 Compare September 15, 2017 17:35

boundsj removed this from the ios-v3.6.3 milestone Sep 18, 2017

boundsj mentioned this pull request Sep 18, 2017

[ios, macos] Add shapes annotations select/deselect delegates. #9755

Closed

fabian-guerra force-pushed the fabian-shape-callout-2082 branch 3 times, most recently from e71320c to 7408d21 Compare September 22, 2017 14:16

fabian-guerra added this to the ios-v3.7.0 milestone Sep 22, 2017

1ec5 reviewed Sep 23, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-shape-callout-2082 branch from 7408d21 to 27b2993 Compare October 9, 2017 21:15

fabian-guerra changed the base branch from release-ios-v3.6.0-android-v5.1.0 to release-agua October 9, 2017 21:15

fabian-guerra force-pushed the fabian-shape-callout-2082 branch 3 times, most recently from 8d81d5e to a69461d Compare October 10, 2017 00:10

1ec5 reviewed Oct 12, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-shape-callout-2082 branch 3 times, most recently from f9580a7 to 6d667e4 Compare October 13, 2017 20:35

1ec5 reviewed Oct 13, 2017

View reviewed changes

1ec5 requested a review from asheemmamoowala October 13, 2017 21:28

fabian-guerra force-pushed the fabian-shape-callout-2082 branch from 6d667e4 to fe5bc8d Compare October 13, 2017 21:59

asheemmamoowala suggested changes Oct 14, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-shape-callout-2082 branch from fe5bc8d to 8d39252 Compare October 17, 2017 18:07

asheemmamoowala reviewed Oct 17, 2017

View reviewed changes

fabian-guerra force-pushed the fabian-shape-callout-2082 branch 2 times, most recently from 93f1785 to 447664f Compare October 18, 2017 01:51

asheemmamoowala approved these changes Oct 18, 2017

View reviewed changes

fabian-guerra added 2 commits October 18, 2017 13:44

[ios, macos] Add selection support to MGLMultiPoint annotations.

9db1c63

[ios, macos] Update changelogs.

4c6e4d8

fabian-guerra force-pushed the fabian-shape-callout-2082 branch from cbef83c to 4c6e4d8 Compare October 18, 2017 17:45

1ec5 reviewed Oct 18, 2017

View reviewed changes

fabian-guerra merged commit b7b8bae into release-agua Oct 18, 2017

fabian-guerra deleted the fabian-shape-callout-2082 branch October 18, 2017 19:19

This was referenced Oct 18, 2017

[ios, macos] Return a shape annotation the second time, as long as persist is set to YES. #10228

Closed

[ios, macos] Display the shape annotation's callout when the center coordinate is off viewport. #10229

Closed

This was referenced Oct 19, 2017

Use queryShapeAnnotations for clickable shape annotations #10234

Closed

Move shape annotation click handling to core #10267

Merged

This was referenced Nov 27, 2017

Prevent specific MGLAnnotation objects from being selected #10539

Closed

[ios, macos] Fix an issue that triggers didSelectAnnotations #10538

Merged

fabian-guerra mentioned this pull request Dec 20, 2017

MGLPointAnnotations with subtitle are not shown well on v3.7.1 #10745

Closed

This was referenced Dec 20, 2017

Support selection of shape and polyline annotations #2082

Closed

[ios] Adds the ability to get selection events for shape annotations #5502

Closed

[ios, macos] Add selection support to MGLMultiPoint annotations. #9984

[ios, macos] Add selection support to MGLMultiPoint annotations. #9984

Conversation

fabian-guerra commented Sep 13, 2017 • edited Loading

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 Oct 13, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

1ec5 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lawrencelm commented Jan 2, 2018 • edited Loading

fabian-guerra commented Jan 2, 2018

lawrencelm commented Jan 2, 2018

lawrencelm commented Jan 2, 2018 • edited Loading

fabian-guerra commented Jan 2, 2018

lawrencelm commented Jan 2, 2018

1ec5 commented Jan 6, 2018

fabian-guerra commented Sep 13, 2017 •

edited

Loading

1ec5 Oct 13, 2017 •

edited

Loading

lawrencelm commented Jan 2, 2018 •

edited

Loading

lawrencelm commented Jan 2, 2018 •

edited

Loading