From 47925a62ff1fd3d7e666afeaf54e513e49713129 Mon Sep 17 00:00:00 2001 From: Vladimir Agafonkin Date: Thu, 10 Jan 2019 11:38:45 +0200 Subject: [PATCH 1/3] Introduce clusterProperties option for aggregated cluster properties (#7584) * introduce clusterProperties option for aggregating cluster properties * fix flow; more robust error handling * fix unit tests * add docs for clusterProperties * add a render test for clusterProperties * move cluster properties validation logic to style-spec; allow custom reduce fn * expose accumulated expression in the docs --- debug/cluster.html | 13 ++- src/source/geojson_source.js | 3 +- src/source/geojson_worker_source.js | 59 ++++++++++++- .../expression/definitions/index.js | 5 ++ src/style-spec/expression/index.js | 15 ++-- src/style-spec/expression/parsing_context.js | 2 +- src/style-spec/reference/v8.json | 13 +++ src/style-spec/types.js | 1 + .../validate/validate_expression.js | 19 ++++- src/style-spec/validate/validate_source.js | 35 ++++++-- .../geojson/clustered-properties/expected.png | Bin 0 -> 6239 bytes .../geojson/clustered-properties/style.json | 78 ++++++++++++++++++ .../style-spec/fixture/sources.input.json | 10 +++ .../style-spec/fixture/sources.output.json | 14 +++- 14 files changed, 241 insertions(+), 26 deletions(-) create mode 100644 test/integration/render-tests/geojson/clustered-properties/expected.png create mode 100644 test/integration/render-tests/geojson/clustered-properties/style.json diff --git a/debug/cluster.html b/debug/cluster.html index 019160c9409..612c0b5b0f1 100644 --- a/debug/cluster.html +++ b/debug/cluster.html @@ -20,7 +20,7 @@ var map = window.map = new mapboxgl.Map({ container: 'map', - zoom: 0, + zoom: 1, center: [0, 0], style: 'mapbox://styles/mapbox/cjf4m44iw0uza2spb3q0a7s41', hash: true @@ -31,7 +31,12 @@ "type": "geojson", "data": "/test/integration/data/places.geojson", "cluster": true, - "clusterRadius": 50 + "clusterRadius": 50, + "clusterProperties": { + "max": ["max", 0, ["get", "scalerank"]], + "sum": ["+", 0, ["get", "scalerank"]], + "has_island": ["any", false, ["==", ["get", "featureclass"], "island"]] + } }); map.addLayer({ "id": "cluster", @@ -39,7 +44,7 @@ "source": "geojson", "filter": ["==", "cluster", true], "paint": { - "circle-color": "rgba(0, 200, 0, 1)", + "circle-color": ["case", ["get", "has_island"], "orange", "rgba(0, 200, 0, 1)"], "circle-radius": 20 } }); @@ -49,7 +54,7 @@ "source": "geojson", "filter": ["==", "cluster", true], "layout": { - "text-field": "{point_count}", + "text-field": "{point_count} ({max})", "text-font": ["Open Sans Semibold", "Arial Unicode MS Bold"], "text-size": 12, "text-allow-overlap": true, diff --git a/src/source/geojson_source.js b/src/source/geojson_source.js index c82d2589467..cfcf397ae8c 100644 --- a/src/source/geojson_source.js +++ b/src/source/geojson_source.js @@ -139,7 +139,8 @@ class GeoJSONSource extends Evented implements Source { extent: EXTENT, radius: (options.clusterRadius || 50) * scale, log: false - } + }, + clusterProperties: options.clusterProperties }, options.workerOptions); } diff --git a/src/source/geojson_worker_source.js b/src/source/geojson_worker_source.js index d8219e90880..cb569e8650a 100644 --- a/src/source/geojson_worker_source.js +++ b/src/source/geojson_worker_source.js @@ -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) => 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; + 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]; + accumulated[key] = reduceExpressions[key].evaluate(globals, feature); + } + }; + + return superclusterOptions; +} + export default GeoJSONWorkerSource; diff --git a/src/style-spec/expression/definitions/index.js b/src/style-spec/expression/definitions/index.js index 1e37a007fb6..af32c11d37f 100644 --- a/src/style-spec/expression/definitions/index.js +++ b/src/style-spec/expression/definitions/index.js @@ -201,6 +201,11 @@ CompoundExpression.register(expressions, { [], (ctx) => ctx.globals.lineProgress || 0 ], + 'accumulated': [ + ValueType, + [], + (ctx) => ctx.globals.accumulated === undefined ? null : ctx.globals.accumulated + ], '+': [ NumberType, varargs(NumberType), diff --git a/src/style-spec/expression/index.js b/src/style-spec/expression/index.js index 7a92e3d2423..6ba02a0f6cf 100644 --- a/src/style-spec/expression/index.js +++ b/src/style-spec/expression/index.js @@ -38,7 +38,8 @@ export type GlobalProperties = $ReadOnly<{ zoom: number, heatmapDensity?: number, lineProgress?: number, - isSupportedScript?: (string) => boolean + isSupportedScript?: (string) => boolean, + accumulated?: Value }>; export class StyleExpression { @@ -49,12 +50,12 @@ export class StyleExpression { _warningHistory: {[key: string]: boolean}; _enumValues: ?{[string]: any}; - constructor(expression: Expression, propertySpec: StylePropertySpecification) { + constructor(expression: Expression, propertySpec: ?StylePropertySpecification) { this.expression = expression; this._warningHistory = {}; this._evaluator = new EvaluationContext(); - this._defaultValue = getDefaultValue(propertySpec); - this._enumValues = propertySpec.type === 'enum' ? propertySpec.values : null; + this._defaultValue = propertySpec ? getDefaultValue(propertySpec) : null; + this._enumValues = propertySpec && propertySpec.type === 'enum' ? propertySpec.values : null; } evaluateWithoutErrorHandling(globals: GlobalProperties, feature?: Feature, featureState?: FeatureState): any { @@ -105,12 +106,12 @@ export function isExpression(expression: mixed) { * * @private */ -export function createExpression(expression: mixed, propertySpec: StylePropertySpecification): Result> { - const parser = new ParsingContext(definitions, [], getExpectedType(propertySpec)); +export function createExpression(expression: mixed, propertySpec: ?StylePropertySpecification): Result> { + const parser = new ParsingContext(definitions, [], propertySpec ? getExpectedType(propertySpec) : undefined); // For string-valued properties, coerce to string at the top level rather than asserting. const parsed = parser.parse(expression, undefined, undefined, undefined, - propertySpec.type === 'string' ? {typeAnnotation: 'coerce'} : undefined); + propertySpec && propertySpec.type === 'string' ? {typeAnnotation: 'coerce'} : undefined); if (!parsed) { assert(parser.errors.length > 0); diff --git a/src/style-spec/expression/parsing_context.js b/src/style-spec/expression/parsing_context.js index a92e5dd53ba..a525cc08469 100644 --- a/src/style-spec/expression/parsing_context.js +++ b/src/style-spec/expression/parsing_context.js @@ -226,5 +226,5 @@ function isConstant(expression: Expression) { } return isFeatureConstant(expression) && - isGlobalPropertyConstant(expression, ['zoom', 'heatmap-density', 'line-progress', 'is-supported-script']); + isGlobalPropertyConstant(expression, ['zoom', 'heatmap-density', 'line-progress', 'accumulated', 'is-supported-script']); } diff --git a/src/style-spec/reference/v8.json b/src/style-spec/reference/v8.json index dc1127d1cdb..174dfbbd51b 100644 --- a/src/style-spec/reference/v8.json +++ b/src/style-spec/reference/v8.json @@ -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\"]]}`" + }, "lineMetrics": { "type": "boolean", "default": false, @@ -2803,6 +2807,15 @@ } } }, + "accumulated": { + "doc": "Gets the value of a cluster property accumulated so far. Can only be used in the `clusterProperties` option of a clustered GeoJSON source.", + "group": "Feature data", + "sdk-support": { + "basic functionality": { + "js": "0.53.0" + } + } + }, "+": { "doc": "Returns the sum of the inputs.", "group": "Math", diff --git a/src/style-spec/types.js b/src/style-spec/types.js index 100fddeae67..4b0ddc77395 100644 --- a/src/style-spec/types.js +++ b/src/style-spec/types.js @@ -121,6 +121,7 @@ export type GeoJSONSourceSpecification = {| "cluster"?: boolean, "clusterRadius"?: number, "clusterMaxZoom"?: number, + "clusterProperties"?: mixed, "lineMetrics"?: boolean, "generateId"?: boolean |} diff --git a/src/style-spec/validate/validate_expression.js b/src/style-spec/validate/validate_expression.js index eea82f9e7a9..d30e85a4ba6 100644 --- a/src/style-spec/validate/validate_expression.js +++ b/src/style-spec/validate/validate_expression.js @@ -4,7 +4,7 @@ import ValidationError from '../error/validation_error'; import { createExpression, createPropertyExpression } from '../expression'; import { deepUnbundle } from '../util/unbundle_jsonlint'; -import { isStateConstant } from '../expression/is_constant'; +import { isStateConstant, isGlobalPropertyConstant, isFeatureConstant } from '../expression/is_constant'; export default function validateExpression(options: any): Array { const expression = (options.expressionContext === 'property' ? createPropertyExpression : createExpression)(deepUnbundle(options.value), options.valueSpec); @@ -14,19 +14,30 @@ export default function validateExpression(options: any): Array }); } + const expressionObj = (expression.value: any).expression || (expression.value: any)._styleExpression.expression; + if (options.expressionContext === 'property' && (options.propertyKey === 'text-font') && - (expression.value: any)._styleExpression.expression.possibleOutputs().indexOf(undefined) !== -1) { + expressionObj.possibleOutputs().indexOf(undefined) !== -1) { return [new ValidationError(options.key, options.value, `Invalid data expression for "${options.propertyKey}". Output values must be contained as literals within the expression.`)]; } if (options.expressionContext === 'property' && options.propertyType === 'layout' && - (!isStateConstant((expression.value: any)._styleExpression.expression))) { + (!isStateConstant(expressionObj))) { return [new ValidationError(options.key, options.value, '"feature-state" data expressions are not supported with layout properties.')]; } - if (options.expressionContext === 'filter' && !isStateConstant((expression.value: any).expression)) { + if (options.expressionContext === 'filter' && !isStateConstant(expressionObj)) { return [new ValidationError(options.key, options.value, '"feature-state" data expressions are not supported with filters.')]; } + if (options.expressionContext && options.expressionContext.indexOf('cluster') === 0) { + if (!isGlobalPropertyConstant(expressionObj, ['zoom', 'feature-state'])) { + return [new ValidationError(options.key, options.value, '"zoom" and "feature-state" expressions are not supported with cluster properties.')]; + } + if (options.expressionContext === 'cluster-initial' && !isFeatureConstant(expressionObj)) { + return [new ValidationError(options.key, options.value, 'Feature data expressions are not supported with initial expression part of cluster properties.')]; + } + } + return []; } diff --git a/src/style-spec/validate/validate_source.js b/src/style-spec/validate/validate_source.js index f1f13a174d1..5499a4af546 100644 --- a/src/style-spec/validate/validate_source.js +++ b/src/style-spec/validate/validate_source.js @@ -3,6 +3,7 @@ import ValidationError from '../error/validation_error'; import { unbundle } from '../util/unbundle_jsonlint'; import validateObject from './validate_object'; import validateEnum from './validate_enum'; +import validateExpression from './validate_expression'; export default function validateSource(options) { const value = options.value; @@ -15,19 +16,19 @@ export default function validateSource(options) { } const type = unbundle(value.type); - let errors = []; + let errors; switch (type) { case 'vector': case 'raster': case 'raster-dem': - errors = errors.concat(validateObject({ + errors = validateObject({ key, value, valueSpec: styleSpec[`source_${type.replace('-', '_')}`], style: options.style, styleSpec - })); + }); if ('url' in value) { for (const prop in value) { if (['type', 'url', 'tileSize'].indexOf(prop) < 0) { @@ -38,13 +39,36 @@ export default function validateSource(options) { return errors; case 'geojson': - return validateObject({ + errors = validateObject({ key, value, valueSpec: styleSpec.source_geojson, style, styleSpec }); + if (value.cluster) { + for (const prop in value.clusterProperties) { + const [operator, initialExpr, mapExpr] = value.clusterProperties[prop]; + const reduceExpr = typeof operator === 'string' ? [operator, ['accumulated'], ['get', prop]] : operator; + + errors.push(...validateExpression({ + key: `${key}.${prop}.initial`, + value: initialExpr, + expressionContext: 'cluster-initial' + })); + errors.push(...validateExpression({ + key: `${key}.${prop}.map`, + value: mapExpr, + expressionContext: 'cluster-map' + })); + errors.push(...validateExpression({ + key: `${key}.${prop}.reduce`, + value: reduceExpr, + expressionContext: 'cluster-reduce' + })); + } + } + return errors; case 'video': return validateObject({ @@ -65,8 +89,7 @@ export default function validateSource(options) { }); case 'canvas': - errors.push(new ValidationError(key, null, `Please use runtime APIs to add canvas sources, rather than including them in stylesheets.`, 'source.canvas')); - return errors; + return [new ValidationError(key, null, `Please use runtime APIs to add canvas sources, rather than including them in stylesheets.`, 'source.canvas')]; default: return validateEnum({ diff --git a/test/integration/render-tests/geojson/clustered-properties/expected.png b/test/integration/render-tests/geojson/clustered-properties/expected.png new file mode 100644 index 0000000000000000000000000000000000000000..a1fb2060a2c76af03ccf0226b5daa8d2c8f28c73 GIT binary patch literal 6239 zcmX9@XH*kR(^Ukdmq_SMdJDaaC>TIOhtP>s1?gSs5=2TO2pDP*2vR~15~?UQ)Bur= zf)s&>(v&JFAJ6;!*|T$I&g_}HyR$QQQ_M{b80om_u3WjoXm}rNdF9I0%juP8G}M>l zH+;)~3JOE84)p2OpWLyZu3X{d{7>5MrbW+$#j7uGo<)7-dSP@|5V&iuz|5xih+0{+ zQPo(FRqOt5%%>^XEWS7HdT9Op<7}Xv*s^RyqJww-Y;ekx=NxrH%Rs@|DppFyvhDg1@>8m`I#T1I&1t|cSM>w$3f53T1-kN6sSj|# z=UuVGI+{&vruW|KSZ^5e9rzk$3dk|Nt#ycQ@+A~x-2`V6G7Wiw#=$2EVWHhdO*!23lpVu4g3155E4IowZ&z0<8U+?NXSeqGe$0`2YQHV0 zM)bq{{pR?pKZ{(S8x3x5C()LlOI9i4o zaLN{AWWb6{RSLBjv}t|7($rGi+Xq2xxrTrav>iH`_l78;r76{rz?K*YM}}zf|kr6RUUU zGGkVI8|f?p%hJmGp7#5eGcPNgvZFnkeem%$TFH~Bs%av-jD7d>&uteG<<2_m@!Sz! z+8k!9Z-r0iPgUF9`R{ZVZ8>Cim5D3*=N?aQwg%JDXHRsF>Yp8~q)8jX^;dbA29)TA zvLI9Q&LKiGH;WOVYCY-{;8 z?9136Zq+FOTz?5hOy4avd$Jql&+6RqQc0Rq4Ov%6hGH`EHJ4!hc|f%Cpnf6bKAiZx z#(fW|n;G(ULo4}u11A?l&nek2OYelAsmq!>iXuxnoORd_7TVyZGLVD&c*J(jdbHB@hyRNhT2Rj=vEn|;b_@g(354IphI|n~88I1U;^|=b> zgrm~cyJ-vK?EPNPbhDIX3utu$62&x~CYtDdEt8^fGh#{&3}n52&nt8o}O7e(^-^eNfd z68mlq>h1pMpm$c!GtwSDNpGloF@BLvW#lJIM>p>8uo|Wn|J9Z8h0h*fXJCGCzHz#; z-{YJuU9ciTBL-L$M+)*F1~uDcqORZcOhmGuk&5gNo;ZFWrNb z|Mg}c_LU5}(mrz08k=g8sLmZdbhWS<2xj!3rfd8qNL^0_ntey#%j8B<6tbybE3`h3 zQMrD+mRMG^c(+Ay>4D+#Sg5QkF|s{7V}#Wr6G!@mpPHU5MG&8pA0~R@vs-zmpK{E) z(K2;w@+Nz}5=;`8?-MJPha`AM1JhR-JbYoD_*BDABT>!#UgI1En@+yV9du2cJtVo^ z(2sJqR;f6D#(cQ8@GkF6swl57Lai~1c_;48ey_bnf5bt#qYVq$X}7AVMOy zIwyN@h5gG*dJ6=Pd168X4fBe$Izh}w;o2g}D&d-noTvK6ypH`T;GEXoxBJ=cT=@kX z*gu7r4By9J{o{@ZXT5g3GSPXT!Zud-H>Q8(oi{KaodCBii)NBr;Vkf-aN*TP zQl@09L!<|Xiv(Pes=>n+NPb;_FTz;@JPijhoyO$gjj7eU;e8H7Hz6ZV6S(QEIY6UaIUD5NE zpY)Hu_wwKND?k@XIX&?)rG7X#*Z%C813C>-;9>CsnVnK+YaFcUW6I}GSW)NIC;*?| zs_Rik_LXo~>q+|eAGUIT)SqgqMA*DSIXm|&sU|auCfMEtyJUe?M?2)%o)0562H@$gHhOPru8y1v3ldSZt9{&E;GiQk z?lv~R@D=ZheneZQxWk*Y@{}gPUGz;q;Ai~d(}wwP&~+rDvV5$YQV} z!5a^o0G_R?MpHTWgsIbbuIioB?YeT+_2=wcpEGil)R#ZhssVc76XeG#)%$tio$FT6 z`X!fskxh5N2tJ3bf$}%6fg31T8in?&nWrwbG6lHvMY_m|3ZpnDm z<$A)>?(?8&@cy)e4yN$=Z>S_rE!*42q7Z#AE9Bfnp=$3sDd(HxDvd#v<*3wRDIWIB zY$3Kg%)wCqR{lwp$_@Wx;c;mW-eM5AzB0`Gq^Uo3-a`piLoM8dmk`&~O{7X)1Ur38 zfD4DR{V3deq){tK7F<>KMEL8c%m7SiiWEs>9@z?jNj!?|m!dP=>F2rM`T$Zh!3v(O z4o$SlBV=lIZywxf{b1hso3nafJrzcDmgD>}LgbWmT)D{i} z_y%TtfZtrOEubp}?Cr8pLVt8kkRk};J7F{dn;a<|KyU}+8)v0Oz z2(i|%$UuM8uS_nzT4>MHAIyE`j)y`)x#dgXUYSg{>|W6~v;3H^)yg)~^bV6&f3Syq z$a^!*$^0zQI*FTa7xdXiRanGRhjB&4w}S0tBMlGK`IVT;M#Mmr(NDZvYO8%j4?xo5roNG4J+5!6=l2Dfm2 z9Y`DXgQl}1k&yq?r1uY+`|rvS zdY>|k$j@yo^|CP-j2-343jgP~W&<_HENHNsZ3{!nUwYnUq9Ys zVPE6mmgy_7O9{B?mAA6>_R>}M^n2Tej_mudy1(LhuZ~RECtPokBe=b$4m$z;S%Ff5 zZq)wQt=+c?fg1#(vj^Q|ViEO%#dI8Ne&^gdZmlL;3VaXMokpm}4gWvr!=2ar)}Szo zG=dxBk^U2{7cyy|?5J&p7gRM`{)Y<*?fQmQ?=$8WQ}<1BU3V}N%v|u9vE#F9w=x#$ zo#cTB2XdIc6E>a9&#pR1yA0-Dql3^%CRv$J?*NE}8WBHnb+bg-#Qe zAzCS=_!h512&~M~QEBpnBEh|ed_||>p>?jPXQuZP&bl#EF)!@JC+8X6ZIGr>*~M$f zU*~r*kU-E;!);8|H>XFrdf|ZX=G-;$R^79_YTZQWR$Nq%!Md@5TYFt=?BZ1u6SCAk(teL9W45ULR>Ecxw4&<^XX>zp%W5sXv}N*yvPh z$rh;8A+y=mif%pKFg{UmnU~|cM`IEj_R^}Hb&g=k|Ah#9lQTwqkg2*M?P$LG*9YgHYol#q<$ZH*gMWwLHcSHP855?wLk!i70 z93t90@c9fok)03IQ(%d`2&#*K|6O+stU=uN$;@HPC{`I&u?yun!mrJ&QULug7Yqn- zN(k_|qhY_u*le0X<1pzfzMKTtX|_Ii(Wf$Og1UrhoQz$FT5t@lhZI3Y;LY+zo+Jq_ z&k~B5S6;qlXiut-E#0BLpe%?V(f0|R{&$*SCgmwzJ;A{oiKk$KaIo00VVQ$>8ms{_ zu@x<4zA@cOW9xdyw~G$_Qri5*51fO$G>%tToA!$MKVzO%LBj<|O_e zMR_yZn_vkLt%P`T`W+Y1e^m7Q*J^qNd3{4-NCK?%L2BCRR?48^%UJZJ)TZO(I~2Oo zxMbW-(OdQ!;z=Q(jNu}$+Tw_$6#(OmyQ=E4{5dyI;GMc&{*zZGV@*Sv(O1(~JSZJ) z>ml(*Y$fW`YS-l4pUs54?ORo!p0+$aXfF@dcZNQe(U~ab`Lpcefg7C()6dKPz5}C@ zEgkH`^7pqbjHTSr=h50{YJuQ85kEyaANx=Q=f)?`?p=qvC~UZb9uQ!aJ@c*Ck5|0T z83I=Pw36FSM3_iT?$P$~BN6}wx*JT5R}%1@31VpfZ+*9nGrb+xYWp`;))NMoEo8b_ z&9gXjU4f~W4;U*Vy(y(=aflzJ23wxDS+~J z3;}~Il6WD00E=oJEVa*my}H0XmCAd3kPNQ}p{0-S?6Y6MVV6{gGuj&s{hXp*?><-j$&4)p ze3Sk|X=!q-%yjsR{M5wDd-g#nQ^LIMv9=Jorj+K-w|~8?mVV+s&yNn1As5(W5jyDL zDTYaG>P!vWtTe}ip~okA;SWur9)QFj!VWGlmFn3nL~~_%TE3Dg%+v z{_+w`RIG5jjkew)<{pD>#h?dUvAVTDcaE|vk@}KcxF{Ht6L1^T{j*Ehx&3~(&>Hzq z;mIB190YFiCN2SYXMJO!?KH?g7R|!hRCIYh#nI~vDIaacr^CgfCZ59NjU5r*l-Hn< z_gEopm_F8gg)p%@7fQ3};c+>a?{syY1)P3t!WXe>KB_FG#2TjoB?PC{SZ*-k^*cvq zVyebD&YV-@FD zvTK|3P(R2I2-t-A`tF!GR*L$I!d>F>NX6dfX*0P~%6sc%{@T=x!bt)WRAH4OmWW;~`&H57Gxyb( zV>wdC_LfkjvAk)SXgGy^A{MN4^qoj{KiOe`yhAI!7CtJG9cbCXtX?6!{#~)@YmsAV zs)hU$x%8la(|X=nmzK^fLdo+|($l!@Ww3_q1>Y0x zAr8}WPYx4%&~&WS7erJHZ1o0pifJFqPlSJEZ*Vs~+@SV1M{lQ}ILbO1KC66_BkvR- za2x4(Y5ZUyW*GKJ&0-8} zwjznG9hL$gzRAK_8!kUvj35VGcF-_Qr5M@2ce;+pZ_-}!G_oJ_rhl}pTkgc>pA~to zdikFybsT$Ke0YyTe-GhO$$bM;26@IqtL}OLF`b=c6O%uyLmq&-j+})^ro}8C*IrK;AsuCs!N2CGH{CVn^t(zJtiO>JavOw_GUw8BTjgUnR_`6*1PK6y zdo}o9L4XZNA;tah-{Q!lXXHsZ*fDv=AkCDJCwcj1>9D4LoYJ$6W8g_Q46tZhq z+elec!iCr?IMWKKqCT^dbTx|THuI!WQbOIPf~H|jWS*1ff?g?={7T=VL1@np&?*|L zTX|nV)X)1;D#39FY Date: Thu, 17 Jan 2019 20:39:01 +0200 Subject: [PATCH 2/3] Simplify clusterProperties expression (#7784) * simplify clusterProperties expression * update clusterProperties docs * fix render test * fix validation test --- debug/cluster.html | 6 +++--- package.json | 2 +- src/source/geojson_worker_source.js | 13 +------------ src/style-spec/reference/v8.json | 2 +- src/style-spec/validate/validate_source.js | 7 +------ .../geojson/clustered-properties/style.json | 6 +++--- test/unit/style-spec/fixture/sources.input.json | 5 ++--- test/unit/style-spec/fixture/sources.output.json | 8 ++------ yarn.lock | 8 ++++---- 9 files changed, 18 insertions(+), 39 deletions(-) diff --git a/debug/cluster.html b/debug/cluster.html index 612c0b5b0f1..919c8200d6a 100644 --- a/debug/cluster.html +++ b/debug/cluster.html @@ -33,9 +33,9 @@ "cluster": true, "clusterRadius": 50, "clusterProperties": { - "max": ["max", 0, ["get", "scalerank"]], - "sum": ["+", 0, ["get", "scalerank"]], - "has_island": ["any", false, ["==", ["get", "featureclass"], "island"]] + "max": ["max", ["get", "scalerank"]], + "sum": ["+", ["get", "scalerank"]], + "has_island": ["any", ["==", ["get", "featureclass"], "island"]] } }); map.addLayer({ diff --git a/package.json b/package.json index 36f4d69cebf..883e6fbe7d4 100644 --- a/package.json +++ b/package.json @@ -34,7 +34,7 @@ "potpack": "^1.0.1", "quickselect": "^1.0.0", "rw": "^1.3.3", - "supercluster": "^5.0.0", + "supercluster": "^6.0.0", "tinyqueue": "^1.1.0", "vt-pbf": "^3.0.1" }, diff --git a/src/source/geojson_worker_source.js b/src/source/geojson_worker_source.js index cb569e8650a..d110840474e 100644 --- a/src/source/geojson_worker_source.js +++ b/src/source/geojson_worker_source.js @@ -298,7 +298,6 @@ 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}; @@ -306,29 +305,19 @@ function getSuperclusterOptions({superclusterOptions, clusterProperties}) { const propertyNames = Object.keys(clusterProperties); for (const key of propertyNames) { - const [operator, initialExpression, mapExpression] = clusterProperties[key]; + const [operator, 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; const properties = {}; diff --git a/src/style-spec/reference/v8.json b/src/style-spec/reference/v8.json index 174dfbbd51b..e68b9dbdea1 100644 --- a/src/style-spec/reference/v8.json +++ b/src/style-spec/reference/v8.json @@ -375,7 +375,7 @@ }, "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\"]]}`" + "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, 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; `map_expression` produces the value of a single point.\n\nExample: `{\"sum\": [\"+\", [\"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\"]], [\"get\", \"scalerank\"]]}`" }, "lineMetrics": { "type": "boolean", diff --git a/src/style-spec/validate/validate_source.js b/src/style-spec/validate/validate_source.js index 5499a4af546..44ec5dceebc 100644 --- a/src/style-spec/validate/validate_source.js +++ b/src/style-spec/validate/validate_source.js @@ -48,14 +48,9 @@ export default function validateSource(options) { }); if (value.cluster) { for (const prop in value.clusterProperties) { - const [operator, initialExpr, mapExpr] = value.clusterProperties[prop]; + const [operator, mapExpr] = value.clusterProperties[prop]; const reduceExpr = typeof operator === 'string' ? [operator, ['accumulated'], ['get', prop]] : operator; - errors.push(...validateExpression({ - key: `${key}.${prop}.initial`, - value: initialExpr, - expressionContext: 'cluster-initial' - })); errors.push(...validateExpression({ key: `${key}.${prop}.map`, value: mapExpr, diff --git a/test/integration/render-tests/geojson/clustered-properties/style.json b/test/integration/render-tests/geojson/clustered-properties/style.json index 374ac0a3cb1..8dfbf751417 100644 --- a/test/integration/render-tests/geojson/clustered-properties/style.json +++ b/test/integration/render-tests/geojson/clustered-properties/style.json @@ -18,9 +18,9 @@ "cluster": true, "clusterRadius": 50, "clusterProperties": { - "max": ["max", 0, ["get", "scalerank"]], - "sum": ["+", 0, ["get", "scalerank"]], - "has_island": ["any", false, ["==", ["get", "featureclass"], "island"]] + "max": ["max", ["get", "scalerank"]], + "sum": ["+", ["get", "scalerank"]], + "has_island": ["any", ["==", ["get", "featureclass"], "island"]] } } }, diff --git a/test/unit/style-spec/fixture/sources.input.json b/test/unit/style-spec/fixture/sources.input.json index f5d051a4879..b57e40ce793 100644 --- a/test/unit/style-spec/fixture/sources.input.json +++ b/test/unit/style-spec/fixture/sources.input.json @@ -46,9 +46,8 @@ "data": "/test/integration/data/places.geojson", "cluster": true, "clusterProperties": { - "initial": ["+", ["get", "scalerank"], 0], - "zoom": ["+", 0, ["zoom"]], - "state": ["+", 0, ["feature-state", "foo"]] + "zoom": ["+", ["zoom"]], + "state": ["+", ["feature-state", "foo"]] } } }, diff --git a/test/unit/style-spec/fixture/sources.output.json b/test/unit/style-spec/fixture/sources.output.json index 66d03b3eba5..34674e6aa96 100644 --- a/test/unit/style-spec/fixture/sources.output.json +++ b/test/unit/style-spec/fixture/sources.output.json @@ -39,16 +39,12 @@ "message": "sources.canvas: Please use runtime APIs to add canvas sources, rather than including them in stylesheets.", "identifier": "source.canvas" }, - { - "message": "sources.cluster-properties.initial.initial: Feature data expressions are not supported with initial expression part of cluster properties.", - "line": 49 - }, { "message": "sources.cluster-properties.zoom.map: \"zoom\" and \"feature-state\" expressions are not supported with cluster properties.", - "line": 50 + "line": 49 }, { "message": "sources.cluster-properties.state.map: \"zoom\" and \"feature-state\" expressions are not supported with cluster properties.", - "line": 51 + "line": 50 } ] diff --git a/yarn.lock b/yarn.lock index ed2ae7afbf7..b933005b894 100644 --- a/yarn.lock +++ b/yarn.lock @@ -11535,10 +11535,10 @@ sugarss@^2.0.0: dependencies: postcss "^7.0.2" -supercluster@^5.0.0: - version "5.0.0" - resolved "https://registry.yarnpkg.com/supercluster/-/supercluster-5.0.0.tgz#2a5a9b1ffbd0d6180dea10039d78b5d95fdb8f27" - integrity sha512-9eeD5Q3908+tqdz+wYHHzi5mLKgnqtpO5mrjUfqr67UmGuOwBtVoQ9pJJrfcVHwMwC0wEBvfNRF9PgFOZgsOpw== +supercluster@^6.0.0: + version "6.0.0" + resolved "https://registry.yarnpkg.com/supercluster/-/supercluster-6.0.0.tgz#7058b45545f8bacafb917a66ff2d4d36f74fd2ba" + integrity sha512-Jw0KG1zheUvNWYyl1NPb3yUURq6j6Q8rm+maY18+DDrJKmhkz0oxXoWQLf2usXVFJ0urQ8h0gh24yYV8y74WEg== dependencies: kdbush "^3.0.0" From 4cc2a7d876db48e18b207a908f5cf7104adb56c3 Mon Sep 17 00:00:00 2001 From: Asheem Mamoowala Date: Wed, 9 Jan 2019 11:47:45 -0800 Subject: [PATCH 3/3] Update feedback url and related test --- src/ui/control/attribution_control.js | 2 +- src/util/config.js | 2 ++ test/unit/ui/control/attribution.test.js | 7 ++++--- 3 files changed, 7 insertions(+), 4 deletions(-) diff --git a/src/ui/control/attribution_control.js b/src/ui/control/attribution_control.js index aaf59ab99b0..6491a8bb76c 100644 --- a/src/ui/control/attribution_control.js +++ b/src/ui/control/attribution_control.js @@ -103,7 +103,7 @@ class AttributionControl { } return acc; }, `?`); - editLink.href = `https://www.mapbox.com/feedback/${paramString}${this._map._hash ? this._map._hash.getHashString(true) : ''}`; + editLink.href = `${config.FEEDBACK_URL}/${paramString}${this._map._hash ? this._map._hash.getHashString(true) : ''}`; } } diff --git a/src/util/config.js b/src/util/config.js index f2ef586a804..0d8ad3017f8 100644 --- a/src/util/config.js +++ b/src/util/config.js @@ -3,6 +3,7 @@ type Config = {| API_URL: string, EVENTS_URL: string, + FEEDBACK_URL: string, REQUIRE_ACCESS_TOKEN: boolean, ACCESS_TOKEN: ?string, MAX_PARALLEL_IMAGE_REQUESTS: number @@ -17,6 +18,7 @@ const config: Config = { return 'https://events.mapbox.com/events/v2'; } }, + FEEDBACK_URL: 'https://apps.mapbox.com/feedback', REQUIRE_ACCESS_TOKEN: true, ACCESS_TOKEN: null, MAX_PARALLEL_IMAGE_REQUESTS: 16 diff --git a/test/unit/ui/control/attribution.test.js b/test/unit/ui/control/attribution.test.js index b5c71bfbeb9..5316a11b650 100644 --- a/test/unit/ui/control/attribution.test.js +++ b/test/unit/ui/control/attribution.test.js @@ -109,17 +109,18 @@ test('AttributionControl dedupes attributions that are substrings of others', (t }); test('AttributionControl has the correct edit map link', (t) => { + config.FEEDBACK_URL = "https://feedback.com"; const map = createMap(t); const attribution = new AttributionControl(); map.addControl(attribution); map.on('load', () => { - map.addSource('1', { type: 'geojson', data: { type: 'FeatureCollection', features: [] }, attribution: 'Improve this map'}); + map.addSource('1', { type: 'geojson', data: { type: 'FeatureCollection', features: [] }, attribution: 'Improve this map'}); map.addLayer({ id: '1', type: 'fill', source: '1' }); map.on('data', (e) => { if (e.dataType === 'source' && e.sourceDataType === 'metadata') { - t.equal(attribution._editLink.href, 'https://www.mapbox.com/feedback/?owner=mapbox&id=streets-v10&access_token=pk.123#/0/0/0', 'edit link contains map location data'); + t.equal(attribution._editLink.href, 'https://feedback.com/?owner=mapbox&id=streets-v10&access_token=pk.123#/0/0/0', 'edit link contains map location data'); map.setZoom(2); - t.equal(attribution._editLink.href, 'https://www.mapbox.com/feedback/?owner=mapbox&id=streets-v10&access_token=pk.123#/0/0/2', 'edit link updates on mapmove'); + t.equal(attribution._editLink.href, 'https://feedback.com/?owner=mapbox&id=streets-v10&access_token=pk.123#/0/0/2', 'edit link updates on mapmove'); t.end(); } });