Skip to content
This repository has been archived by the owner on Aug 8, 2023. It is now read-only.

MGLMapView should map annotation IDs to annotation metadata #3159

Closed
1ec5 opened this issue Dec 1, 2015 · 3 comments · Fixed by #3261
Closed

MGLMapView should map annotation IDs to annotation metadata #3159

1ec5 opened this issue Dec 1, 2015 · 3 comments · Fixed by #3261
Assignees
Labels
iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage refactor
Milestone

Comments

@1ec5
Copy link
Contributor

1ec5 commented Dec 1, 2015

MGLMapView holds an NSMapTable mapping MGLAnnotation objects to dictionaries containing IDs and other metadata. The problem is that IDs aren’t metadata; they’re the unique key. So many operations that should be simple lookups end up requiring expensive loops or are simply impossible. For example, this loop through all annotation metadata can be very expensive, and it isn’t possible to put a fix for #1504 in there because a) we’re iterating over all the annotations, not just the nearby ones, and b) the annotation ID is inaccessible.

I ran into this issue while working on #1496 and again while trying to port the fix for #1504 back from OS X to iOS. For the OS X version of MGLMapView in #3135, I started out with a mapping from annotation IDs (MGLAnnotationTag) to annotation context objects, and it has worked out very well. There are couple places where I need to get an ID from an annotation, and performance is fine in those places.

/cc @incanus

@1ec5 1ec5 added iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage refactor labels Dec 1, 2015
@1ec5 1ec5 self-assigned this Dec 1, 2015
@1ec5 1ec5 added this to the ios-v3.1.0 milestone Dec 1, 2015
1ec5 added a commit that referenced this issue Dec 11, 2015
MGLMapView now maps “annotation tags” (Cocoa-speak for numeric annotation IDs) to context objects that pair annotations with their identifiers. Previously, MGLMapView mapped the annotation objects themselves to context objects that contained annotation IDs, but this caused most operations involving annotations to be rather expensive: since mbgl tracks annotations by ID, most of the time you know the ID and need either the annotation or some piece of metadata about it.

Along for the ride, geographic methods and support for alignment rect insets on annotation images have been ported from #3135 for OS X. Hit testing now works much like in #3135, with the exception that UIKit doesn’t support hit testing based on pixel opacity. -convertRect:toLatLngBoundsFromView: returns a world-spanning bounds when the world is wrapping.

Fixes #3159.
1ec5 added a commit that referenced this issue Dec 14, 2015
MGLMapView now maps “annotation tags” (Cocoa-speak for numeric annotation IDs) to context objects that pair annotations with their identifiers. Previously, MGLMapView mapped the annotation objects themselves to context objects that contained annotation IDs, but this caused most operations involving annotations to be rather expensive: since mbgl tracks annotations by ID, most of the time you know the ID and need either the annotation or some piece of metadata about it.

Along for the ride, geographic methods and support for alignment rect insets on annotation images have been ported from #3135 for OS X. Hit testing now works much like in #3135, with the exception that UIKit doesn’t support hit testing based on pixel opacity. -convertRect:toLatLngBoundsFromView: returns a world-spanning bounds when the world is wrapping.

Fixes #3159.
@t-yoshii
Copy link

After this change, removeAnnotation() and removeAnnotations() of MGLMapView crashes when annotation has not been added, with following assertion.

NSAssert(annotationTag != MGLAnnotationTagNotFound, @"No ID for annotation %@", annotation);

I think removeAnnotation can be called whether the annotation is added or not.

@1ec5
Copy link
Contributor Author

1ec5 commented Jan 25, 2016

Thanks, @t-yoshii. Fixed in #3685.

@t-yoshii
Copy link

Thank you for your quick response!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
iOS Mapbox Maps SDK for iOS performance Speed, stability, CPU usage, memory usage, or power usage refactor
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants