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

An option to use a feature property as ID for feature state #8987

Merged
merged 12 commits into from
Dec 2, 2019
3 changes: 2 additions & 1 deletion bench/lib/tile_parser.js
Original file line number Diff line number Diff line change
Expand Up @@ -129,7 +129,8 @@ export default class TileParser {
pitch: 0,
cameraToCenterDistance: 0,
cameraToTileDistance: 0,
returnDependencies
returnDependencies,
promoteId: undefined
});

const vectorTile = new VT.VectorTile(new Protobuf(tile.buffer));
Expand Down
2 changes: 2 additions & 0 deletions build/generate-flow-typed-style-spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -122,6 +122,8 @@ export type FormattedSpecification = string;

export type ResolvedImageSpecification = string;

export type PromoteIdSpecification = {[string]: string} | string;

export type FilterSpecification =
| ['has', string]
| ['!has', string]
Expand Down
73 changes: 73 additions & 0 deletions debug/featurestate.html
Original file line number Diff line number Diff line change
@@ -0,0 +1,73 @@
<!DOCTYPE html>
<html>
<head>
<title>Mapbox GL JS debug page</title>
<meta charset='utf-8'>
<meta name="viewport" content="width=device-width, initial-scale=1.0, user-scalable=no">
<link rel='stylesheet' href='../dist/mapbox-gl.css' />
<style>
body { margin: 0; padding: 0; }
html, body, #map { height: 100%; }
</style>
</head>

<body>
<div id='map'></div>

<script src='../dist/mapbox-gl-dev.js'></script>
<script src='../debug/access_token_generated.js'></script>
<script>

var map = window.map = new mapboxgl.Map({
container: 'map',
zoom: 4,
center: [-96, 38],
style: {version: 8, layers: [], sources: {}}
});

map.on('load', () => {

map.addSource('counties', {
"type": "vector",
"url": "mapbox://mapbox.82pkq93d",
"promoteId": {"original": "COUNTY"}
});

map.addLayer({
"id": "counties",
"type": "fill",
"source": "counties",
"source-layer": "original",
"paint": {
"fill-outline-color": "black",
"fill-color": ["case", ["boolean", ["feature-state", "hover"], false], "red", "lightgrey"]
}
});

let selectedCounty = null;

function resetFeatureState() {
if (selectedCounty) {
map.setFeatureState({source: 'counties', sourceLayer: 'original', id: selectedCounty}, {hover: false});
selectedCounty = null;
}
}

map.on("mouseleave", "counties", () => {
resetFeatureState();
});

map.on("mousemove", "counties", (e) => {
const feature = e.features[0];

if (selectedCounty !== feature.id) {
resetFeatureState();
map.setFeatureState({source: 'counties', sourceLayer: 'original', id: feature.id}, {hover: true});
selectedCounty = feature.id;
}
});
});

</script>
</body>
</html>
1 change: 1 addition & 0 deletions src/data/bucket.js
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ export type PopulateParameters = {

export type IndexedFeature = {
feature: VectorTileFeature,
id: number | string,
index: number,
sourceLayerIndex: number,
}
Expand Down
4 changes: 2 additions & 2 deletions src/data/bucket/circle_bucket.js
Original file line number Diff line number Diff line change
Expand Up @@ -85,15 +85,15 @@ class CircleBucket<Layer: CircleStyleLayer | HeatmapStyleLayer> implements Bucke
circleSortKey = ((styleLayer: any): CircleStyleLayer).layout.get('circle-sort-key');
}

for (const {feature, index, sourceLayerIndex} of features) {
for (const {feature, id, index, sourceLayerIndex} of features) {
if (this.layers[0]._featureFilter(new EvaluationParameters(this.zoom), feature)) {
const geometry = loadGeometry(feature);
const sortKey = circleSortKey ?
circleSortKey.evaluate(feature, {}) :
undefined;

const bucketFeature: BucketFeature = {
id: feature.id,
id,
properties: feature.properties,
type: feature.type,
sourceLayerIndex,
Expand Down
4 changes: 2 additions & 2 deletions src/data/bucket/fill_bucket.js
Original file line number Diff line number Diff line change
Expand Up @@ -78,7 +78,7 @@ class FillBucket implements Bucket {
const fillSortKey = this.layers[0].layout.get('fill-sort-key');
const bucketFeatures = [];

for (const {feature, index, sourceLayerIndex} of features) {
for (const {feature, id, index, sourceLayerIndex} of features) {
if (!this.layers[0]._featureFilter(new EvaluationParameters(this.zoom), feature)) continue;

const geometry = loadGeometry(feature);
Expand All @@ -87,7 +87,7 @@ class FillBucket implements Bucket {
undefined;

const bucketFeature: BucketFeature = {
id: feature.id,
id,
properties: feature.properties,
type: feature.type,
sourceLayerIndex,
Expand Down
3 changes: 2 additions & 1 deletion src/data/bucket/fill_extrusion_bucket.js
Original file line number Diff line number Diff line change
Expand Up @@ -91,12 +91,13 @@ class FillExtrusionBucket implements Bucket {
this.features = [];
this.hasPattern = hasPattern('fill-extrusion', this.layers, options);

for (const {feature, index, sourceLayerIndex} of features) {
for (const {feature, id, index, sourceLayerIndex} of features) {
if (!this.layers[0]._featureFilter(new EvaluationParameters(this.zoom), feature)) continue;

const geometry = loadGeometry(feature);

const patternFeature: BucketFeature = {
id,
sourceLayerIndex,
index,
geometry,
Expand Down
6 changes: 2 additions & 4 deletions src/data/bucket/symbol_bucket.js
Original file line number Diff line number Diff line change
Expand Up @@ -401,7 +401,7 @@ class SymbolBucket implements Bucket {
const availableImages = options.availableImages;
const globalProperties = new EvaluationParameters(this.zoom);

for (const {feature, index, sourceLayerIndex} of features) {
for (const {feature, id, index, sourceLayerIndex} of features) {
if (!layer._featureFilter(globalProperties, feature)) {
continue;
}
Expand Down Expand Up @@ -447,6 +447,7 @@ class SymbolBucket implements Bucket {
undefined;

const symbolFeature: SymbolFeature = {
id,
text,
icon,
index,
Expand All @@ -456,9 +457,6 @@ class SymbolBucket implements Bucket {
type: vectorTileFeatureTypes[feature.type],
sortKey
};
if (typeof feature.id !== 'undefined') {
symbolFeature.id = feature.id;
}
this.features.push(symbolFeature);

if (icon) {
Expand Down
36 changes: 24 additions & 12 deletions src/data/feature_index.js
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import {polygonIntersectsBox} from '../util/intersection_tests';
import type StyleLayer from '../style/style_layer';
import type {FeatureFilter} from '../style-spec/feature_filter';
import type Transform from '../geo/transform';
import type {FilterSpecification} from '../style-spec/types';
import type {FilterSpecification, PromoteIdSpecification} from '../style-spec/types';

import {FeatureIndexArray} from './array_types';

Expand All @@ -46,23 +46,23 @@ class FeatureIndex {
grid: Grid;
grid3D: Grid;
featureIndexArray: FeatureIndexArray;
promoteId: ?PromoteIdSpecification;

rawTileData: ArrayBuffer;
bucketLayerIDs: Array<Array<string>>;

vtLayers: {[string]: VectorTileLayer};
sourceLayerCoder: DictionaryCoder;

constructor(tileID: OverscaledTileID,
grid?: Grid,
featureIndexArray?: FeatureIndexArray) {
constructor(tileID: OverscaledTileID, promoteId?: ?PromoteIdSpecification) {
this.tileID = tileID;
this.x = tileID.canonical.x;
this.y = tileID.canonical.y;
this.z = tileID.canonical.z;
this.grid = grid || new Grid(EXTENT, 16, 0);
this.grid = new Grid(EXTENT, 16, 0);
this.grid3D = new Grid(EXTENT, 16, 0);
this.featureIndexArray = featureIndexArray || new FeatureIndexArray();
this.featureIndexArray = new FeatureIndexArray();
this.promoteId = promoteId;
}

insert(feature: VectorTileFeature, geometry: Array<Array<Point>>, featureIndex: number, sourceLayerIndex: number, bucketIndex: number, is3D?: boolean) {
Expand Down Expand Up @@ -146,14 +146,14 @@ class FeatureIndex {
filter,
params.layers,
styleLayers,
(feature: VectorTileFeature, styleLayer: StyleLayer) => {
(feature: VectorTileFeature, styleLayer: StyleLayer, id: string | number | void) => {
if (!featureGeometry) {
featureGeometry = loadGeometry(feature);
}
let featureState = {};
if (feature.id) {
if (id !== undefined) {
// `feature-state` expression evaluation requires feature state to be available
featureState = sourceFeatureState.getState(styleLayer.sourceLayer || '_geojsonTileLayer', feature.id);
featureState = sourceFeatureState.getState(styleLayer.sourceLayer || '_geojsonTileLayer', id);
}
return styleLayer.queryIntersectsFeature(queryGeometry, feature, featureState, featureGeometry, this.z, args.transform, pixelsToTileUnits, args.pixelPosMatrix);
}
Expand All @@ -171,7 +171,7 @@ class FeatureIndex {
filter: FeatureFilter,
filterLayerIDs: Array<string>,
styleLayers: {[string]: StyleLayer},
intersectionTest?: (feature: VectorTileFeature, styleLayer: StyleLayer) => boolean | number) {
intersectionTest?: (feature: VectorTileFeature, styleLayer: StyleLayer, id: string | number | void) => boolean | number) {

const layerIDs = this.bucketLayerIDs[bucketIndex];
if (filterLayerIDs && !arraysIntersect(filterLayerIDs, layerIDs))
Expand All @@ -184,6 +184,8 @@ class FeatureIndex {
if (!filter(new EvaluationParameters(this.tileID.overscaledZ), feature))
return;

const id = this.getId(feature, sourceLayerName);

for (let l = 0; l < layerIDs.length; l++) {
const layerID = layerIDs[l];

Expand All @@ -194,13 +196,13 @@ class FeatureIndex {
const styleLayer = styleLayers[layerID];
if (!styleLayer) continue;

const intersectionZ = !intersectionTest || intersectionTest(feature, styleLayer);
const intersectionZ = !intersectionTest || intersectionTest(feature, styleLayer, id);
if (!intersectionZ) {
// Only applied for non-symbol features
continue;
}

const geojsonFeature = new GeoJSONFeature(feature, this.z, this.x, this.y);
const geojsonFeature = new GeoJSONFeature(feature, this.z, this.x, this.y, id);
(geojsonFeature: any).layer = styleLayer.serialize();
let layerResult = result[layerID];
if (layerResult === undefined) {
Expand Down Expand Up @@ -247,6 +249,16 @@ class FeatureIndex {

return false;
}

getId(feature: VectorTileFeature, sourceLayerId: string): string | number | void {
let id = feature.id;
if (this.promoteId) {
const propName = typeof this.promoteId === 'string' ? this.promoteId : this.promoteId[sourceLayerId];
id = feature.properties[propName];
if (typeof id === 'boolean') id = Number(id);
}
return id;
}
}

register(
Expand Down
23 changes: 18 additions & 5 deletions src/data/feature_position_map.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// @flow

import murmur3 from 'murmurhash-js';
import {register} from '../util/web_worker_transfer';
import assert from 'assert';

Expand All @@ -26,28 +27,30 @@ export default class FeaturePositionMap {
this.indexed = false;
}

add(id: number, index: number, start: number, end: number) {
this.ids.push(id);
add(id: mixed, index: number, start: number, end: number) {
this.ids.push(getNumericId(id));
this.positions.push(index, start, end);
}

getPositions(id: number): Array<FeaturePosition> {
getPositions(id: mixed): Array<FeaturePosition> {
assert(this.indexed);

const intId = getNumericId(id);

// binary search for the first occurrence of id in this.ids;
// relies on ids/positions being sorted by id, which happens in serialization
let i = 0;
let j = this.ids.length - 1;
while (i < j) {
const m = (i + j) >> 1;
if (this.ids[m] >= id) {
if (this.ids[m] >= intId) {
j = m;
} else {
i = m + 1;
}
}
const positions = [];
while (this.ids[i] === id) {
while (this.ids[i] === intId) {
const index = this.positions[3 * i];
const start = this.positions[3 * i + 1];
const end = this.positions[3 * i + 2];
Expand Down Expand Up @@ -79,6 +82,16 @@ export default class FeaturePositionMap {
}
}

const MAX_SAFE_INTEGER = Math.pow(2, 53) - 1;

function getNumericId(value: mixed) {
const numValue = +value;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think this incorrectly handles strings that can be converted to javascript numbers but not javascript integers:

  • integers javascript can't safely represent '9007199254740993'
  • decimals '1.2'
  • 'Infinity'

It also looks like users of this function expect integers but this can return a float if given a float. Is that ok because it should never be given a float?

Copy link
Member Author

Choose a reason for hiding this comment

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

Initially this was hashing all non-integer ids, but then I changed it because I thought there's no longer a need to have it limited to integer — those get saved in a Float64Array anyway, which supports decimals, values like Infinity, and values bigger than MAX_SAFE_INTEGER.

Copy link
Member Author

Choose a reason for hiding this comment

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

It also looks like users of this function expect integers

Oh, actually, in addition to the above, this function being exported is a leftover from earlier — how things get stored in the position map should be an internal detail so there won't be "users" of this. I'll remove the "export".

Copy link
Contributor

Choose a reason for hiding this comment

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

those get saved in a Float64Array anyway, which supports decimals, values like Infinity, and values bigger than MAX_SAFE_INTEGER

Neither javascript numbers (which are 64 bit floats) or Float64Array can store integers > MAX_SAFE_INTEGER safely. Both '9007199254740993' and '9007199254740992' will look like the same id to this implementation. This might seem unlikely but we ran into this with iD editor because osm uses sequential 64 integers as ids.

Similar problems theoretically exist with decimals but seem less likely.

Copy link
Member Author

Choose a reason for hiding this comment

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

Fair enough — updated to hash as a string in case a numeric id is more than MAX_SAFE_INTEGER.

if (!isNaN(numValue) && numValue <= MAX_SAFE_INTEGER) {
return numValue;
}
return murmur3(String(value));
}

// custom quicksort that sorts ids, indices and offsets together (by ids)
function sort(ids, positions, left, right) {
if (left >= right) return;
Expand Down
4 changes: 2 additions & 2 deletions src/data/program_configuration.js
Original file line number Diff line number Diff line change
Expand Up @@ -624,7 +624,7 @@ export default class ProgramConfiguration {
updatePaintArrays(featureStates: FeatureStates, featureMap: FeaturePositionMap, vtLayer: VectorTileLayer, layer: TypedStyleLayer, imagePositions: {[string]: ImagePosition}): boolean {
let dirty: boolean = false;
for (const id in featureStates) {
const positions = featureMap.getPositions(+id);
const positions = featureMap.getPositions(id);

for (const pos of positions) {
const feature = vtLayer.feature(pos.index);
Expand Down Expand Up @@ -746,7 +746,7 @@ export class ProgramConfigurationSet<Layer: TypedStyleLayer> {
}

if (feature.id !== undefined) {
this._featureMap.add(+feature.id, index, this._bufferOffset, length);
this._featureMap.add(feature.id, index, this._bufferOffset, length);
}
this._bufferOffset = length;

Expand Down
7 changes: 5 additions & 2 deletions src/source/geojson_source.js
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ import type Tile from './tile';
import type Actor from '../util/actor';
import type {Callback} from '../types/callback';
import type {GeoJSON, GeoJSONFeature} from '@mapbox/geojson-types';
import type {GeoJSONSourceSpecification} from '../style-spec/types';
import type {GeoJSONSourceSpecification, PromoteIdSpecification} from '../style-spec/types';

/**
* A source containing GeoJSON.
Expand Down Expand Up @@ -69,6 +69,7 @@ class GeoJSONSource extends Evented implements Source {
maxzoom: number;
tileSize: number;
attribution: string;
promoteId: ?PromoteIdSpecification;

isTileClipped: boolean;
reparseOverscaled: boolean;
Expand Down Expand Up @@ -114,6 +115,7 @@ class GeoJSONSource extends Evented implements Source {
if (options.maxzoom !== undefined) this.maxzoom = options.maxzoom;
if (options.type) this.type = options.type;
if (options.attribution) this.attribution = options.attribution;
this.promoteId = options.promoteId;

const scale = EXTENT / this.tileSize;

Expand Down Expand Up @@ -295,7 +297,8 @@ class GeoJSONSource extends Evented implements Source {
tileSize: this.tileSize,
source: this.id,
pixelRatio: browser.devicePixelRatio,
showCollisionBoxes: this.map.showCollisionBoxes
showCollisionBoxes: this.map.showCollisionBoxes,
promoteId: this.promoteId
};

tile.request = this.actor.send(message, params, (err, data) => {
Expand Down
Loading