Skip to content

fix: Don't serialize GeoJSON vector tiles from the worker to support undefined properties #6803

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.

Sign up for GitHub

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

Draft
wants to merge 4 commits into
base: main
Choose a base branch
from
+58 −23
Draft

fix: Don't serialize GeoJSON vector tiles from the worker to support undefined properties #6803

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
bd4d6a8
Transfer raw feature data from GeoJSON worker and pass to FeatureIndex
mwilsnd Nov 20, 2025
db722ad
Merge branch 'main' into fix-undefined-mvt-properties
mwilsnd Dec 2, 2025
d8e4f47
Fix setFeatureState, query, clean up GeoJSONWrapper handling
mwilsnd Dec 2, 2025
619322b
Merge branch 'main' into fix-undefined-mvt-properties
mwilsnd Dec 3, 2025
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
View file
Open in desktop
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

@lucaswoj lucaswoj Dec 3, 2025

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 •
edited
Loading

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

@lucaswoj lucaswoj Dec 3, 2025

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
View file
Open in desktop
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

@HarelM HarelM Dec 2, 2025

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
6 changes: 4 additions & 2 deletions src/source/geojson_worker_source.ts
View file
Open in desktop
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 @@ -94,7 +93,10 @@ export class GeoJSONWorkerSource extends VectorTileWorkerSource {

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

return toVirtualVectorTile(geojsonWrapper);
return {
vectorTile: geojsonWrapper,
isGeojson: true
};
Copy link
Contributor

@lucaswoj lucaswoj Dec 2, 2025

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

@lucaswoj lucaswoj Dec 2, 2025

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

@mwilsnd mwilsnd Dec 2, 2025

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

@lucaswoj lucaswoj Dec 2, 2025

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

@lucaswoj lucaswoj Dec 3, 2025

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

@mwilsnd mwilsnd Dec 5, 2025

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

@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.

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 •
edited
Loading

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

@mwilsnd mwilsnd Dec 5, 2025

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

@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.

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
22 changes: 17 additions & 5 deletions src/source/vector_tile_worker_source.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,11 +16,12 @@
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';

Check failure on line 19 in src/source/vector_tile_worker_source.ts

View workflow job for this annotation

GitHub Actions / Code Hygiene 

All imports in the declaration are only used as types. Use `import type`

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

Expand Down Expand Up @@ -140,6 +141,7 @@
}

workerTile.vectorTile = response.vectorTile;
workerTile.geoJsonFeatures = response.isGeojson ? (response.vectorTile as GeoJSONWrapper).features : null;
Copy link
Collaborator

@HarelM HarelM Dec 2, 2025

Choose a reason for hiding this comment

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

I wonder if this can be done better considering we have inheritance here with geojson and vector tile...

Copy link
Collaborator Author

@mwilsnd mwilsnd Dec 2, 2025

Choose a reason for hiding this comment

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

Is reflecting the type of an object here more or less the same, performance-wise? No issues with changing this.

Copy link
Collaborator

@HarelM HarelM Dec 2, 2025

Choose a reason for hiding this comment

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

Less about reflection, more about OOP... if you are doing as GeoJSONWrapper in a class that doesn't suppose to know about GeoJSONWrapper then it seems weird...

const parsePromise = workerTile.parse(response.vectorTile, this.layerIndex, this.availableImages, this.actor, params.subdivisionGranularity);
this.loaded[tileUid] = workerTile;
// keep the original fetching state so that reload tile can pick it up if the original parse is cancelled by reloads' parse
Expand All @@ -148,7 +150,11 @@
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.geoJsonFeatures,
encoding: params.encoding
}, result, cacheControl, resourceTiming);
} finally {
delete this.fetching[tileUid];
}
Expand Down Expand Up @@ -214,7 +220,11 @@
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.geoJsonFeatures,
encoding: params.encoding
}, result, cacheControl, resourceTiming);
} else {
parseResult = result;
}
Expand All @@ -224,7 +234,9 @@
// 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.geoJsonFeatures
}, 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
View file
Open in desktop
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
3 changes: 3 additions & 0 deletions src/source/worker_tile.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,8 @@ import type {PromoteIdSpecification} from '@maplibre/maplibre-gl-style-spec';
import type {VectorTileLike} from '@maplibre/vt-pbf';
import {type GetDashesResponse, MessageType, type GetGlyphsResponse, type GetImagesResponse} from '../util/actor_messages';
import type {SubdivisionGranularitySetting} from '../render/subdivision_granularity_settings';
import type {Feature} from '@maplibre/vt-pbf';

export class WorkerTile {
tileID: OverscaledTileID;
uid: string | number;
Expand All @@ -43,6 +45,7 @@ export class WorkerTile {

abort: AbortController;
vectorTile: VectorTileLike;
geoJsonFeatures?: Feature[];
Copy link
Collaborator

@HarelM HarelM Dec 2, 2025

Choose a reason for hiding this comment

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

Can't this be inferred from the vectorTile? It is mandatory to add this field?

Copy link
Collaborator Author

@mwilsnd mwilsnd Dec 2, 2025

Choose a reason for hiding this comment

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

I think so - with the update to vt-pbf I can likely simplify things further.

Copy link
Collaborator

@HarelM HarelM Dec 2, 2025

Choose a reason for hiding this comment

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

It doesn't look it is being used in the parse method, so I wonder if this can be removed from WorkerTile class...

inFlightDependencies: AbortController[];

constructor(params: WorkerTileParameters) {
Expand Down
6 changes: 5 additions & 1 deletion src/tile/tile.ts
View file
Open in desktop
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
Loading