[ios] Add platform annotation views #4801

boundsj · 2016-04-22T14:14:28Z

Implement platform annotation view (UIViews) for iOS.

API Proposal

For native annotation views in the Mapbox SDK, I don't see any reason to stray too far from what a developer coming from the Apple's MapKit might expect. Combined with the existing Mapbox API for adding annotations this would look like:

// existing annotation model management API
-addAnnotation(s):
-removeAnnotation(s):
-selectAnnotation:animated:
...

// a new method to dequeue a reusable annotation view
-dequeueReusableAnnotationViewWithIdentifier:

// a new method in MGLMapViewDelegate protocol
-mapView:viewForAnnotation:

Below is an example of how the new methods noted above might be used in a future implementation. Note that MGLMapView will expect its delegate to supply a UIView that is a subclass of a new MGLAnnotationView. MGLAnnotationView will hold additional meta information about the annotation, will allow for reuse of code to deal with touch interactions, and will help when view reuse is eventually
added.

// In the MGLMapView's delegate implementation
func mapView(mapView: MGLMapView, viewForAnnotation annotation: MGLAnnotation) -> MGLAnnotationView? {
    var customAnnotationView = mapView.dequeueReusableAnnotationViewWithIdentifier("reuse-me") as? SomeMGLAnnotationViewSubclass

    if ((customAnnotationView == nil)) {
        customAnnotationView = SomeMGLAnnotationViewSubclass(annotation: annotation, reuseIdentifier: "reuse-me")
    } else {
        customAnnotationView?.annotation = annotation
    }

    return customAnnotationView
}

Next steps:

Investigate where imageForAnnotation is currently called in map view -- this might inform what happens next...
Investigate Minh's PR for updating an annotation w/o removing it
Introduce new protocol method for obtaining a view
Introduce logic to use native pin if no view or image, use image if image, use view if view
Support for existing interaction API
Introduce view reuse queue
Explore using protocol to handle interactions for MGLAnnotationView annotations

cc @1ec5 @friedbunny @tobrun

1ec5 · 2016-04-25T08:20:17Z

platform/ios/app/MBXAnnotationView.m

+#import "MBXAnnotationView.h"
+
+static const CGFloat size = 35.0;
+static const CGFloat padding = 7.5;


Nit: Globals should be named the same way as classes, in UpperCamelCase with a prefix.

1ec5 · 2016-04-25T09:01:24Z

platform/ios/src/MGLMapView.mm

-            annotationImage.styleIconIdentifier = @(points[i].icon.c_str());
-            id <MGLAnnotation> annotation = annotations[i];
+            id<MGLAnnotation> annotation = annotations[i];
+            NSValue *annotationValue = [NSValue valueWithNonretainedObject:annotation];


The existing code is already more indirect than it should be: we should build the MGLAnnotationContexts upfront, in the loop up above, and store them in a vector. We can assume that Map::addPointAnnotations() adds the annotations in order, so this loop would only need to associate the new tags with their annotation contexts.

Agreed. I will refactor in a future commit.

@1ec5 fyi 4db0d0f

1ec5 · 2016-04-25T16:44:41Z

platform/ios/src/MGLMapView.mm

@@ -750,6 +753,20 @@ - (void)glkView:(__unused GLKView *)view drawInRect:(__unused CGRect)rect
        _mbglMap->render();

        [self updateUserLocationAnnotationView];
+
+        // Hide all annotation views


This code runs 60 times a second due to the CADisplayLink. Intuitively, I’d expect hiding, repositioning, and showing views in this relatively hot code to be a bit of a drag on performance. Can we move this into a separate method that gets called when appropriate, for example in -notifyMapChange:, as well as in -observeValueForKeyPath:ofObject:change:context: when an annotation’s coordinate key path changes?

Definitely. I'll work on that next.

tobrun · 2016-04-27T09:08:32Z

@boundsj I have been running at the Android equivalent and looking into how we can efficiently manage which annotations are currently visible by using MapChangeRegionIsChanging map change event as seen in this PR.

I noticed the following scenario: for the following screen I received the MapChangeRegionIsChanging event and was able to determine the marker shown below:

When I slowly pan to the bottom, I'm not receiving the required map change event and I'm not able to determine that we have 4 point annotations in bounds instead of 1.

For the reference my logs.
In other cases of panning normally or doing a double tap zoom, I'm receiving the events as expected.

@boundsj are you seeing the same behaviour on iOS?

PS. note that when a region change is detected, I'm receiving multiple ones after each other. We might want to look into creating an idling system that will only act on one of these events and will ignore other ones for a certain timeout time. For Android this would make sense because it would limit the amount JNI trips.

cc @1ec5

1ec5 · 2016-05-04T03:18:17Z

platform/ios/src/MGLAnnotationView_Private.h

+
+@property (nonatomic, weak) id<MGLAnnotationViewDelegate> delegate;
+
+// Identifier used to specifiy a reuse queue for the annotation view type.


Nit: /// for single-line documentation comments.

1ec5 · 2016-05-04T05:40:21Z

PS. note that when a region change is detected, I'm receiving multiple ones after each other. We might want to look into creating an idling system that will only act on one of these events and will ignore other ones for a certain timeout time. For Android this would make sense because it would limit the amount JNI trips.

I’m not sure whether the multiple events in succession reflects a problem in mbgl or in the Android SDK itself, but I haven’t personally witnessed this issue for gestures or programmatic animations. The iOS SDK uses a different pattern in which there’s only one listener for map change notifications – the mbgl::View subclass owned by MGLMapView – rather than a separate OnMapChangedListener for every little thing that might need reacting to. So I’m not sure an idling system would be needed on the iOS side.

We should pay special attention to where we update the annotation layer. Currently, in this PR, all annotations are being updated in -[MGLMapView glkView:drawInRect:], which gets called 60 times per second as long as the view is in the foreground. Even if the annotation layer appears to be in sync with the map, the annotations are probably bogging down the map itself, causing it to skip frames. It would be valuable to move this work to -notifyMapChange:, specifically in response to both MapChangeDidFinishRenderingFrame and MapChangeDidFinishRenderingFrameFullyRendered. If responding to these two notifications isn’t sufficient to ensure that the views are synchronized with the map, that’s an mbgl bug we should prioritize.

1ec5 · 2016-05-04T05:45:01Z

platform/ios/src/MGLMapView.mm

+            if (annotationContext.viewReuseIdentifier)
+            {
+                NSMutableArray *annotationViewReuseQueue = [self annotationViewReuseQueueForIdentifier:annotationContext.viewReuseIdentifier];
+                if (![annotationViewReuseQueue containsObject:annotationView])


Do the semantics of the reuse queue require this search to form a pointer comparison or object comparison on each view in the reuse queue? If the latter, MGLAnnotationView should have custom implementations of -isEqual: and -hash.

It's a pointer comparison. This guard just protects against adding a reference to any exact same view object that was previously added to the queue. I used an array for the queue since that API feels better (as a queue) in most places -- except here where a set might make things clearer. In any case, the intention is to capture any view object that goes offscreen in the appropriate queue so that it can be reused

Add an UIView subclass that can be used as the base class for all client provided UIViews for annotations. Teach MGLMapView to be able to display annotation views over the map if provided by the client delegate. For now, if the delegate provides a UIView then it will be used. If not, the map view will fall back to the old strategy of using GL annotations with an image provided by the delegate or a default image if not. The map keeps a reuse queue and will store annotation views that are panned offscreen in the queue if the application developer supplied a reuse queue identifer. The views in the queue are reused when more annotation views are required. This view reuse provides a performance gain when many annotations are shown and most of them are offscreen. iosapp now implements the new delegate method to supply a native view. Add a playground to the workspace to facilitate experimentation with new features. A playground is capable of importing frameworks if it exists in the same workspace that builds the imported framework. The initial playground demonstrates annotation views. This also fixes a crash due to nullptr in annotations array If the `annotations` method is called while the user dot's callout view was showing, the userdot annotation is represented as null in the annotation context map. This caused a crash when the null pointer was attempted to be converted into an NSArray via C array. This protects against this bug by filtering out such a null annotation.

This change eliminates the call to the problematic `annotationTagsInRect:` by using the annotation views themselves and the map view to check if a view is visible or not.

friedbunny · 2016-05-14T02:23:28Z

1ec5 · 2016-05-14T05:24:37Z

Yes! Filed some issues for tail work: #5035 #5036 #5037 #5038 #5039 #5040.

1ec5 · 2016-05-15T18:09:05Z

Updated the changelog in 54381d2.

boundsj · 2016-05-17T21:19:02Z

I added an additional issue to the tail work pile: #5059

boundsj self-assigned this Apr 22, 2016

boundsj added feature iOS Mapbox Maps SDK for iOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Apr 22, 2016

boundsj force-pushed the boundsj_ios_platform_annotations branch from d1b32bc to a4b3ff5 Compare April 22, 2016 20:55

1ec5 mentioned this pull request Apr 25, 2016

VoiceOver accessibility #1496

Merged

8 tasks

1ec5 reviewed Apr 25, 2016
View reviewed changes

1ec5 mentioned this pull request Apr 25, 2016

platform views vs. core GL sprites for annotations & callouts #3116

Closed

1ec5 reviewed Apr 25, 2016
View reviewed changes

1ec5 added this to the ios-v3.3.0 milestone Apr 25, 2016

1ec5 reviewed Apr 25, 2016
View reviewed changes

1ec5 mentioned this pull request May 3, 2016

Tweak these projects to work with MapBox SDK? #4933

Closed

1ec5 reviewed May 4, 2016
View reviewed changes

1ec5 mentioned this pull request May 13, 2016

If there are too many annotation images, some annotation images are invisible #3185

Closed

boundsj force-pushed the boundsj_ios_platform_annotations branch from dbd83a4 to b1251cc Compare May 13, 2016 20:00

boundsj added 3 commits May 13, 2016 15:51

[ios] Refactor of annotation view updates

fb759e8

This change eliminates the call to the problematic `annotationTagsInRect:` by using the annotation views themselves and the map view to check if a view is visible or not.

[ios] Add method documentation for view based annotations.

1ee1491

boundsj force-pushed the boundsj_ios_platform_annotations branch from b1251cc to 1ee1491 Compare May 13, 2016 22:57

boundsj merged commit 1ee1491 into master May 13, 2016

friedbunny deleted the boundsj_ios_platform_annotations branch May 14, 2016 02:13

This was referenced May 14, 2016

Annotation views on macOS #5035

Closed

iOS annotation views #1784

Closed

1ec5 removed the ✓ ready for review label May 14, 2016

This was referenced May 14, 2016

allow iOS per-frame callback #1981

Closed

Annotation views are inaccessible to VoiceOver #5038

Closed

1ec5 mentioned this pull request May 14, 2016

MGLUserLocationAnnotationView should inherit from MGLAnnotationView #5039

Closed

1ec5 added a commit that referenced this pull request May 15, 2016

[ios] Updated changelog for #4801

54381d2

tobrun mentioned this pull request May 17, 2016

Expose a generic Android View Marker API #3276

Closed

19 tasks

boundsj added the annotations Annotations on iOS and macOS or markers on Android label May 17, 2016

boundsj mentioned this pull request May 25, 2016

Add a container view for UIView based annotations #5140

Closed

This was referenced May 26, 2016

getPointAnnotationsInBounds with a rotated map #5151

Closed

Add MGLAnnotationView to jazzy TOC #5185

Merged

Align annotation rotation and pitch with map #2528

Closed

This was referenced May 31, 2016

[ios] Add annotation container view #5194

Merged

AnnotationManager::getPointAnnotationsInBounds() should map IDs to screen coordinates #4443

Closed

1ec5 mentioned this pull request Jul 25, 2016

unnecessary performance overhead for non-view annotations on iOS #5787

Closed

1ec5 mentioned this pull request Sep 18, 2016

View-backed annotations nitaliano/react-native-mapbox-gl#404

Closed

boundsj mentioned this pull request Mar 24, 2017

MGLMapView sets the tint color of annotation views when its tint color is changed. #8522

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[ios] Add platform annotation views #4801

[ios] Add platform annotation views #4801

boundsj commented Apr 22, 2016 •

edited

Loading

1ec5 Apr 25, 2016

1ec5 Apr 25, 2016

boundsj Apr 25, 2016

boundsj Apr 26, 2016

1ec5 Apr 25, 2016

boundsj Apr 25, 2016

tobrun commented Apr 27, 2016

1ec5 May 4, 2016

1ec5 commented May 4, 2016

1ec5 May 4, 2016

boundsj May 6, 2016

friedbunny commented May 14, 2016

1ec5 commented May 14, 2016 •

edited

Loading

1ec5 commented May 15, 2016

boundsj commented May 17, 2016


		@property (nonatomic, weak) id<MGLAnnotationViewDelegate> delegate;

		// Identifier used to specifiy a reuse queue for the annotation view type.

[ios] Add platform annotation views #4801

[ios] Add platform annotation views #4801

Conversation

boundsj commented Apr 22, 2016 • edited Loading

API Proposal

Next steps:

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

tobrun commented Apr 27, 2016

Choose a reason for hiding this comment

1ec5 commented May 4, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

friedbunny commented May 14, 2016

1ec5 commented May 14, 2016 • edited Loading

1ec5 commented May 15, 2016

boundsj commented May 17, 2016

boundsj commented Apr 22, 2016 •

edited

Loading

1ec5 commented May 14, 2016 •

edited

Loading