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

Don't tie Annotation geometries to Map maxzoom #10791

Merged
merged 1 commit into from
Jan 2, 2018

Conversation

kkaefer
Copy link
Member

@kkaefer kkaefer commented Dec 22, 2017

Instead, geometry generation via GeoJSONVT is now bound to the hardcoded limit of the annotation tile source

// Zoom level 16 is typically sufficient for annotations.
// See https://github.com/mapbox/mapbox-gl-native/issues/10197
{ 0, 16 },

Fixes #10177

Instead, geometry generation via GeoJSONVT is now bound to the hardcoded limit of the annotation tile source.
@kkaefer kkaefer added annotations Annotations on iOS and macOS or markers on Android bug Core The cross-platform C++ core, aka mbgl crash labels Dec 22, 2017
@kkaefer kkaefer modified the milestones: android-v5.3.1, ios-v3.7.2 Dec 22, 2017
options.maxZoom = maxZoom;
// The annotation source is currently hard coded to maxzoom 16, so we're topping out at z16
// here as well.
options.maxZoom = 16;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: could this be a shared constant with RenderAnnotationSource?

Copy link
Contributor

@jfirebaugh jfirebaugh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I looked through the history to see if there was a particular reason to use the map's maxZoom and it dates way back to before we used GeoJSONVT. So if there was, it's almost certainly no longer relevant.

https://github.com/mapbox/mapbox-gl-native/blame/4ba78b6bcfea756c274a05cba61352cd4681ae86/src/mbgl/map/annotation.cpp#L91-L96

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
annotations Annotations on iOS and macOS or markers on Android bug Core The cross-platform C++ core, aka mbgl crash
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants