Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add methods for inspecting GeoJSON clusters #6829

Merged
merged 4 commits into from
Jun 21, 2018
Merged

Conversation

mourner
Copy link
Member

@mourner mourner commented Jun 18, 2018

Closes #3318. Introduces 3 new methods to clustered GeoJSON sources:

  • getChildren for returning cluster's immediate children (from the next zoom)
  • getLeaves for returning original points that belong to the given cluster, with pagination support.
  • getClusterExpansionZoom for returning the zoom the cluster expands on.
map.on('click', 'cluster', (e) => {
  var clusterId = e.features[0].properties.cluster_id;

  map.getSource('geojson').getClusterExpansionZoom(clusterId, (err, zoom) => {
    ...
  });
});

Misc. open questions:

  • Is it worth adding unit tests for this, and if so, how? The methods are just a thin layer between the GL JS API and the Supercluster library, and the functionality is tested there directly. We could add tests similar to those for GeoJSONSource#setData, but they don't actually test the roundtrip — they just mock the dispatcher send method, so for this case I doubt such tests will be useful.
  • Is the minimal flow typing in geojson_source_worker enough, or should we try doing something elaborate (e.g. setting up Supercluster/GeoJSON-VT typings, casting the index to either etc.)? Currently GeoJSONIndex interface in the master branch is empty and typing checks for it don't run at all for some reason — e.g. there's this._geoJSONIndex.getTile(...), this method isn't typed anywhere but Flow doesn't complain.
  • Is it worth packing the features through vt-pbf when sending them back? In most use cases, the payload will be tiny (a few point features), so getting maximum performance didn't seem necessary here, so for now I'm just sending GeoJSON directly. Should we leave it like this and get a round of feedback before trying to optimize?

Launch Checklist

  • briefly describe the changes in this PR
  • write tests for all new functionality
  • document any changes to public APIs
  • n/a post benchmark scores
  • manually test the debug page

@mourner mourner requested a review from jfirebaugh June 18, 2018 14:08
@mourner mourner mentioned this pull request Jun 18, 2018
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.

I think it's fine with manual test page and without vt-pbf packing for now, but the flow types could be more precise -- see inline comments.

* @param callback A callback to be called when the zoom value is retrieved (`(error, zoom) => { ... }`).
* @returns {GeoJSONSource} this
*/
getClusterExpansionZoom(clusterId: number, callback: Function) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Type the callbacks more precisely, e.g. Callback<number>.

* @param callback A callback to be called when the features are retrieved (`(error, features) => { ... }`).
* @returns {GeoJSONSource} this
*/
getClusterChildren(clusterId: number, callback: Function) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Callback<Array<GeoJSONFeature>>


// supercluster methods
getClusterExpansionZoom(clusterId: number): number;
getChildren(clusterId: number): GeoJSON[];
Copy link
Contributor

Choose a reason for hiding this comment

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

We're using the Array<GeoJSON> convention rather than GeoJSON[].

@@ -37,6 +37,12 @@ export type LoadGeoJSONParameters = {
export type LoadGeoJSON = (params: LoadGeoJSONParameters, callback: Callback<mixed>) => void;

export interface GeoJSONIndex {
getTile(z: number, x: number, y: number): GeoJSON;
Copy link
Contributor

Choose a reason for hiding this comment

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

Let's replace export type GeoJSON = Object above with types from flow-typed/geojson.js.

@mourner mourner merged commit bf88cd7 into master Jun 21, 2018
@mourner mourner deleted the supercluster-apis branch June 21, 2018 15:35
@lewis500
Copy link

Is there any good way to find the cluster_id associated with a given piece of data?

I have a map with clusters on it. When people search for something and select it, I want to highlight which cluster it's a part of.

@andrewharvey
Copy link
Collaborator

Is there any good way to find the cluster_id associated with a given piece of data?
I have a map with clusters on it. When people search for something and select it, I want to highlight which cluster it's a part of.

@lewis500 I guess you could use map.querySourceFeatures to get all the cluster_id's of visible clusters, then do getClusterLeaves on each to build an index of features => cluster_id's. Certainly not ideal and only works for clusters in the view, otherwise you'll probably need to use private undocumented methods and properties.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants