-
Notifications
You must be signed in to change notification settings - Fork 1.3k
[ios] Support getLeaves (and related) clustering methods #12952
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Happy to review more docs as you write them.
platform/darwin/src/MGLShapeSource.h
Outdated
@@ -305,6 +307,10 @@ MGL_EXPORT | |||
*/ | |||
- (NSArray<id <MGLFeature>> *)featuresMatchingPredicate:(nullable NSPredicate *)predicate; | |||
|
|||
// TODO: doc |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When you do write the docs for this part, I think its a good idea to add extra clarification on what are leaves vs. clusters since the metaphor is a bit odd.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Really curious to what extent we can align with MapKit’s built-in clustering API. Certainly there are structural differences in the underlying data and types that we have to work with, but it would be helpful to be able to say that such-and-such clustering-related type in MapKit is analogous to such-and-such type in this SDK.
@1ec5 I think that's a great goal - I'm hoping that a simpler interface along the lines of MapKit's can be something that can follow after and inform this PR. /cc @chloekraw |
Reading the MapKit documentation more closely, I think there may be opportunities to align terminology but not the API itself, because MapKit’s clustering feature is rather application-driven, whereas the application passively consumes the contents of the style in the API being presented here. We’ll have more opportunities to align API members when we get around to implementing #5814 and especially #5815. |
14dafd0
to
23474b6
Compare
4266214
to
58b1dc7
Compare
@tobrun I've updated this PR with latest changes - would you mind checking to see if this matches what you expect? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Two small notes.
…nsion required MGLShape)
504b0f4
to
72381ba
Compare
This PR is now the equivalent of the Android PR (#13631) due to the introduction of the Because of the introduction of this API, this PR has been refactored so that an If it turns out that types other than MGLPointFeature can be clustered, we can revisit. Though I suspect that we'll want to address #7454 before then. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Tiny nits and a couple small documentation suggestions.
platform/darwin/src/MGLShapeSource.h
Outdated
@param cluster An object that conforms to the `MGLCluster` protocol. Currently | ||
the only types that can conform are private subclasses of `MGLPointFeature`. | ||
@param offset Number of features to skip. | ||
@param limit Maximum number of features to return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
☝️
Fixes #12442.
This WIP PR adds anMGLPointCluster
protocol, and- (id<MGLPointCluster>)cluster
toMGLPointFeature
which returns non-null for clusters. (MGLPointFeature
conforms toMGLPointCluster
privately).We could also consider these options:Add the abovecluster
method toMGLFeature
, for future compatibility (e.g. if supercluster supported features other than points)Instead of a protocol, create a subclassMGLPointFeature
for point clusters, e.g.MGLPointClusterFeature
. This may be preferable, but again if other features end up being clustered we could suffer an explosion of types.Still TBD: documentation, tests, macos project.This PR introduces a new public protocol,
MGLCluster
, thatprivate types (that conform toMGLFeature
) will also conform to if they have the appropriate cluster attributes.MGLPointFeatureCluster
(a new subclass ofMGLPointFeature
) conforms to.(Currently the only supported type that adopts
MGLCluster
is a subclass ofMGLPointFeature
.)This also adds:
-[MGLShapeSource leavesOfCluster:offset:limit:]
-[MGLShapeSource childrenOfCluster:]
-[MGLShapeSource zoomLevelForExpandingCluster:]
exposing features added to supercluster
/cc @tobrun