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

[ios] Support getLeaves (and related) clustering methods #12952

Merged
merged 29 commits into from
Jan 14, 2019

Conversation

julianrex
Copy link
Contributor

@julianrex julianrex commented Sep 25, 2018

Fixes #12442.

This WIP PR adds an MGLPointCluster protocol, and - (id<MGLPointCluster>)cluster to MGLPointFeature which returns non-null for clusters. (MGLPointFeature conforms to MGLPointCluster privately).

We could also consider these options:

  • Add the above cluster method to MGLFeature, for future compatibility (e.g. if supercluster supported features other than points)
  • Instead of a protocol, create a subclass MGLPointFeature 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, that private types (that conform to MGLFeature) will also conform to if they have the appropriate cluster attributes. MGLPointFeatureCluster (a new subclass of MGLPointFeature) conforms to.

(Currently the only supported type that adopts MGLCluster is a subclass of MGLPointFeature.)

This also adds:

  • -[MGLShapeSource leavesOfCluster:offset:limit:]
  • -[MGLShapeSource childrenOfCluster:]
  • -[MGLShapeSource zoomLevelForExpandingCluster:]

exposing features added to supercluster
/cc @tobrun

@julianrex julianrex added iOS Mapbox Maps SDK for iOS ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Sep 25, 2018
@julianrex julianrex self-assigned this Sep 25, 2018
@julianrex julianrex added this to the ios-v4.5.0 milestone Sep 25, 2018
Copy link
Contributor

@captainbarbosa captainbarbosa left a 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.

@@ -305,6 +307,10 @@ MGL_EXPORT
*/
- (NSArray<id <MGLFeature>> *)featuresMatchingPredicate:(nullable NSPredicate *)predicate;

// TODO: doc
Copy link
Contributor

@captainbarbosa captainbarbosa Sep 29, 2018

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.

platform/darwin/src/MGLFeature.mm Outdated Show resolved Hide resolved
platform/darwin/src/MGLShapeSource.h Outdated Show resolved Hide resolved
Copy link
Contributor

@1ec5 1ec5 left a 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.

platform/darwin/src/MGLPointCluster.h Outdated Show resolved Hide resolved
@julianrex
Copy link
Contributor Author

@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

@1ec5
Copy link
Contributor

1ec5 commented Oct 2, 2018

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.

@friedbunny friedbunny removed this from the ios-v4.5.0-gazpacho milestone Oct 9, 2018
@friedbunny friedbunny self-requested a review November 8, 2018 19:22
@julianrex julianrex removed the request for review from fabian-guerra November 13, 2018 22:00
@julianrex julianrex force-pushed the jrex/12442-get-leaves branch 2 times, most recently from 4266214 to 58b1dc7 Compare November 26, 2018 15:45
@julianrex julianrex added macOS Mapbox Maps SDK for macOS Core The cross-platform C++ core, aka mbgl and removed in progress ⚠️ DO NOT MERGE Work in progress, proof of concept, or on hold labels Nov 26, 2018
@julianrex
Copy link
Contributor Author

@tobrun I've updated this PR with latest changes - would you mind checking to see if this matches what you expect?

Copy link
Member

@tobrun tobrun left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@friedbunny friedbunny left a comment

Choose a reason for hiding this comment

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

Two small notes.

platform/darwin/src/MGLShapeSource.h Outdated Show resolved Hide resolved
platform/darwin/test/MGLDocumentationExampleTests.swift Outdated Show resolved Hide resolved
@friedbunny friedbunny added this to the release-iowaska milestone Nov 26, 2018
@julianrex julianrex requested a review from a team January 12, 2019 20:47
@julianrex
Copy link
Contributor Author

julianrex commented Jan 12, 2019

This PR is now the equivalent of the Android PR (#13631) due to the introduction of the queryFeatureExtension API introduced in #13382.

Because of the introduction of this API, this PR has been refactored so that an MGLShape that conforms to MGLCluster is used: specifically this is MGLPointFeatureCluster (a subclass of MGLPointFeature). MGLCluster has not changed other than the static free functions that were used to create cluster subclasses at runtime being turned in to methods of MGLPointFeatureCluster.

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.

@julianrex julianrex requested review from tobrun and removed request for tobrun January 12, 2019 21:10
Copy link
Contributor

@friedbunny friedbunny left a 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/MGLFeature.h Show resolved Hide resolved
platform/darwin/src/MGLFeature.mm Outdated Show resolved Hide resolved
@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
Copy link
Contributor

Choose a reason for hiding this comment

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

☝️

platform/darwin/test/MGLCodingTests.mm Outdated Show resolved Hide resolved
@julianrex julianrex merged commit c8c664f into master Jan 14, 2019
@julianrex julianrex deleted the jrex/12442-get-leaves branch January 14, 2019 20:17
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Core The cross-platform C++ core, aka mbgl iOS Mapbox Maps SDK for iOS macOS Mapbox Maps SDK for macOS
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants