diff --git a/src/source/geojson_source.test.ts b/src/source/geojson_source.test.ts index 92474d5437..0a7f0bd786 100644 --- a/src/source/geojson_source.test.ts +++ b/src/source/geojson_source.test.ts @@ -977,7 +977,7 @@ describe('GeoJSONSource.shoudReloadTile', () => { expect(result).toBeTruthy(); }); - + 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; @@ -1030,7 +1030,7 @@ describe('GeoJSONSource.shoudReloadTile', () => { expect(result).toBe(false); }); - + test('handles string feature ids and returns no bounds since feature does not exist', async () => { const diff: GeoJSONSourceDiff = {remove: ['abc']}; @@ -1060,4 +1060,4 @@ describe('GeoJSONSource.shoudReloadTile', () => { expect(shouldReloadTileOptions).toBeUndefined(); }); -}); \ No newline at end of file +}); diff --git a/src/source/geojson_source.ts b/src/source/geojson_source.ts index de12fcf08e..53651ed3e4 100644 --- a/src/source/geojson_source.ts +++ b/src/source/geojson_source.ts @@ -417,7 +417,8 @@ export class GeoJSONSource extends Evented implements Source { this.fire(new Event('dataabort', {dataType: 'source'})); return; } - const affectedBounds = this._applyDiffIfNeeded(diff); + const affectedGeometries = this._applyDiffToSource(diff); + const shouldReloadTileOptions = this._getShouldReloadTileOptions(affectedGeometries); let resourceTiming: PerformanceResourceTiming[] = null; if (result.resourceTiming && result.resourceTiming[this.id]) { @@ -432,7 +433,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: affectedBounds !== undefined ? {affectedBounds} : undefined})); + this.fire(new Event('data', {...eventData, sourceDataType: 'content', shouldReloadTileOptions})); } catch (err) { this._isUpdatingWorker = false; if (this._removed) { @@ -449,14 +450,15 @@ 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. - * @returns The bounds of the affected geometries, undefined if the diff is not applicable or all geometries are affected. + * 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 _applyDiffIfNeeded(diff: GeoJSONSourceDiff): LngLatBounds[] | undefined { + 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 @@ -472,13 +474,26 @@ export class GeoJSONSource extends Evented implements Source { } const affectedGeometries = applySourceDiff(this._data.updateable, diff, promoteId); - if (this._options.cluster || diff.removeAll) { + if (diff.removeAll || this._options.cluster) { return undefined; } - - return affectedGeometries + + return affectedGeometries; + } + + /** + * 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 affectedBounds = affectedGeometries .filter(Boolean) .map(g => getGeoJSONBounds(g)); + + return {affectedBounds}; } /** @@ -492,7 +507,8 @@ export class GeoJSONSource extends Evented implements Source { 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, @@ -503,6 +519,7 @@ export class GeoJSONSource extends Evented implements Source { return true; } } + return false; } diff --git a/src/source/geojson_source_diff.ts b/src/source/geojson_source_diff.ts index ce1aabd8b2..a04286ca71 100644 --- a/src/source/geojson_source_diff.ts +++ b/src/source/geojson_source_diff.ts @@ -106,55 +106,66 @@ 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; + } + + 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 result; + 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 + * @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): GeoJSON.Geometry[] { const affectedGeometries: GeoJSON.Geometry[] = []; + if (diff.removeAll) { updateable.clear(); } else if (diff.remove) { for (const id of diff.remove) { - if (updateable.has(id)) { - affectedGeometries.push(updateable.get(id).geometry); - updateable.delete(id); - } + const existing = updateable.get(id); + if (!existing) continue; + + affectedGeometries.push(existing.geometry); + updateable.delete(id); } } if (diff.add) { for (const feature of diff.add) { const id = getFeatureId(feature, promoteId); - if (id != null) { - affectedGeometries.push(feature.geometry); - updateable.set(id, feature); - } + if (id == null) continue; + + // we are allowed to replace duplicate features with the same id, so need to check for existing + const existing = updateable.get(id); + if (existing) affectedGeometries.push(existing.geometry); + + affectedGeometries.push(feature.geometry); + updateable.set(id, feature); } } if (diff.update) { for (const update of diff.update) { - let feature = updateable.get(update.id); - if (!feature) continue; + const existing = updateable.get(update.id); + if (!existing) continue; const changeGeometry = !!update.newGeometry; @@ -167,7 +178,7 @@ export function applySourceDiff(updateable: Map