Skip to content
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
13 changes: 9 additions & 4 deletions src/data/feature_index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ import type {MapGeoJSONFeature} from '../util/vectortile_to_geojson';
import type {StyleLayer} from '../style/style_layer';
import type {FeatureFilter, FeatureState, FilterSpecification, PromoteIdSpecification} from '@maplibre/maplibre-gl-style-spec';
import type {IReadonlyTransform} from '../geo/transform_interface';
import {type VectorTileFeatureLike, type VectorTileLayerLike, GEOJSON_TILE_LAYER_NAME} from '@maplibre/vt-pbf';
import {type VectorTileFeatureLike, type VectorTileLayerLike, type Feature, GeoJSONWrapper, GEOJSON_TILE_LAYER_NAME} from '@maplibre/vt-pbf';
import {VectorTile} from '@mapbox/vector-tile';

export {GEOJSON_TILE_LAYER_NAME};
Expand Down Expand Up @@ -69,6 +69,7 @@ export class FeatureIndex {
promoteId?: PromoteIdSpecification;
encoding: string;
rawTileData: ArrayBuffer;
geoJsonFeatureData: Feature[];
bucketLayerIDs: Array<Array<string>>;

vtLayers: {[_: string]: VectorTileLayerLike};
Expand Down Expand Up @@ -114,9 +115,13 @@ export class FeatureIndex {

loadVTLayers(): {[_: string]: VectorTileLayerLike} {
if (!this.vtLayers) {
this.vtLayers = this.encoding !== 'mlt'
? new VectorTile(new Protobuf(this.rawTileData)).layers
: new MLTVectorTile(this.rawTileData).layers;
if (this.geoJsonFeatureData) {
this.vtLayers = new GeoJSONWrapper(this.geoJsonFeatureData, {version: 2, extent: EXTENT}).layers;
Copy link
Contributor

Choose a reason for hiding this comment

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

How long does constructing GeoJSONWrapper take for a source with 100k points? We used to do this step on the worker thread.

Copy link
Collaborator

@HarelM HarelM Dec 3, 2025

Choose a reason for hiding this comment

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

It's only a wrapper, it doesn't take right...

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah, right. I thought this was actually constructing some kind of vector tiles.

} else {
this.vtLayers = this.encoding !== 'mlt' ?
new VectorTile(new Protobuf(this.rawTileData)).layers :
new MLTVectorTile(this.rawTileData).layers;
}
this.sourceLayerCoder = new DictionaryCoder(this.vtLayers ? Object.keys(this.vtLayers).sort() : [GEOJSON_TILE_LAYER_NAME]);
}
return this.vtLayers;
Expand Down
24 changes: 19 additions & 5 deletions src/source/geojson_worker_source.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,11 @@ describe('reloadTile', () => {
'geometry': {
'type': 'Point',
'coordinates': [0, 0]
},
'properties': {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Probably better to split this out to a different test...

'property1': 10,
'property2': undefined,
'property3': false
}
};
const tileParams = {
Expand All @@ -41,27 +46,36 @@ describe('reloadTile', () => {
maxZoom: 10
};

await source.loadData({source: 'sourceId', data: geoJson} as LoadGeoJSONParameters);
await source.loadData({type: 'geojson', source: 'sourceId', data: geoJson} as LoadGeoJSONParameters);

// first call should load vector data from geojson
const firstData = await source.reloadTile(tileParams as any as WorkerTileParameters);
expect(spy).toHaveBeenCalledTimes(1);

// second call won't give us new rawTileData
// geojson shouldn't encode any tile data, just provide features
expect(firstData.rawTileData).toBeUndefined();

// geojson should support undefined properties
expect(firstData.geoJsonFeatures[0].tags).toStrictEqual({
'property1': 10,
'property2': undefined,
'property3': false
});

// second data should return the same features
let data = await source.reloadTile(tileParams as any as WorkerTileParameters);
expect('rawTileData' in data).toBeFalsy();
data.rawTileData = firstData.rawTileData;
expect(data).toEqual(firstData);

// also shouldn't call loadVectorData again
expect(spy).toHaveBeenCalledTimes(1);

// replace geojson data
await source.loadData({source: 'sourceId', data: geoJson} as LoadGeoJSONParameters);
await source.loadData({type: 'geojson', source: 'sourceId', data: geoJson} as LoadGeoJSONParameters);

// should call loadVectorData again after changing geojson data
data = await source.reloadTile(tileParams as any as WorkerTileParameters);
expect('rawTileData' in data).toBeTruthy();
expect(data.geoJsonFeatures).toBeTruthy();
expect(data).toEqual(firstData);
expect(spy).toHaveBeenCalledTimes(2);
});
Expand Down
7 changes: 3 additions & 4 deletions src/source/geojson_worker_source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ import geojsonvt, {type Options as GeoJSONVTOptions} from 'geojson-vt';
import {VectorTileWorkerSource} from './vector_tile_worker_source';
import {createExpression} from '@maplibre/maplibre-gl-style-spec';
import {isAbortError} from '../util/abort_error';
import {toVirtualVectorTile} from './vector_tile_overzoomed';
import {isUpdateableGeoJSON, type GeoJSONSourceDiff, applySourceDiff, toUpdateable, type GeoJSONFeatureId} from './geojson_source_diff';
import type {WorkerTileParameters, WorkerTileResult} from './worker_source';
import type {LoadVectorTileResult} from './vector_tile_worker_source';
Expand Down Expand Up @@ -92,9 +91,9 @@ export class GeoJSONWorkerSource extends VectorTileWorkerSource {
return null;
}

const geojsonWrapper = new GeoJSONWrapper(geoJSONTile.features, {version: 2, extent: EXTENT});

return toVirtualVectorTile(geojsonWrapper);
return {
vectorTile: new GeoJSONWrapper(geoJSONTile.features, {version: 2, extent: EXTENT})
};
Copy link
Contributor

Choose a reason for hiding this comment

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

When we convert the GeoJSON data to a vector tile here, the data is encoded as a UInt8Array. This can be transferred to the main thread with zero overhead via the WebWorker transfer API.

The plain GeoJSON data cannot take advantage of the transfer API and will be much slower.

See https://developer.chrome.com/blog/transferable-objects-lightning-fast/ for more info.

Copy link
Contributor

Choose a reason for hiding this comment

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

Tested against my performance benchmark (add 100k points, move one, https://jsbin.com/bubejanoha/edit?html,output) this branch is ~100% slower than main.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Running your tool, I'm seeing about a 15% regression on my system over an average of 40 samples in Firefox. Could you add this test to the benchmarks? I think it'd be really valuable.

Copy link
Contributor

Choose a reason for hiding this comment

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

I've gotta step out for the afternoon but I'll try to get back to these questions in the next ~12 hrs.

Copy link
Contributor

Choose a reason for hiding this comment

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

Running your tool, I'm seeing about a 15% regression on my system over an average of 40 samples in Firefox.

Are you sure you're running npm run build-dist on this branch and on main? The dist build is not generated automatically as part of the dev server. I'm certain that you'll see a measurable and noticeable difference.

Could you add this test to the benchmarks? I think it'd be really valuable.

I added a GeoJSONDiff benchmark that measures perf on a relatively small (10k features) source. You can manually edit the bench to test a large (100k features) source but the runtime is prohibitively long to include in the standard suite.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks! Prior to merging main my local branch was behind enough to not include your GeoJSONDiff benchmark. Not many great options here seeing this regression. Indirectly looking up properties after querying the feature index might be the best approach if it works.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks! Prior to merging main my local branch was behind enough to not include your GeoJSONDiff benchmark.

If you pull in the latest changes from main you can compare this branch to main with one command. It takes a minute to run. Good time to get a glass of water, bother your pet, etc.

npm run benchmark -- GeoJSONSourceUpdateData && npm run benchmark -- GeoJSONSourceSetData 

Indirectly looking up properties after querying the feature index might be the best approach if it works.

I think we can make it work! The one big hurdle to navigate will be supporting GeoJSON sources with string feature IDs or no feature IDs. I'm happy to chat more, help brainstorm or prototype but I respect that you're the primary issue owner here.

Copy link
Contributor

@lucaswoj lucaswoj Dec 5, 2025

Choose a reason for hiding this comment

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

I think there are a couple other performance solutions worth considering...

  1. pin to the previous vector-tile-js version
  2. fork vector-tile-js to maintain support for null
  3. accept the breaking change (how many users are really differentiating between null and undefined in queryRenderedFeatures results?)
  4. use a "sentinel" value to represent null in vector tiles (like the string "__MAPLIBRE_NULL_VALUE__")

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

3 & 4 are certainly worth considering but I'm hesitant about using an older or modified version of vector-tile-js. For better or worse the current in-memory tile representation used is coupled to MVT, trying to ignore the specification (even if a minor deviation) doesn't sound too appealing to me.

While I question the usefulness of undefined/null GeoJSON properties, that specification allows for them so if possible I'd prefer a solution that complies with both formats. Other options like custom serialization, a forked vector tile deserializer or tile feature culling on the main thread would all add more complexity compared to a simple sentinel, so I'm currently leaning more towards that approach. An indirect lookup from the feature index would be the runner up, with the perk of not needing a special reserved property value.

@HarelM What are your thoughts on the possible approaches here? If we're fine introducing another reserved string then this whole PR becomes a lot more simple.

Copy link
Contributor

Choose a reason for hiding this comment

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

that specification allows for them so if possible I'd prefer a solution that complies with both formats

queryRenderedFeatures has never supported the GeoJSON specification. This project's stance has consistently been that GeoJSON gets converted to a vector tile internally and that's what comes out of qRF. For example, it's never supported arrays or nested objects. Support for the null value was a quirk and does not indicate a broader commitment to full support of the GeoJSON specification.

}

/**
Expand Down
1 change: 0 additions & 1 deletion src/source/vector_tile_worker_source.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ import {setPerformance, sleep} from '../util/test/util';
import {ABORT_ERROR} from '../util/abort_error';
import {SubdivisionGranularitySetting} from '../render/subdivision_granularity_settings';
import {VectorTile} from '@mapbox/vector-tile';
import {VectorTileLike} from '@maplibre/vt-pbf';
import Point from '@mapbox/point-geometry';

describe('vector tile worker source', () => {
Expand Down
20 changes: 15 additions & 5 deletions src/source/vector_tile_worker_source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,11 @@ import type {
import type {IActor} from '../util/actor';
import type {StyleLayer} from '../style/style_layer';
import type {StyleLayerIndex} from '../style/style_layer_index';
import type {VectorTileLayerLike, VectorTileLike} from '@maplibre/vt-pbf';
import {type VectorTileLayerLike, type VectorTileLike, GeoJSONWrapper} from '@maplibre/vt-pbf';

export type LoadVectorTileResult = {
vectorTile: VectorTileLike;
rawData: ArrayBufferLike;
rawData?: ArrayBufferLike;
resourceTiming?: Array<PerformanceResourceTiming>;
} & ExpiryData;

Expand Down Expand Up @@ -148,7 +148,11 @@ export class VectorTileWorkerSource implements WorkerSource {
try {
const result = await parsePromise;
// Transferring a copy of rawTileData because the worker needs to retain its copy.
return extend({rawTileData: rawTileData.slice(0), encoding: params.encoding}, result, cacheControl, resourceTiming);
return extend({
rawTileData: rawTileData?.slice(0),
geoJsonFeatures: workerTile.vectorTile instanceof GeoJSONWrapper ? (workerTile.vectorTile as GeoJSONWrapper).features : null,
encoding: params.encoding
}, result, cacheControl, resourceTiming);
} finally {
delete this.fetching[tileUid];
}
Expand Down Expand Up @@ -214,7 +218,11 @@ export class VectorTileWorkerSource implements WorkerSource {
if (this.fetching[uid]) {
const {rawTileData, cacheControl, resourceTiming} = this.fetching[uid];
delete this.fetching[uid];
parseResult = extend({rawTileData: rawTileData.slice(0), encoding: params.encoding}, result, cacheControl, resourceTiming);
parseResult = extend({
rawTileData: rawTileData?.slice(0),
geoJsonFeatures: workerTile.vectorTile instanceof GeoJSONWrapper ? (workerTile.vectorTile as GeoJSONWrapper).features : null,
encoding: params.encoding
}, result, cacheControl, resourceTiming);
} else {
parseResult = result;
}
Expand All @@ -224,7 +232,9 @@ export class VectorTileWorkerSource implements WorkerSource {
// if there was no vector tile data on the initial load, don't try and re-parse tile
if (workerTile.status === 'done' && workerTile.vectorTile) {
// this seems like a missing case where cache control is lost? see #3309
return workerTile.parse(workerTile.vectorTile, this.layerIndex, this.availableImages, this.actor, params.subdivisionGranularity);
return extend({
geoJsonFeatures: workerTile.vectorTile instanceof GeoJSONWrapper ? (workerTile.vectorTile as GeoJSONWrapper).features : null,
}, await workerTile.parse(workerTile.vectorTile, this.layerIndex, this.availableImages, this.actor, params.subdivisionGranularity));
}
}

Expand Down
2 changes: 2 additions & 0 deletions src/source/worker_source.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,6 +15,7 @@ import type {IActor} from '../util/actor';
import type {StyleLayerIndex} from '../style/style_layer_index';
import type {SubdivisionGranularitySetting} from '../render/subdivision_granularity_settings';
import type {DashEntry} from '../render/line_atlas';
import type {Feature} from '@maplibre/vt-pbf';

/**
* Parameters to identify a tile
Expand Down Expand Up @@ -79,6 +80,7 @@ export type WorkerTileResult = ExpiryData & {
featureIndex: FeatureIndex;
collisionBoxArray: CollisionBoxArray;
rawTileData?: ArrayBuffer;
geoJsonFeatures?: Feature[];
encoding?: string;
resourceTiming?: Array<PerformanceResourceTiming>;
// Only used for benchmarking:
Expand Down
10 changes: 7 additions & 3 deletions src/tile/tile.ts
Original file line number Diff line number Diff line change
Expand Up @@ -218,7 +218,11 @@ export class Tile {

if (data.featureIndex) {
this.latestFeatureIndex = data.featureIndex;
if (data.rawTileData) {
if (data.geoJsonFeatures) {
// GeoJSON sources provide feature data directly instead of encoded
// tile data
this.latestFeatureIndex.geoJsonFeatureData = data.geoJsonFeatures;
} else if (data.rawTileData) {
// Only vector tiles have rawTileData, and they won't update it for
// 'reloadTile'
this.latestRawTileData = data.rawTileData;
Expand Down Expand Up @@ -350,7 +354,7 @@ export class Tile {
pixelPosMatrix: mat4,
getElevation: undefined | ((x: number, y: number) => number)
): QueryResults {
if (!this.latestFeatureIndex || !this.latestFeatureIndex.rawTileData)
if (!this.latestFeatureIndex || !(this.latestFeatureIndex.rawTileData || this.latestFeatureIndex.geoJsonFeatureData))
return {};

return this.latestFeatureIndex.query({
Expand Down Expand Up @@ -466,7 +470,7 @@ export class Tile {

setFeatureState(states: LayerFeatureStates, painter: any) {
if (!this.latestFeatureIndex ||
!this.latestFeatureIndex.rawTileData ||
!(this.latestFeatureIndex.rawTileData || this.latestFeatureIndex.geoJsonFeatureData) ||
Object.keys(states).length === 0) {
return;
}
Expand Down
4 changes: 3 additions & 1 deletion src/util/vectortile_to_geojson.ts
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,9 @@ export class GeoJSONFeature {
case 1: {
const points = [];
for (const line of vtCoords) {
points.push(line[0]);
for (const point of line) {
points.push(point);
}
}
const coordinates = this.projectLine(points, x0, y0, size);
this._geometry = points.length === 1 ?
Expand Down