From cce1503056ac9c624c19d36b67228e016f555bd2 Mon Sep 17 00:00:00 2001 From: HarelM Date: Tue, 2 Dec 2025 13:08:16 +0200 Subject: [PATCH 01/15] Use only bounds for tile reload --- src/source/geojson_source.test.ts | 5 +- src/source/geojson_source.ts | 70 ++++---------- src/source/geojson_source_diff.ts | 11 ++- src/source/geojson_worker_source.ts | 2 +- test/examples/display-a-map.html | 143 +++++++++++++++++++++++++--- 5 files changed, 166 insertions(+), 65 deletions(-) diff --git a/src/source/geojson_source.test.ts b/src/source/geojson_source.test.ts index cfc3196e907..a01fa48e7e9 100644 --- a/src/source/geojson_source.test.ts +++ b/src/source/geojson_source.test.ts @@ -936,7 +936,7 @@ describe('GeoJSONSource.applyDiff', () => { }); }); }); - + /* describe('GeoJSONSource.shoudReloadTile', () => { let source: GeoJSONSource; @@ -959,6 +959,7 @@ describe('GeoJSONSource.shoudReloadTile', () => { return tile; } + test('returns true when diff.removeAll is true', () => { const diff: GeoJSONSourceDiff = {removeAll: true}; @@ -1114,4 +1115,6 @@ describe('GeoJSONSource.shoudReloadTile', () => { expect(source.shouldReloadTile(getMockTile(5, 0, 15, []), source._getShouldReloadTileOptions(diff))).toBe(true); expect(source.shouldReloadTile(getMockTile(5, 31, 15, []), source._getShouldReloadTileOptions(diff))).toBe(true); }); + }); + */ \ No newline at end of file diff --git a/src/source/geojson_source.ts b/src/source/geojson_source.ts index efa77338b09..2b66ab28e6b 100644 --- a/src/source/geojson_source.ts +++ b/src/source/geojson_source.ts @@ -45,12 +45,7 @@ export type GeoJSONSourceShouldReloadTileOptions = { /** * Refresh all tiles that WILL contain these bounds. */ - nextBounds: LngLatBounds[]; - - /** - * Refresh all tiles that PREVIOUSLY DID contain these feature ids. - */ - prevIds: Set; + affectedBounds: LngLatBounds[]; }; /** @@ -423,8 +418,10 @@ export class GeoJSONSource extends Evented implements Source { this.fire(new Event('dataabort', {dataType: 'source'})); return; } - - if (diff) this._applyDiff(diff); + let shouldReloadTileOptions: GeoJSONSourceShouldReloadTileOptions | undefined; + if (diff) { + shouldReloadTileOptions = this._applyDiff(diff); + } let resourceTiming: PerformanceResourceTiming[] = null; if (result.resourceTiming && result.resourceTiming[this.id]) { @@ -439,7 +436,7 @@ export class GeoJSONSource extends Evented implements Source { // although GeoJSON sources contain no metadata, we fire this event to let the TileManager // know its ok to start requesting tiles. this.fire(new Event('data', {...eventData, sourceDataType: 'metadata'})); - this.fire(new Event('data', {...eventData, sourceDataType: 'content', shouldReloadTileOptions: this._getShouldReloadTileOptions(diff)})); + this.fire(new Event('data', {...eventData, sourceDataType: 'content', shouldReloadTileOptions})); } catch (err) { this._isUpdatingWorker = false; if (this._removed) { @@ -455,7 +452,7 @@ export class GeoJSONSource extends Evented implements Source { } } - private _applyDiff(diff: GeoJSONSourceDiff): void { + private _applyDiff(diff: GeoJSONSourceDiff): GeoJSONSourceShouldReloadTileOptions { const promoteId = typeof this.promoteId === 'string' ? this.promoteId : undefined; // Lazily convert `this._data` to updateable if it's not already @@ -467,69 +464,44 @@ export class GeoJSONSource extends Evented implements Source { } if (this._data.updateable) { - applySourceDiff(this._data.updateable, diff, promoteId); - } - } - - _getShouldReloadTileOptions(diff?: GeoJSONSourceDiff): GeoJSONSourceShouldReloadTileOptions | undefined { - if (this._options.cluster || !diff || diff.removeAll) return undefined; - - const {add = [], update = [], remove = []} = (diff || {}); + const affectedGeometries = applySourceDiff(this._data.updateable, diff, promoteId); - const prevIds = new Set([...update.map(u => u.id), ...remove]); - - for (const id of prevIds.values()) { - if (typeof id !== 'number' && this.promoteId == null) { - warnOnce(`GeoJSONSource "${this.id}": updateData is slower when using string GeoJSON feature IDs. Consider using promoteId or numeric IDs for better performance.`); + if (this._options.cluster || diff.removeAll) { return undefined; } + + const affectedBounds = affectedGeometries + .filter(Boolean) + .map(g => getGeoJSONBounds(g)); + + return { + affectedBounds, + }; } - - const nextBounds = [ - ...update.map(f => f.newGeometry), - ...add.map(f => f.geometry) - ] - .filter(Boolean) - .map(g => getGeoJSONBounds(g)); - - return { - nextBounds, - prevIds - }; + return undefined; } /** * Determine whether a tile should be reloaded based on a set of options associated with a {@link MapSourceDataChangedEvent}. * @internal */ - shouldReloadTile(tile: Tile, {nextBounds, prevIds}: GeoJSONSourceShouldReloadTileOptions) : boolean { + shouldReloadTile(tile: Tile, {affectedBounds}: GeoJSONSourceShouldReloadTileOptions) : boolean { if (!tile.latestFeatureIndex) { return tile.state !== 'unloaded'; } - // Update the tile if it PREVIOUSLY contained an updated feature. - const layers = tile.latestFeatureIndex.loadVTLayers(); - for (let i = 0; i < tile.latestFeatureIndex.featureIndexArray.length; i++) { - const featureIndex = tile.latestFeatureIndex.featureIndexArray.get(i); - const feature = layers[GEOJSON_TILE_LAYER_NAME].feature(featureIndex.featureIndex); - const id = tile.latestFeatureIndex.getId(feature, GEOJSON_TILE_LAYER_NAME); - if (prevIds.has(id)) { - return true; - } - } - // Update the tile if it WILL NOW contain an updated feature. const {buffer, extent} = this.workerOptions.geojsonVtOptions; const tileBounds = tileIdToLngLatBounds( tile.tileID.canonical, buffer / extent ); - for (const bounds of nextBounds) { + for (const bounds of affectedBounds) { if (tileBounds.intersects(bounds)) { return true; } } - + console.log('shouldReloadTile false for tile: ', tile.tileID.canonical.toString()); return false; } diff --git a/src/source/geojson_source_diff.ts b/src/source/geojson_source_diff.ts index aa35ed23a11..94d102272ac 100644 --- a/src/source/geojson_source_diff.ts +++ b/src/source/geojson_source_diff.ts @@ -1,4 +1,3 @@ - /** * A way to identify a feature, either by string or by number */ @@ -126,13 +125,16 @@ export function toUpdateable(data: UpdateableGeoJSON, promoteId?: string) { * 1. Remove operations (removeAll, remove) * 2. Add operations (add) * 3. Update operations (update) + * @returns an array of geometries that were affected by the diff */ -export function applySourceDiff(updateable: Map, diff: GeoJSONSourceDiff, promoteId?: string): void { +export function applySourceDiff(updateable: Map, diff: GeoJSONSourceDiff, promoteId?: string): GeoJSON.Geometry[] { + const affectedGeometries: GeoJSON.Geometry[] = []; if (diff.removeAll) { updateable.clear(); } else if (diff.remove) { for (const id of diff.remove) { + affectedGeometries.push(updateable.get(id)?.geometry); updateable.delete(id); } } @@ -141,6 +143,7 @@ export function applySourceDiff(updateable: Map { - let data; + let data: GeoJSON.GeoJSON; if (params.request) { // Data is loaded from a fetchable URL diff --git a/test/examples/display-a-map.html b/test/examples/display-a-map.html index 016827816ca..8c3a2be4949 100644 --- a/test/examples/display-a-map.html +++ b/test/examples/display-a-map.html @@ -1,27 +1,146 @@ + Display a map + + + -
- +
+ +
+

Map Control

+ +
+ + + \ No newline at end of file From e5caba2f7fd1e25195f59209b083b88fea39596e Mon Sep 17 00:00:00 2001 From: HarelM Date: Tue, 2 Dec 2025 13:13:41 +0200 Subject: [PATCH 02/15] Fix lint --- src/source/geojson_source.test.ts | 9 ++++----- src/source/geojson_source.ts | 1 - src/source/vector_tile_worker_source.test.ts | 1 - 3 files changed, 4 insertions(+), 7 deletions(-) diff --git a/src/source/geojson_source.test.ts b/src/source/geojson_source.test.ts index a01fa48e7e9..af400d3396f 100644 --- a/src/source/geojson_source.test.ts +++ b/src/source/geojson_source.test.ts @@ -1,4 +1,4 @@ -import {describe, test, expect, vi, beforeEach} from 'vitest'; +import {describe, test, expect, vi, /*beforeEach*/} from 'vitest'; import {Tile} from '../tile/tile'; import {OverscaledTileID} from '../tile/tile_id'; import {GeoJSONSource, type GeoJSONSourceOptions} from './geojson_source'; @@ -8,7 +8,7 @@ import {extend} from '../util/util'; import {SubdivisionGranularitySetting} from '../render/subdivision_granularity_settings'; import {MercatorTransform} from '../geo/projection/mercator_transform'; import {sleep, waitForEvent} from '../util/test/util'; -import {FeatureIndex, GEOJSON_TILE_LAYER_NAME} from '../data/feature_index'; +//import {FeatureIndex, GEOJSON_TILE_LAYER_NAME} from '../data/feature_index'; import {type ActorMessage, MessageType} from '../util/actor_messages'; import type {IReadonlyTransform} from '../geo/transform_interface'; import type {Dispatcher} from '../util/dispatcher'; @@ -16,7 +16,7 @@ import type {RequestManager} from '../util/request_manager'; import type {Actor} from '../util/actor'; import type {MapSourceDataEvent} from '../ui/events'; import type {GeoJSONSourceDiff, UpdateableGeoJSON} from './geojson_source_diff'; -import type {VectorTileFeatureLike, VectorTileLayerLike} from '@maplibre/vt-pbf'; +//import type {VectorTileFeatureLike, VectorTileLayerLike} from '@maplibre/vt-pbf'; const wrapDispatcher = (dispatcher) => { return { @@ -936,7 +936,7 @@ describe('GeoJSONSource.applyDiff', () => { }); }); }); - /* +/* describe('GeoJSONSource.shoudReloadTile', () => { let source: GeoJSONSource; @@ -959,7 +959,6 @@ describe('GeoJSONSource.shoudReloadTile', () => { return tile; } - test('returns true when diff.removeAll is true', () => { const diff: GeoJSONSourceDiff = {removeAll: true}; diff --git a/src/source/geojson_source.ts b/src/source/geojson_source.ts index 2b66ab28e6b..429e4a069ed 100644 --- a/src/source/geojson_source.ts +++ b/src/source/geojson_source.ts @@ -7,7 +7,6 @@ import {applySourceDiff, isUpdateableGeoJSON, mergeSourceDiffs, toUpdateable} fr import {getGeoJSONBounds} from '../util/geojson_bounds'; import {MessageType} from '../util/actor_messages'; import {tileIdToLngLatBounds} from '../tile/tile_id_to_lng_lat_bounds'; -import {GEOJSON_TILE_LAYER_NAME} from '../data/feature_index'; import type {LngLatBounds} from '../geo/lng_lat_bounds'; import type {Source} from './source'; diff --git a/src/source/vector_tile_worker_source.test.ts b/src/source/vector_tile_worker_source.test.ts index bfef8c6acc3..830a3acaf37 100644 --- a/src/source/vector_tile_worker_source.test.ts +++ b/src/source/vector_tile_worker_source.test.ts @@ -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', () => { From bb535d19879f700efde27318545bf44732121189 Mon Sep 17 00:00:00 2001 From: HarelM Date: Tue, 2 Dec 2025 13:33:11 +0200 Subject: [PATCH 03/15] Update min test --- test/build/min.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/test/build/min.test.ts b/test/build/min.test.ts index 31a5b548e8b..8ebff5ae6a2 100644 --- a/test/build/min.test.ts +++ b/test/build/min.test.ts @@ -38,7 +38,7 @@ describe('test min build', () => { const decreaseQuota = 4096; // feel free to update this value after you've checked that it has changed on purpose :-) - const expectedBytes = 1015555; + const expectedBytes = 1011249; expect(actualBytes, `Consider changing expectedBytes to: ${actualBytes}`).toBeLessThan(expectedBytes + increaseQuota); expect(actualBytes).toBeGreaterThan(expectedBytes - decreaseQuota); From c9799d55150e1bc605b2e58e5c52970a2deb2582 Mon Sep 17 00:00:00 2001 From: HarelM Date: Tue, 2 Dec 2025 13:36:24 +0200 Subject: [PATCH 04/15] Remove console log --- src/source/geojson_source.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/src/source/geojson_source.ts b/src/source/geojson_source.ts index 429e4a069ed..0a6c0837044 100644 --- a/src/source/geojson_source.ts +++ b/src/source/geojson_source.ts @@ -500,7 +500,6 @@ export class GeoJSONSource extends Evented implements Source { return true; } } - console.log('shouldReloadTile false for tile: ', tile.tileID.canonical.toString()); return false; } From fcf2d113fc1653a0268adab9a153947966e996b9 Mon Sep 17 00:00:00 2001 From: HarelM Date: Tue, 2 Dec 2025 13:45:01 +0200 Subject: [PATCH 05/15] Revert accidental file commit. --- test/examples/display-a-map.html | 129 +++---------------------------- 1 file changed, 9 insertions(+), 120 deletions(-) diff --git a/test/examples/display-a-map.html b/test/examples/display-a-map.html index 8c3a2be4949..860c9f90fd0 100644 --- a/test/examples/display-a-map.html +++ b/test/examples/display-a-map.html @@ -6,8 +6,6 @@ - -
- -
-

Map Control

- -
- - - - \ No newline at end of file + \ No newline at end of file From 811a12c799da60a36d56106cac567078dea8f2be Mon Sep 17 00:00:00 2001 From: Harel M Date: Tue, 2 Dec 2025 13:46:37 +0200 Subject: [PATCH 06/15] revert display-a-map file --- test/examples/display-a-map.html | 36 +++++++++++++------------------- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/test/examples/display-a-map.html b/test/examples/display-a-map.html index 860c9f90fd0..32a2c43f142 100644 --- a/test/examples/display-a-map.html +++ b/test/examples/display-a-map.html @@ -1,6 +1,5 @@ - Display a map @@ -9,27 +8,20 @@ - -
- \ No newline at end of file +
+ + + From 82cfc63d8c03dbe661775db5b8c64a69edb5e75e Mon Sep 17 00:00:00 2001 From: HarelM Date: Tue, 2 Dec 2025 14:04:38 +0200 Subject: [PATCH 07/15] Reduce indentation --- src/source/geojson_source.ts | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-) diff --git a/src/source/geojson_source.ts b/src/source/geojson_source.ts index 0a6c0837044..ff778eaecb7 100644 --- a/src/source/geojson_source.ts +++ b/src/source/geojson_source.ts @@ -462,22 +462,22 @@ export class GeoJSONSource extends Evented implements Source { this._data = {updateable: toUpdateable(this._data.geojson, promoteId)}; } - if (this._data.updateable) { - const affectedGeometries = applySourceDiff(this._data.updateable, diff, promoteId); + if (!this._data.updateable) { + return undefined; + } + const affectedGeometries = applySourceDiff(this._data.updateable, diff, promoteId); - if (this._options.cluster || diff.removeAll) { - return undefined; - } - - const affectedBounds = affectedGeometries - .filter(Boolean) - .map(g => getGeoJSONBounds(g)); - - return { - affectedBounds, - }; + if (this._options.cluster || diff.removeAll) { + return undefined; } - return undefined; + + const affectedBounds = affectedGeometries + .filter(Boolean) + .map(g => getGeoJSONBounds(g)); + + return { + affectedBounds, + }; } /** From bc023cad06dad92a544ad2f1b5fa97dd25c1ed5f Mon Sep 17 00:00:00 2001 From: Harel M Date: Tue, 2 Dec 2025 14:06:11 +0200 Subject: [PATCH 08/15] Fix missing newline at end of display-a-map.html Add a newline at the end of the HTML file. From e6398a6fa7c8b5e1c5599d72e81d4ca9e3581eca Mon Sep 17 00:00:00 2001 From: HarelM Date: Tue, 2 Dec 2025 15:47:33 +0200 Subject: [PATCH 09/15] Updated tests --- src/geo/lng_lat_bounds.test.ts | 6 + src/source/geojson_source.test.ts | 246 ++++++++++--------------- src/source/geojson_source.ts | 8 +- src/source/geojson_source_diff.test.ts | 33 ++-- 4 files changed, 127 insertions(+), 166 deletions(-) diff --git a/src/geo/lng_lat_bounds.test.ts b/src/geo/lng_lat_bounds.test.ts index 0f5c7368fef..da3297bf869 100644 --- a/src/geo/lng_lat_bounds.test.ts +++ b/src/geo/lng_lat_bounds.test.ts @@ -423,6 +423,12 @@ describe('LngLatBounds', () => { const bounds = new LngLatBounds([-180, 5], [-175, 10]); expect(tileBounds.intersects(bounds)).toBe(true); }); + + test('entire worlds tile should return true', () => { + const tileBounds = new LngLatBounds([270, -88.97061836630758], [-270, 88.97061836630758]); + const bounds = new LngLatBounds([0, 0], [10, 10]); + expect(tileBounds.intersects(bounds)).toBe(true); + }); }); }); }); diff --git a/src/source/geojson_source.test.ts b/src/source/geojson_source.test.ts index af400d3396f..b9906902a82 100644 --- a/src/source/geojson_source.test.ts +++ b/src/source/geojson_source.test.ts @@ -1,14 +1,13 @@ -import {describe, test, expect, vi, /*beforeEach*/} from 'vitest'; +import {describe, test, expect, vi, beforeEach} from 'vitest'; import {Tile} from '../tile/tile'; import {OverscaledTileID} from '../tile/tile_id'; -import {GeoJSONSource, type GeoJSONSourceOptions} from './geojson_source'; +import {GeoJSONSource, type GeoJSONSourceShouldReloadTileOptions, type GeoJSONSourceOptions} from './geojson_source'; import {EXTENT} from '../data/extent'; import {LngLat} from '../geo/lng_lat'; import {extend} from '../util/util'; import {SubdivisionGranularitySetting} from '../render/subdivision_granularity_settings'; import {MercatorTransform} from '../geo/projection/mercator_transform'; import {sleep, waitForEvent} from '../util/test/util'; -//import {FeatureIndex, GEOJSON_TILE_LAYER_NAME} from '../data/feature_index'; import {type ActorMessage, MessageType} from '../util/actor_messages'; import type {IReadonlyTransform} from '../geo/transform_interface'; import type {Dispatcher} from '../util/dispatcher'; @@ -16,7 +15,6 @@ import type {RequestManager} from '../util/request_manager'; import type {Actor} from '../util/actor'; import type {MapSourceDataEvent} from '../ui/events'; import type {GeoJSONSourceDiff, UpdateableGeoJSON} from './geojson_source_diff'; -//import type {VectorTileFeatureLike, VectorTileLayerLike} from '@maplibre/vt-pbf'; const wrapDispatcher = (dispatcher) => { return { @@ -641,12 +639,8 @@ describe('GeoJSONSource.updateData', () => { add: [{id: '5', type: 'Feature', properties: {}, geometry: {type: 'LineString', coordinates: []}}], update: [{id: '6', addOrUpdateProperties: [], newGeometry: {type: 'LineString', coordinates: []}}] } satisfies GeoJSONSourceDiff; - source.updateData(update1); - source.updateData(update2); - - // Wait for both updateData calls to be performed - await waitForEvent(source, 'data', (e: MapSourceDataEvent) => e.sourceDataType === 'metadata'); - await waitForEvent(source, 'data', (e: MapSourceDataEvent) => e.sourceDataType === 'metadata'); + await source.updateData(update1, true); + await source.updateData(update2, true); expect(spy).toHaveBeenCalledTimes(2); expect(spy.mock.calls[0][0].data.dataDiff).toEqual(update1); @@ -775,18 +769,6 @@ describe('GeoJSONSource.updateData', () => { expect(spy.mock.calls[2][0].data.dataDiff).toEqual(update1); }); - test('updateData with waitForCompletion=true returns promise that resolves to this', async () => { - const source = new GeoJSONSource('id', {} as any, wrapDispatcher({ - sendAsync(_message: ActorMessage) { - return new Promise((resolve) => { - setTimeout(() => resolve({abandoned: true}), 0); - }); - } - }), undefined); - const result = source.updateData({add: []} as GeoJSONSourceDiff, true); - expect(result).toBeInstanceOf(Promise); - }); - test('throws error when updating data that is not compatible with updateData', async () => { const initialData: GeoJSON.FeatureCollection = { type: 'FeatureCollection', @@ -925,8 +907,7 @@ describe('GeoJSONSource.applyDiff', () => { const diff: GeoJSONSourceDiff = { update: [{id: 0, newGeometry: {type: 'Point', coordinates: [0, 1]}}] }; - source.updateData(diff); - await waitForEvent(source, 'data', (e: MapSourceDataEvent) => e.sourceDataType === 'metadata'); + await source.updateData(diff, true); expect(source.serialize().data).toEqual({ type: 'FeatureCollection', @@ -936,92 +917,86 @@ describe('GeoJSONSource.applyDiff', () => { }); }); }); -/* + describe('GeoJSONSource.shoudReloadTile', () => { let source: GeoJSONSource; + let tile: Tile; beforeEach(() => { source = new GeoJSONSource('id', {data: {}} as GeoJSONSourceOptions, mockDispatcher, undefined); + tile = new Tile(new OverscaledTileID(0, 0, 0, 0, 0), source.tileSize); }); - function getMockTile(z: number, x: number, y: number, features: Array>) { - const tile = new Tile(new OverscaledTileID(z, 0, z, x, y), source.tileSize); - tile.latestFeatureIndex = new FeatureIndex(tile.tileID, source.promoteId); - tile.latestFeatureIndex.vtLayers = { - [GEOJSON_TILE_LAYER_NAME]: { - feature: (i: number) => features[i] || {} - } as VectorTileLayerLike - }; + test('returns true when tile is still loading', () => { + const result = source.shouldReloadTile(tile, {} as GeoJSONSourceShouldReloadTileOptions); - for (let i = 0; i < features.length; i++) { - tile.latestFeatureIndex.insert(features[i] as VectorTileFeatureLike, [], i, 0, 0, false); - } - return tile; - } + expect(result).toBe(true); + }); - test('returns true when diff.removeAll is true', () => { - const diff: GeoJSONSourceDiff = {removeAll: true}; + test('returns false when tile has been unloaded', () => { + tile.state = 'unloaded'; - const result = source._getShouldReloadTileOptions(diff); + const result = source.shouldReloadTile(tile, {} as GeoJSONSourceShouldReloadTileOptions); - expect(result).toBe(undefined); + expect(result).toBe(false); }); - test('returns true when tile contains a feature that is being updated', () => { - const tile = getMockTile(0, 0, 0, [{id: 0}]); - const diff: GeoJSONSourceDiff = { - update: [{ - id: 0, - newGeometry: {type: 'Point', coordinates: [0, 0]} - }] - }; + test('fires undefined when diff.removeAll is true', async () => { + tile.state = 'loaded'; + const diff: GeoJSONSourceDiff = {removeAll: true}; - const result = source.shouldReloadTile(tile, source._getShouldReloadTileOptions(diff)); + let shouldReloadTileOptions: GeoJSONSourceShouldReloadTileOptions = undefined; + source.on('data', (e) => { + if (e.shouldReloadTileOptions) { + shouldReloadTileOptions = e.shouldReloadTileOptions; + } + }); + await source.updateData(diff, true); - expect(result).toBe(true); + expect(shouldReloadTileOptions).toBeUndefined(); }); - test('returns true when tile contains a feature that is being updated via addOrUpdateProperties', () => { - const tile = getMockTile(0, 0, 0, [{id: 0}]); + test('returns true when tile contains a feature that is being updated', async () => { + tile.state = 'loaded'; const diff: GeoJSONSourceDiff = { update: [{ id: 0, - addOrUpdateProperties: [{key: 'foo', value: true}] + newGeometry: {type: 'Point', coordinates: [1, 1]} }] }; - const result = source.shouldReloadTile(tile, source._getShouldReloadTileOptions(diff)); + let shouldReloadTileOptions: GeoJSONSourceShouldReloadTileOptions = undefined; + source.on('data', (e) => { + if (e.shouldReloadTileOptions) { + shouldReloadTileOptions = e.shouldReloadTileOptions; + } + }); + await source.setData({type: 'FeatureCollection', features: [{type: 'Feature', id: 0, properties: {}, geometry: {type: 'Point', coordinates: [0, 0]}}]}, true); + await source.updateData(diff, true); + const result = source.shouldReloadTile(tile, shouldReloadTileOptions); - expect(result).toBe(true); + expect(result).toBeTruthy(); }); - - test('returns true when tile contains a feature that is being removed', () => { - const tile = getMockTile(0, 0, 0, [{id: 0}]); + + test('returns false when tile contains a feature that is being removed but was never added', async () => { + tile.state = 'loaded'; const diff: GeoJSONSourceDiff = {remove: [0]}; + let shouldReloadTileOptions: GeoJSONSourceShouldReloadTileOptions = undefined; + source.on('data', (e) => { + if (e.shouldReloadTileOptions) { + shouldReloadTileOptions = e.shouldReloadTileOptions; + } + }); + await source.updateData(diff, true); + const result = source.shouldReloadTile(tile, shouldReloadTileOptions); - const result = source.shouldReloadTile(tile, source._getShouldReloadTileOptions(diff)); - - expect(result).toBe(true); - }); - - test('returns true when updated feature new geometry intersects tile bounds', () => { - // Feature update with new geometry at 0,0 should intersect with tile 0/0/0 - const tile = getMockTile(0, 0, 0, [{id: 0}]); - const diff: GeoJSONSourceDiff = { - update: [{ - id: 0, - newGeometry: {type: 'Point', coordinates: [0, 0]} - }] - }; - - const result = source.shouldReloadTile(tile, source._getShouldReloadTileOptions(diff)); - - expect(result).toBe(true); + expect(result).toBe(false); }); - test('returns false when diff has no changes affecting the tile', () => { + test('returns false when diff has no changes affecting the tile', async () => { // Feature far away from tile bounds - const tile = getMockTile(10, 500, 500, [{id: 0}]); + const tile = new Tile(new OverscaledTileID(10, 0, 10, 500, 500), source.tileSize); + tile.state = 'loaded'; const diff: GeoJSONSourceDiff = { add: [{ id: 1, @@ -1030,90 +1005,61 @@ describe('GeoJSONSource.shoudReloadTile', () => { geometry: {type: 'Point', coordinates: [-170, -80]} }] }; - - const result = source.shouldReloadTile(tile, source._getShouldReloadTileOptions(diff)); - - expect(result).toBe(false); - }); - - test('returns false when diff is empty', () => { - const tile = getMockTile(0, 0, 0, []); - const diff: GeoJSONSourceDiff = {}; - - const result = source.shouldReloadTile(tile, source._getShouldReloadTileOptions(diff)); + let shouldReloadTileOptions: GeoJSONSourceShouldReloadTileOptions = undefined; + source.on('data', (e) => { + if (e.shouldReloadTileOptions) { + shouldReloadTileOptions = e.shouldReloadTileOptions; + } + }); + await source.updateData(diff, true); + const result = source.shouldReloadTile(tile, shouldReloadTileOptions); expect(result).toBe(false); }); - - test('returns false when tile has been unloaded', () => { - const tile = getMockTile(0, 0, 0, []); - tile.latestFeatureIndex = null; - tile.state = 'unloaded'; - + + test('returns false when diff is empty', async () => { + tile.state = 'loaded'; const diff: GeoJSONSourceDiff = {}; - const result = source.shouldReloadTile(tile, source._getShouldReloadTileOptions(diff)); + let shouldReloadTileOptions: GeoJSONSourceShouldReloadTileOptions = undefined; + source.on('data', (e) => { + if (e.shouldReloadTileOptions) { + shouldReloadTileOptions = e.shouldReloadTileOptions; + } + }); + await source.updateData(diff, true); + const result = source.shouldReloadTile(tile, shouldReloadTileOptions); expect(result).toBe(false); }); - - test('returns true when tile is still loading', () => { - const tile = getMockTile(0, 0, 0, []); - tile.latestFeatureIndex = null; - - const diff: GeoJSONSourceDiff = {}; - - const result = source.shouldReloadTile(tile, source._getShouldReloadTileOptions(diff)); - - expect(result).toBe(true); - }); - - test('handles string feature ids', () => { + + test('handles string feature ids and returns no bounds since feature does not exist', async () => { const diff: GeoJSONSourceDiff = {remove: ['abc']}; - const result = source._getShouldReloadTileOptions(diff); - - expect(result).toBe(undefined); - }); - - test('handles promoteId', () => { - source.promoteId = 'id'; - const tile = getMockTile(0, 0, 0, [{id: 0, properties: {id: 'abc'}}]); - const diff: GeoJSONSourceDiff = {remove: ['abc']}; - - const result = source.shouldReloadTile(tile, source._getShouldReloadTileOptions(diff)); + let shouldReloadTileOptions: GeoJSONSourceShouldReloadTileOptions = undefined; + source.on('data', (e) => { + if (e.shouldReloadTileOptions) { + shouldReloadTileOptions = e.shouldReloadTileOptions; + } + }); + await source.updateData(diff, true); - expect(result).toBe(true); + expect(shouldReloadTileOptions.affectedBounds).toHaveLength(0); }); - test('handles cluster', () => { - const diff: GeoJSONSourceDiff = {remove: ['abc']}; - source._options.cluster = true; - - const result = source._getShouldReloadTileOptions(diff); - - expect(result).toBe(undefined); - }); + test('handles cluster', async () => { + const diff: GeoJSONSourceDiff = {remove: [1]}; + source = new GeoJSONSource('id', {data: {}, cluster: true} as GeoJSONSourceOptions, mockDispatcher, undefined); - test('handles features that span the international date line', () => { - const diff: GeoJSONSourceDiff = { - add: [{ - type: 'Feature', - properties: {}, - geometry: { - type: 'LineString', - coordinates: [ - [-185, 10], - [-175, 10] - ], - } - }] - }; + let shouldReloadTileOptions: GeoJSONSourceShouldReloadTileOptions = undefined; + source.on('data', (e) => { + if (e.shouldReloadTileOptions) { + shouldReloadTileOptions = e.shouldReloadTileOptions; + } + }); + await source.updateData(diff, true); - expect(source.shouldReloadTile(getMockTile(5, 1, 15, []), source._getShouldReloadTileOptions(diff))).toBe(false); - expect(source.shouldReloadTile(getMockTile(5, 0, 15, []), source._getShouldReloadTileOptions(diff))).toBe(true); - expect(source.shouldReloadTile(getMockTile(5, 31, 15, []), source._getShouldReloadTileOptions(diff))).toBe(true); + expect(shouldReloadTileOptions).toBeUndefined(); }); -}); - */ \ No newline at end of file +}); \ No newline at end of file diff --git a/src/source/geojson_source.ts b/src/source/geojson_source.ts index ff778eaecb7..8f3974d7b03 100644 --- a/src/source/geojson_source.ts +++ b/src/source/geojson_source.ts @@ -485,10 +485,12 @@ export class GeoJSONSource extends Evented implements Source { * @internal */ shouldReloadTile(tile: Tile, {affectedBounds}: GeoJSONSourceShouldReloadTileOptions) : boolean { - if (!tile.latestFeatureIndex) { - return tile.state !== 'unloaded'; + if (tile.state === 'loading') { + return true; + } + if (tile.state === 'unloaded') { + return false; } - // Update the tile if it WILL NOW contain an updated feature. const {buffer, extent} = this.workerOptions.geojsonVtOptions; const tileBounds = tileIdToLngLatBounds( diff --git a/src/source/geojson_source_diff.test.ts b/src/source/geojson_source_diff.test.ts index a408149cc38..b5833c3af12 100644 --- a/src/source/geojson_source_diff.test.ts +++ b/src/source/geojson_source_diff.test.ts @@ -241,7 +241,7 @@ describe('applySourceDiff', () => { type: 'Feature', geometry: { type: 'Point', - coordinates: [0, 0], + coordinates: [1, 1], }, properties: { promoteId: 'point2' @@ -261,11 +261,12 @@ describe('applySourceDiff', () => { test('adds a feature using the feature id', () => { const updateable = new Map(); - applySourceDiff(updateable, { + const affectedGeometries = applySourceDiff(updateable, { add: [point] }); expect(updateable.size).toBe(1); expect(updateable.has('point')).toBeTruthy(); + expect(affectedGeometries).toStrictEqual([point.geometry]); }); test('adds a feature using the promoteId', () => { @@ -279,32 +280,34 @@ describe('applySourceDiff', () => { test('removes a feature by its id', () => { const updateable = new Map([['point', point], ['point2', point2]]); - applySourceDiff(updateable, { + const affectedGeometries = applySourceDiff(updateable, { remove: ['point2'], }); expect(updateable.size).toBe(1); expect(updateable.has('point2')).toBeFalsy(); + expect(affectedGeometries).toStrictEqual([point2.geometry]); }); test('updates a feature geometry', () => { const updateable = new Map([['point', point]]); - // update -> new geometry - applySourceDiff(updateable, { + const newGeometry: GeoJSON.Point = { + type: 'Point', + coordinates: [1, 0] + }; + const affectedGeometries = applySourceDiff(updateable, { update: [{ id: 'point', - newGeometry: { - type: 'Point', - coordinates: [1, 0] - } + newGeometry: newGeometry, }] }); expect(updateable.size).toBe(1); expect((updateable.get('point')?.geometry as GeoJSON.Point).coordinates[0]).toBe(1); + expect(affectedGeometries).toStrictEqual([point.geometry, newGeometry]); }); test('adds properties', () => { const updateable = new Map([['point', point]]); - applySourceDiff(updateable, { + const affectedGeometries = applySourceDiff(updateable, { update: [{ id: 'point', addOrUpdateProperties: [ @@ -318,11 +321,12 @@ describe('applySourceDiff', () => { expect(Object.keys(properties)).toHaveLength(2); expect(properties.prop).toBe('value'); expect(properties.prop2).toBe('value2'); + expect(affectedGeometries).toStrictEqual([point.geometry]); }); test('updates properties', () => { const updateable = new Map([['point', {...point, properties: {prop: 'value', prop2: 'value2'}}]]); - applySourceDiff(updateable, { + const affectedGeometries = applySourceDiff(updateable, { update: [{ id: 'point', addOrUpdateProperties: [ @@ -335,11 +339,12 @@ describe('applySourceDiff', () => { expect(Object.keys(properties2)).toHaveLength(2); expect(properties2.prop).toBe('value'); expect(properties2.prop2).toBe('value3'); + expect(affectedGeometries).toStrictEqual([point.geometry]); }); test('removes properties', () => { const updateable = new Map([['point', {...point, properties: {prop: 'value', prop2: 'value2'}}]]); - applySourceDiff(updateable, { + const affectedGeometries = applySourceDiff(updateable, { update: [{ id: 'point', removeProperties: ['prop2'] @@ -349,11 +354,12 @@ describe('applySourceDiff', () => { const properties3 = updateable.get('point')?.properties!; expect(Object.keys(properties3)).toHaveLength(1); expect(properties3.prop).toBe('value'); + expect(affectedGeometries).toStrictEqual([point.geometry]); }); test('removes all properties', () => { const updateable = new Map([['point', {...point, properties: {prop: 'value', prop2: 'value2'}}]]); - applySourceDiff(updateable, { + const affectedGeometries = applySourceDiff(updateable, { update: [{ id: 'point', removeAllProperties: true, @@ -361,6 +367,7 @@ describe('applySourceDiff', () => { }); expect(updateable.size).toBe(1); expect(Object.keys(updateable.get('point')?.properties!)).toHaveLength(0); + expect(affectedGeometries).toStrictEqual([point.geometry]); }); test('adds a feature with properties, removes the feature, then adds it back with different geometry and properties', () => { From d13c69ac192ddc7d00f71c8ebf2e451e36cd0ced Mon Sep 17 00:00:00 2001 From: HarelM Date: Tue, 2 Dec 2025 15:55:05 +0200 Subject: [PATCH 10/15] Remove loaded and move it to beforeEach --- src/source/geojson_source.test.ts | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/source/geojson_source.test.ts b/src/source/geojson_source.test.ts index b9906902a82..2147c20a7cb 100644 --- a/src/source/geojson_source.test.ts +++ b/src/source/geojson_source.test.ts @@ -925,9 +925,11 @@ describe('GeoJSONSource.shoudReloadTile', () => { beforeEach(() => { source = new GeoJSONSource('id', {data: {}} as GeoJSONSourceOptions, mockDispatcher, undefined); tile = new Tile(new OverscaledTileID(0, 0, 0, 0, 0), source.tileSize); + tile.state = 'loaded'; }); test('returns true when tile is still loading', () => { + tile.state = "loading"; const result = source.shouldReloadTile(tile, {} as GeoJSONSourceShouldReloadTileOptions); expect(result).toBe(true); @@ -942,7 +944,6 @@ describe('GeoJSONSource.shoudReloadTile', () => { }); test('fires undefined when diff.removeAll is true', async () => { - tile.state = 'loaded'; const diff: GeoJSONSourceDiff = {removeAll: true}; let shouldReloadTileOptions: GeoJSONSourceShouldReloadTileOptions = undefined; @@ -957,7 +958,6 @@ describe('GeoJSONSource.shoudReloadTile', () => { }); test('returns true when tile contains a feature that is being updated', async () => { - tile.state = 'loaded'; const diff: GeoJSONSourceDiff = { update: [{ id: 0, @@ -979,7 +979,6 @@ describe('GeoJSONSource.shoudReloadTile', () => { }); test('returns false when tile contains a feature that is being removed but was never added', async () => { - tile.state = 'loaded'; const diff: GeoJSONSourceDiff = {remove: [0]}; let shouldReloadTileOptions: GeoJSONSourceShouldReloadTileOptions = undefined; source.on('data', (e) => { @@ -1018,7 +1017,6 @@ describe('GeoJSONSource.shoudReloadTile', () => { }); test('returns false when diff is empty', async () => { - tile.state = 'loaded'; const diff: GeoJSONSourceDiff = {}; let shouldReloadTileOptions: GeoJSONSourceShouldReloadTileOptions = undefined; From 574e03d9f9276baac61cb189c3f34dacfc6a6606 Mon Sep 17 00:00:00 2001 From: HarelM Date: Tue, 2 Dec 2025 15:55:38 +0200 Subject: [PATCH 11/15] Remove white space --- src/source/geojson_source.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/source/geojson_source.test.ts b/src/source/geojson_source.test.ts index 2147c20a7cb..c7a70651a20 100644 --- a/src/source/geojson_source.test.ts +++ b/src/source/geojson_source.test.ts @@ -1015,7 +1015,7 @@ describe('GeoJSONSource.shoudReloadTile', () => { expect(result).toBe(false); }); - + test('returns false when diff is empty', async () => { const diff: GeoJSONSourceDiff = {}; From 2d676694bca3d8d4aa1a8362dee2bc425d14df43 Mon Sep 17 00:00:00 2001 From: HarelM Date: Tue, 2 Dec 2025 15:58:53 +0200 Subject: [PATCH 12/15] Update changelog --- CHANGELOG.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index de9e6b50e55..fd566e55ba8 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -12,6 +12,7 @@ - Fix `queryTerrainElevation` to use higher zoom level tiles when possible ([#6791](https://github.com/maplibre/maplibre-gl-js/issues/6791)) (by [@HarelM](https://github.com/HarelM)) - Fix unwanted movement when moving a pitched terrain map at high latitudes; fix freezes when moving a pitched and rotated terrain map at low zoom ([#6775](https://github.com/maplibre/maplibre-gl-js/pull/6775)) (by [@larsmaxfield](https://github.com/larsmaxfield)) - Fix issue with `static` modifier as part of mlt package ([#6796](https://github.com/maplibre/maplibre-gl-js/pull/6796)) (by [@HarelM](https://github.com/HarelM)) +- Fix GeoJSONSource tile reloading when updating data ([#6800](https://github.com/maplibre/maplibre-gl-js/pull/6800)) (by [@HarelM](https://github.com/HarelM)) - _...Add new stuff here..._ ## 5.13.0 From 6f9fd250cecd35112bd21b0d825cc5bf6a134a9d Mon Sep 17 00:00:00 2001 From: HarelM Date: Tue, 2 Dec 2025 16:47:46 +0200 Subject: [PATCH 13/15] Fix lint --- src/source/geojson_source.test.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/source/geojson_source.test.ts b/src/source/geojson_source.test.ts index c7a70651a20..92474d54370 100644 --- a/src/source/geojson_source.test.ts +++ b/src/source/geojson_source.test.ts @@ -929,7 +929,7 @@ describe('GeoJSONSource.shoudReloadTile', () => { }); test('returns true when tile is still loading', () => { - tile.state = "loading"; + tile.state = 'loading'; const result = source.shouldReloadTile(tile, {} as GeoJSONSourceShouldReloadTileOptions); expect(result).toBe(true); From 755ec5f41ff0f9bd86e475e8cd1cb13e1585d4bd Mon Sep 17 00:00:00 2001 From: HarelM Date: Thu, 4 Dec 2025 09:21:39 +0200 Subject: [PATCH 14/15] Code review changes --- src/source/geojson_source.ts | 23 ++++++++++++----------- src/source/geojson_source_diff.test.ts | 10 ++++++++++ src/source/geojson_source_diff.ts | 6 ++++-- 3 files changed, 26 insertions(+), 13 deletions(-) diff --git a/src/source/geojson_source.ts b/src/source/geojson_source.ts index 8f3974d7b03..443fd762f64 100644 --- a/src/source/geojson_source.ts +++ b/src/source/geojson_source.ts @@ -417,10 +417,7 @@ export class GeoJSONSource extends Evented implements Source { this.fire(new Event('dataabort', {dataType: 'source'})); return; } - let shouldReloadTileOptions: GeoJSONSourceShouldReloadTileOptions | undefined; - if (diff) { - shouldReloadTileOptions = this._applyDiff(diff); - } + const affectedGeometries = this._applyDiffIfNeeded(diff); let resourceTiming: PerformanceResourceTiming[] = null; if (result.resourceTiming && result.resourceTiming[this.id]) { @@ -435,7 +432,7 @@ export class GeoJSONSource extends Evented implements Source { // although GeoJSON sources contain no metadata, we fire this event to let the TileManager // know its ok to start requesting tiles. this.fire(new Event('data', {...eventData, sourceDataType: 'metadata'})); - this.fire(new Event('data', {...eventData, sourceDataType: 'content', shouldReloadTileOptions})); + this.fire(new Event('data', {...eventData, sourceDataType: 'content', shouldReloadTileOptions: affectedGeometries !== undefined ? {affectedGeometries} : undefined})); } catch (err) { this._isUpdatingWorker = false; if (this._removed) { @@ -451,7 +448,15 @@ export class GeoJSONSource extends Evented implements Source { } } - private _applyDiff(diff: GeoJSONSourceDiff): GeoJSONSourceShouldReloadTileOptions { + /** + * Apply a diff to the source data and return the bounds of the affected geometries. + * @param diff The diff to apply. + * @returns The bounds of the affected geometries, undefined if the diff is not applicable or all geometries are affected. + */ + private _applyDiffIfNeeded(diff: GeoJSONSourceDiff): LngLatBounds[] | undefined { + if (!diff) { + return undefined; + } const promoteId = typeof this.promoteId === 'string' ? this.promoteId : undefined; // Lazily convert `this._data` to updateable if it's not already @@ -471,13 +476,9 @@ export class GeoJSONSource extends Evented implements Source { return undefined; } - const affectedBounds = affectedGeometries + return affectedGeometries .filter(Boolean) .map(g => getGeoJSONBounds(g)); - - return { - affectedBounds, - }; } /** diff --git a/src/source/geojson_source_diff.test.ts b/src/source/geojson_source_diff.test.ts index b5833c3af12..5a09f776ed1 100644 --- a/src/source/geojson_source_diff.test.ts +++ b/src/source/geojson_source_diff.test.ts @@ -288,6 +288,16 @@ describe('applySourceDiff', () => { expect(affectedGeometries).toStrictEqual([point2.geometry]); }); + test('removes a feature by its id and dont return undefined geometries', () => { + const updateable = new Map([['point', point], ['point2', point2]]); + const affectedGeometries = applySourceDiff(updateable, { + remove: ['point2', 'point3'], + }); + expect(updateable.size).toBe(1); + expect(updateable.has('point2')).toBeFalsy(); + expect(affectedGeometries).toStrictEqual([point2.geometry]); + }); + test('updates a feature geometry', () => { const updateable = new Map([['point', point]]); const newGeometry: GeoJSON.Point = { diff --git a/src/source/geojson_source_diff.ts b/src/source/geojson_source_diff.ts index 94d102272ac..ce1aabd8b2c 100644 --- a/src/source/geojson_source_diff.ts +++ b/src/source/geojson_source_diff.ts @@ -134,8 +134,10 @@ export function applySourceDiff(updateable: Map Date: Thu, 4 Dec 2025 09:32:32 +0200 Subject: [PATCH 15/15] Fix tests, fix lint --- src/source/geojson_source.ts | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/src/source/geojson_source.ts b/src/source/geojson_source.ts index 443fd762f64..de12fcf08e5 100644 --- a/src/source/geojson_source.ts +++ b/src/source/geojson_source.ts @@ -417,7 +417,7 @@ export class GeoJSONSource extends Evented implements Source { this.fire(new Event('dataabort', {dataType: 'source'})); return; } - const affectedGeometries = this._applyDiffIfNeeded(diff); + const affectedBounds = this._applyDiffIfNeeded(diff); let resourceTiming: PerformanceResourceTiming[] = null; if (result.resourceTiming && result.resourceTiming[this.id]) { @@ -432,7 +432,7 @@ export class GeoJSONSource extends Evented implements Source { // although GeoJSON sources contain no metadata, we fire this event to let the TileManager // know its ok to start requesting tiles. this.fire(new Event('data', {...eventData, sourceDataType: 'metadata'})); - this.fire(new Event('data', {...eventData, sourceDataType: 'content', shouldReloadTileOptions: affectedGeometries !== undefined ? {affectedGeometries} : undefined})); + this.fire(new Event('data', {...eventData, sourceDataType: 'content', shouldReloadTileOptions: affectedBounds !== undefined ? {affectedBounds} : undefined})); } catch (err) { this._isUpdatingWorker = false; if (this._removed) { @@ -450,7 +450,7 @@ export class GeoJSONSource extends Evented implements Source { /** * Apply a diff to the source data and return the bounds of the affected geometries. - * @param diff The diff to apply. + * @param diff - The diff to apply. * @returns The bounds of the affected geometries, undefined if the diff is not applicable or all geometries are affected. */ private _applyDiffIfNeeded(diff: GeoJSONSourceDiff): LngLatBounds[] | undefined {