diff --git a/CHANGELOG.md b/CHANGELOG.md index 92d6490fa72..8ff3de5b57e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ - Fix `queryTerrainElevation` to use higher zoom level tiles when possible ([#6791](https://github.com/maplibre/maplibre-gl-js/pull/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)) - `LngLatBounds#intersects` now returns `true` when bounds touch along an edge or at a corner ([#6802](https://github.com/maplibre/maplibre-gl-js/pull/6802)) (by [@lucaswoj](https://github.com/lucaswoj)) ## 5.13.0 diff --git a/src/source/geojson_source.test.ts b/src/source/geojson_source.test.ts index 751d1554b34..b3edd689e48 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 {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 { @@ -655,12 +653,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); @@ -789,18 +783,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', @@ -939,8 +921,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', @@ -953,89 +934,82 @@ 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); + tile.state = 'loaded'; }); - 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', () => { + tile.state = '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 () => { + 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 () => { 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 () => { 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, @@ -1044,88 +1018,60 @@ 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 () => { 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', () => { - 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'}}]); + test('handles string feature ids and returns no bounds since feature does not exist', async () => { 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; + test('handles cluster', async () => { + const diff: GeoJSONSourceDiff = {remove: [1]}; + source = new GeoJSONSource('id', {data: {}, cluster: true} as GeoJSONSourceOptions, mockDispatcher, undefined); - const result = 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(undefined); + expect(shouldReloadTileOptions).toBeUndefined(); }); - test('handles features that span the international date line', () => { - const diff: GeoJSONSourceDiff = { - add: [{ - type: 'Feature', - properties: {}, - geometry: { - type: 'LineString', - coordinates: [ - [-185, 10], - [-175, 10] - ], - } - }] - }; - - 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); - }); }); diff --git a/src/source/geojson_source.ts b/src/source/geojson_source.ts index 6294cf6009f..f54d862fb9d 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'; @@ -45,12 +44,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,9 +417,9 @@ export class GeoJSONSource extends Evented implements Source { this.fire(new Event('dataabort', {dataType: 'source'})); return; } - if (result.data) this._data = {geojson: result.data}; - else if (diff) this._applyDiff(diff); + const affectedGeometries = this._applyDiffToSource(diff); + const shouldReloadTileOptions = this._getShouldReloadTileOptions(affectedGeometries); let resourceTiming: PerformanceResourceTiming[] = null; if (result.resourceTiming && result.resourceTiming[this.id]) { @@ -440,7 +434,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) { @@ -456,7 +450,16 @@ export class GeoJSONSource extends Evented implements Source { } } - private _applyDiff(diff: GeoJSONSourceDiff): void { + /** + * Apply a diff to this source's data and return the affected feature geometries. + * @param diff - The {@link GeoJSONSourceDiff} to apply. + * @returns The affected geometries, or undefined if the diff is not applicable or all geometries are affected. + */ + private _applyDiffToSource(diff: GeoJSONSourceDiff): GeoJSON.Geometry[] | 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 @@ -467,65 +470,52 @@ export class GeoJSONSource extends Evented implements Source { this._data = {updateable: toUpdateable(this._data.geojson, promoteId)}; } - if (this._data.updateable) { - applySourceDiff(this._data.updateable, diff, promoteId); + if (!this._data.updateable) { + return undefined; } - } - - _getShouldReloadTileOptions(diff?: GeoJSONSourceDiff): GeoJSONSourceShouldReloadTileOptions | undefined { - if (this._options.cluster || !diff || diff.removeAll) return undefined; + const affectedGeometries = applySourceDiff(this._data.updateable, diff, promoteId); - const {add = [], update = [], remove = []} = (diff || {}); + if (diff.removeAll || this._options.cluster) { + return undefined; + } - const prevIds = new Set([...update.map(u => u.id), ...remove]); + return affectedGeometries; + } - 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.`); - return undefined; - } - } + /** + * Get options for use in determining whether to reload a tile based on the modified features. + * @param affectedGeometries - The feature geometries affected by the update. + * @returns A {@link GeoJSONSourceShouldReloadTileOptions} object which contains an array of affected bounds caused by the update. + */ + private _getShouldReloadTileOptions(affectedGeometries: GeoJSON.Geometry[]): GeoJSONSourceShouldReloadTileOptions | undefined { + if (!affectedGeometries) return undefined; - const nextBounds = [ - ...update.map(f => f.newGeometry), - ...add.map(f => f.geometry) - ] + const affectedBounds = affectedGeometries .filter(Boolean) .map(g => getGeoJSONBounds(g)); - return { - nextBounds, - prevIds - }; + return {affectedBounds}; } /** * 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 { - if (!tile.latestFeatureIndex) { - return tile.state !== 'unloaded'; + shouldReloadTile(tile: Tile, {affectedBounds}: GeoJSONSourceShouldReloadTileOptions) : boolean { + if (tile.state === 'loading') { + return true; } - - // 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; - } + if (tile.state === 'unloaded') { + return false; } - // Update the tile if it WILL NOW contain an updated feature. + // Update the tile if contained or will 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; } diff --git a/src/source/geojson_source_diff.test.ts b/src/source/geojson_source_diff.test.ts index a408149cc38..5a09f776ed1 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,44 @@ 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('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]]); - // 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 +331,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 +349,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 +364,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 +377,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', () => { diff --git a/src/source/geojson_source_diff.ts b/src/source/geojson_source_diff.ts index aa35ed23a11..a04286ca719 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 */ @@ -107,32 +106,44 @@ export function isUpdateableGeoJSON(data: GeoJSON.GeoJSON | undefined, promoteId } export function toUpdateable(data: UpdateableGeoJSON, promoteId?: string) { - const result = new Map(); + const updateable = new Map(); + + // empty updateable if (data == null || data.type == null) { - // empty result - } else if (data.type === 'Feature') { - result.set(getFeatureId(data, promoteId)!, data); - } else { - for (const feature of data.features) { - result.set(getFeatureId(feature, promoteId)!, feature); - } + return updateable; } - return result; + if (data.type === 'Feature') { + updateable.set(getFeatureId(data, promoteId)!, data); + return updateable; + } + + for (const feature of data.features) { + updateable.set(getFeatureId(feature, promoteId)!, feature); + } + + return updateable; } /** - * Mutates updateable and applies a GeoJSONSourceDiff. Operations are processed in a specific order to ensure predictable behavior: + * Mutates updateable and applies a {@link GeoJSONSourceDiff}. Operations are processed in a specific order to ensure predictable behavior: * 1. Remove operations (removeAll, remove) * 2. Add operations (add) * 3. Update operations (update) + * @returns an array of geometries that were affected by the diff - with the exception of removeAll which does not track any affected geometries. */ -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) { + const existing = updateable.get(id); + if (!existing) continue; + + affectedGeometries.push(existing.geometry); updateable.delete(id); } } @@ -140,16 +151,21 @@ export function applySourceDiff(updateable: Map { - let data; + let data: GeoJSON.GeoJSON; if (params.request) { // Data is loaded from a fetchable URL 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', () => { 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); diff --git a/test/examples/display-a-map.html b/test/examples/display-a-map.html index 016827816ca..32a2c43f142 100644 --- a/test/examples/display-a-map.html +++ b/test/examples/display-a-map.html @@ -24,4 +24,4 @@ }); - \ No newline at end of file +