-
Notifications
You must be signed in to change notification settings - Fork 2.3k
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
Introduce clusterProperties option for aggregated cluster properties #7584
Changes from 6 commits
7624804
276bed8
6032e41
52fff4a
96969e0
41ad877
f7bf955
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -10,6 +10,7 @@ import Supercluster from 'supercluster'; | |
import geojsonvt from 'geojson-vt'; | ||
import assert from 'assert'; | ||
import VectorTileWorkerSource from './vector_tile_worker_source'; | ||
import { createExpression } from '../style-spec/expression'; | ||
|
||
import type { | ||
WorkerTileParameters, | ||
|
@@ -30,7 +31,8 @@ export type LoadGeoJSONParameters = { | |
source: string, | ||
cluster: boolean, | ||
superclusterOptions?: Object, | ||
geojsonVtOptions?: Object | ||
geojsonVtOptions?: Object, | ||
clusterProperties?: Object | ||
}; | ||
|
||
export type LoadGeoJSON = (params: LoadGeoJSONParameters, callback: ResponseCallback<Object>) => void; | ||
|
@@ -171,7 +173,7 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource { | |
|
||
try { | ||
this._geoJSONIndex = params.cluster ? | ||
new Supercluster(params.superclusterOptions).load(data.features) : | ||
new Supercluster(getSuperclusterOptions(params)).load(data.features) : | ||
geojsonvt(data, params.geojsonVtOptions); | ||
} catch (err) { | ||
return callback(err); | ||
|
@@ -293,4 +295,57 @@ class GeoJSONWorkerSource extends VectorTileWorkerSource { | |
} | ||
} | ||
|
||
function getSuperclusterOptions({superclusterOptions, clusterProperties}) { | ||
if (!clusterProperties || !superclusterOptions) return superclusterOptions; | ||
|
||
const initialValues = {}; | ||
const mapExpressions = {}; | ||
const reduceExpressions = {}; | ||
const globals = {accumulated: null, zoom: 0}; | ||
const feature = {properties: null}; | ||
const propertyNames = Object.keys(clusterProperties); | ||
|
||
for (const key of propertyNames) { | ||
const [operator, initialExpression, mapExpression] = clusterProperties[key]; | ||
|
||
const initialExpressionParsed = createExpression(initialExpression); | ||
const mapExpressionParsed = createExpression(mapExpression); | ||
const reduceExpressionParsed = createExpression( | ||
typeof operator === 'string' ? [operator, ['accumulated'], ['get', key]] : operator); | ||
|
||
assert(initialExpressionParsed.result === 'success'); | ||
assert(mapExpressionParsed.result === 'success'); | ||
assert(reduceExpressionParsed.result === 'success'); | ||
|
||
initialValues[key] = (initialExpressionParsed.value: any).evaluate(globals); | ||
mapExpressions[key] = mapExpressionParsed.value; | ||
reduceExpressions[key] = reduceExpressionParsed.value; | ||
} | ||
|
||
superclusterOptions.initial = () => { | ||
const properties = {}; | ||
for (const key of propertyNames) { | ||
properties[key] = initialValues[key]; | ||
} | ||
return properties; | ||
}; | ||
superclusterOptions.map = (pointProperties) => { | ||
feature.properties = pointProperties; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Why is this (and in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The map function and expression evaluation is synchronous, meaning there are no side effects from using mutable state here (which is reset at the beginning). At the same time, we avoid constantly creating new feature objects, improving performance and lots of garbage collection passes. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah, OK that makes sense. I thought it might have something to do with performance optimization. |
||
const properties = {}; | ||
for (const key of propertyNames) { | ||
properties[key] = mapExpressions[key].evaluate(globals, feature); | ||
} | ||
return properties; | ||
}; | ||
superclusterOptions.reduce = (accumulated, clusterProperties) => { | ||
feature.properties = clusterProperties; | ||
for (const key of propertyNames) { | ||
globals.accumulated = accumulated[key]; | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this mean a reducer could access the
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Currently not, but I think this could be added (check if There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah ok, I see - the reduce expression has to be something that fits into 👍 |
||
accumulated[key] = reduceExpressions[key].evaluate(globals, feature); | ||
} | ||
}; | ||
|
||
return superclusterOptions; | ||
} | ||
|
||
export default GeoJSONWorkerSource; |
Original file line number | Diff line number | Diff line change | ||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -373,6 +373,10 @@ | |||||||||||||||||||
"type": "number", | ||||||||||||||||||||
"doc": "Max zoom on which to cluster points if clustering is enabled. Defaults to one zoom less than maxzoom (so that last zoom features are not clustered)." | ||||||||||||||||||||
}, | ||||||||||||||||||||
"clusterProperties": { | ||||||||||||||||||||
"type": "*", | ||||||||||||||||||||
"doc": "An object defining custom properties on the generated clusters if clustering is enabled, aggregating values from clustered points. Has the form `{\"property_name\": [operator, initial_expression, map_expression]}`. `operator` is any expression function that accepts at least 2 operands (e.g. `\"+\"` or `\"max\"`) — it accumulates the property value from clusters/points the cluster contains; `initial_expression` evaluates the initial value of the property before accummulating other points/clusters; `map_expression` produces the value of a single point.\n\nExample: `{\"sum\": [\"+\", 0, [\"get\", \"scalerank\"]]}`.\n\nFor more advanced use cases, in place of `operator`, you can use a custom reduce expression that references a special `[\"accumulated\"]` value, e.g.:\n`{\"sum\": [[\"+\", [\"accumulated\"], [\"get\", \"sum\"]], 0, [\"get\", \"scalerank\"]]}`" | ||||||||||||||||||||
}, | ||||||||||||||||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Do you think it would be useful to add an expression support block for each of the expressions - initial,map, reduce ?Something similar to what is in the the layer properties. mapbox-gl-js/src/style-spec/reference/v8.json Lines 3500 to 3508 in 1208cfc
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not sure how to separate those into separate properties. There are two expressions (map, initial) and an operator that accepts them as parameters. Map expression would be just Given the non-standard syntax and that support block is dictated more by property aggregation semantics rather than technical limitations (like in case of many properties), perhaps we should leave that to validation, and maybe add a note clarifying this in the doc string. |
||||||||||||||||||||
"lineMetrics": { | ||||||||||||||||||||
"type": "boolean", | ||||||||||||||||||||
"default": false, | ||||||||||||||||||||
|
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.
Should this still be deleted, given that it is now usable as part of a custom reduce expression?
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.
Right, just readded it.