From b873cd2a0d3953a9c08f3918a49e01a1b26d4b7d Mon Sep 17 00:00:00 2001 From: Masashi Katsumata Date: Sat, 7 Nov 2026 21:56:48 -0800 Subject: [PATCH] Stop resync from rewriting sources and layers that did not change styleReady flipped between true and false about every 0.3 s on a map that was doing nothing (26-52 flips in 10 s on the clearOverlays test page, with the map re-rendering ~100 times in that window). The loop: a styledata arriving while a GeoJSON source is still loading sets styleReady to false, the idle that follows sets it back and resyncs every overlay, and each resync called setData on every source and moveLayer on the marker layers. moveLayer marks the style changed even when the layer stays where it was, so another styledata came while the just-rewritten sources were loading, and so on. Stopping any single controller's resync did not end it, because they all wrote. bringMarkerLayersToFront now moves nothing when the marker layers are already on top (or the order is still the one it last produced), and the layers write through setSourceFeatures, which skips setData when the features are the same objects as the last write to that same source. Renderers build a new feature for every change, so identity means "unchanged"; a style swap creates new source objects, so a lost source is always written again and design changes still restore the overlays. ResyncWritesNothing.test.mjs pins both, plus "a resync with nothing to do touches nothing" and "after a style swap it rebuilds". --- src/circle/MapTilerCircleLayer.ts | 4 +- src/helpers.ts | 72 +++++++++++++- src/marker/MarkerLayer.ts | 4 +- src/polygon/MapTilerPolygonLayer.ts | 6 +- src/polyline/MapTilerPolylineLayer.ts | 4 +- test/ResyncWritesNothing.test.mjs | 129 ++++++++++++++++++++++++++ 6 files changed, 209 insertions(+), 10 deletions(-) create mode 100644 test/ResyncWritesNothing.test.mjs diff --git a/src/circle/MapTilerCircleLayer.ts b/src/circle/MapTilerCircleLayer.ts index f591615..ba6cf35 100644 --- a/src/circle/MapTilerCircleLayer.ts +++ b/src/circle/MapTilerCircleLayer.ts @@ -3,7 +3,7 @@ import type { LayerSpecification, } from 'maplibre-gl'; import { type CircleEntity } from '@mapconductor/js-sdk-core'; -import { bringMarkerLayersToFront, type FeatureCollection, type PolygonFeature } from '../helpers'; +import { bringMarkerLayersToFront, setSourceFeatures, type FeatureCollection, type PolygonFeature } from '../helpers'; import { MapTilerMapViewHolder } from '../MapTilerMapViewHolder'; export type MapTilerActualCircle = PolygonFeature & { id?: string | number }; @@ -51,7 +51,7 @@ export class MapTilerCircleLayer { try { const source = this.holder.map.getSource(this.sourceId) as GeoJSONSource | undefined; if (!source) return false; - source.setData(data); + setSourceFeatures(source, data.features); return true; } catch { return false; diff --git a/src/helpers.ts b/src/helpers.ts index 6528588..c751406 100644 --- a/src/helpers.ts +++ b/src/helpers.ts @@ -42,14 +42,84 @@ export function removeSourceIfExists(map: maplibregl.Map, sourceId: string): voi if (map.getSource(sourceId)) map.removeSource(sourceId); } +const MARKER_LAYER_IDS = ['mc-raster-lyr-mc-marker-tiles', 'mc-marker-layer', 'mc-marker-drag-layer']; + +// The layer order this module last produced, per map. Lets bringMarkerLayersToFront tell +// "nothing moved since I arranged it" apart from "someone added a layer on top". +const arrangedOrders = new WeakMap(); + +function currentLayerOrder(map: maplibregl.Map): string[] | null { + try { + const m = map as unknown as { + getLayersOrder?: () => string[]; + style?: { order?: string[] }; + getStyle?: () => { layers?: Array<{ id: string }> } | undefined; + }; + if (typeof m.getLayersOrder === 'function') return m.getLayersOrder(); + if (Array.isArray(m.style?.order)) return [...m.style!.order!]; + const layers = m.getStyle?.()?.layers; + return layers ? layers.map((layer) => layer.id) : null; + } catch { + return null; + } +} + +/** + * Keep the marker layers above every other layer. + * + * **Moves nothing when they already are.** `moveLayer` marks the style as changed even when + * the layer stays where it was, and every change fires `styledata`. The provider treats a + * `styledata` that arrives while sources are still loading as "style not ready", and the + * `idle` that follows resyncs every overlay — which called this again. The style flipped + * between ready and not ready about every 0.3 s, forever, with nothing on the map changing. + */ export function bringMarkerLayersToFront(map: maplibregl.Map): void { - for (const layerId of ['mc-raster-lyr-mc-marker-tiles', 'mc-marker-layer', 'mc-marker-drag-layer']) { + const order = currentLayerOrder(map); + if (order) { + const present = MARKER_LAYER_IDS.filter((layerId) => order.includes(layerId)); + if (present.length === 0) return; + const tail = order.slice(order.length - present.length); + if (tail.every((layerId, i) => layerId === present[i])) return; + if (arrangedOrders.get(map) === order.join('\n')) return; + } + for (const layerId of MARKER_LAYER_IDS) { try { if (map.getLayer(layerId)) map.moveLayer(layerId); } catch { continue; } } + const arranged = currentLayerOrder(map); + if (arranged) arrangedOrders.set(map, arranged.join('\n')); +} + +// What each GeoJSON source was last given, keyed by the source object itself. A style swap +// replaces the source objects, so a lost source never matches and is written again. +const writtenFeatures = new WeakMap(); + +/** + * `source.setData` that skips the write when the features are the same objects, in the same + * order, as the last write to this very source. + * + * Renderers build a new feature object whenever an overlay changes, so identity is enough to + * say "nothing changed". Skipping matters for the same reason as in + * [bringMarkerLayersToFront]: a redundant setData puts the source back into "loading", the + * next `styledata` then reads as "style not ready", and the resync on `idle` wrote it again. + */ +export function setSourceFeatures( + source: { setData(data: GeoJSONSourceData): unknown }, + features: FeatureCollection['features'], +): void { + const previous = writtenFeatures.get(source); + if ( + previous && + previous.length === features.length && + previous.every((feature, i) => feature === features[i]) + ) { + return; + } + source.setData({ type: 'FeatureCollection', features } as GeoJSONSourceData); + writtenFeatures.set(source, [...features]); } export function metersToPixels(meters: number, latitude: number): number { diff --git a/src/marker/MarkerLayer.ts b/src/marker/MarkerLayer.ts index 3fcf461..d9ee339 100644 --- a/src/marker/MarkerLayer.ts +++ b/src/marker/MarkerLayer.ts @@ -4,7 +4,7 @@ import type { LayerSpecification, } from 'maplibre-gl'; import type { MarkerEntity } from '@mapconductor/js-sdk-core'; -import { bringMarkerLayersToFront, type FeatureCollection, type PointFeature } from '../helpers'; +import { bringMarkerLayersToFront, setSourceFeatures, type FeatureCollection, type PointFeature } from '../helpers'; import { MapTilerMapViewHolder } from '../MapTilerMapViewHolder'; export type MapTilerActualMarker = PointFeature; @@ -93,7 +93,7 @@ export class MarkerLayer { try { const source = this.holder.map.getSource(this.sourceId) as GeoJSONSource | undefined; if (!source) return false; - source.setData(data); + setSourceFeatures(source, data.features); return true; } catch { return false; diff --git a/src/polygon/MapTilerPolygonLayer.ts b/src/polygon/MapTilerPolygonLayer.ts index 3496210..4702fda 100644 --- a/src/polygon/MapTilerPolygonLayer.ts +++ b/src/polygon/MapTilerPolygonLayer.ts @@ -5,7 +5,7 @@ import type { LineFeature, PolygonFeature, } from '../helpers'; -import { bringMarkerLayersToFront } from '../helpers'; +import { bringMarkerLayersToFront, setSourceFeatures } from '../helpers'; import { MapTilerMapViewHolder } from '../MapTilerMapViewHolder'; export interface MapTilerActualPolygon { @@ -69,8 +69,8 @@ export class MapTilerPolygonLayer { const outlineSource = this.holder.map.getSource(this.outlineSourceId) as GeoJSONSource | undefined; if (!fillSource || !outlineSource) return false; - fillSource.setData(fillData); - outlineSource.setData(outlineData); + setSourceFeatures(fillSource, fillData.features); + setSourceFeatures(outlineSource, outlineData.features); return true; } catch { return false; diff --git a/src/polyline/MapTilerPolylineLayer.ts b/src/polyline/MapTilerPolylineLayer.ts index d7b27b3..7b35d51 100644 --- a/src/polyline/MapTilerPolylineLayer.ts +++ b/src/polyline/MapTilerPolylineLayer.ts @@ -1,6 +1,6 @@ import type { GeoJSONSource, LayerSpecification } from 'maplibre-gl'; import type { PolylineEntity } from '@mapconductor/js-sdk-core'; -import { bringMarkerLayersToFront, type FeatureCollection, type LineFeature } from '../helpers'; +import { bringMarkerLayersToFront, setSourceFeatures, type FeatureCollection, type LineFeature } from '../helpers'; import { MapTilerMapViewHolder } from '../MapTilerMapViewHolder'; export type MapTilerActualPolyline = LineFeature[]; @@ -45,7 +45,7 @@ export class MapTilerPolylineLayer { try { const source = this.holder.map.getSource(this.sourceId) as GeoJSONSource | undefined; if (!source) return false; - source.setData(data); + setSourceFeatures(source, data.features); return true; } catch { return false; diff --git a/test/ResyncWritesNothing.test.mjs b/test/ResyncWritesNothing.test.mjs new file mode 100644 index 0000000..31712d8 --- /dev/null +++ b/test/ResyncWritesNothing.test.mjs @@ -0,0 +1,129 @@ +import assert from 'node:assert/strict'; +import { mkdirSync, mkdtempSync, rmSync } from 'node:fs'; +import { after, test } from 'node:test'; +import { pathToFileURL } from 'node:url'; +import { resolve } from 'node:path'; +import { build } from 'tsup'; +import { createGeoPoint, createPolylineState, PolylineManager } from '@mapconductor/js-sdk-core'; + +// The provider marks the style "not ready" on a styledata that arrives while a source is +// still loading, and resyncs every overlay on the idle that follows. A resync that rewrote +// unchanged sources (setData) or re-moved layers that were already on top (moveLayer) fired +// another styledata, so the style flipped between ready and not ready every ~0.3 s forever. +// These tests pin "a resync with nothing to do writes nothing", and that a lost style is +// still rebuilt. +mkdirSync(new URL('../build/', import.meta.url), { recursive: true }); +const outDir = mkdtempSync(new URL('../build/resync-tests-', import.meta.url)); +after(() => rmSync(outDir, { recursive: true, force: true })); +await build({ + entry: { + helpers: 'src/helpers.ts', + controller: 'src/polyline/MapTilerPolylineController.ts', + renderer: 'src/polyline/MapTilerPolylineOverlayRenderer.ts', + layer: 'src/polyline/MapTilerPolylineLayer.ts', + }, + outDir, format: ['esm'], platform: 'node', splitting: false, + dts: false, sourcemap: false, config: false, silent: true, +}); +const { bringMarkerLayersToFront, setSourceFeatures } = await import(pathToFileURL(resolve(outDir, 'helpers.mjs'))); +const { MapTilerPolylineController } = await import(pathToFileURL(resolve(outDir, 'controller.mjs'))); +const { MapTilerPolylineOverlayRenderer } = await import(pathToFileURL(resolve(outDir, 'renderer.mjs'))); +const { MapTilerPolylineLayer } = await import(pathToFileURL(resolve(outDir, 'layer.mjs'))); + +function fakeMap(initialLayers = ['background', 'road-label']) { + const writes = { setData: 0, moveLayer: 0, addSource: 0, addLayer: 0 }; + let sources = new Map(); + let order = [...initialLayers]; + const layers = new Map(initialLayers.map((id) => [id, { id }])); + const map = { + writes, + get order() { return [...order]; }, + getLayersOrder: () => [...order], + style: { get order() { return [...order]; } }, + getStyle: () => ({ layers: order.map((id) => layers.get(id)) }), + getSource: (id) => sources.get(id), + getLayer: (id) => layers.get(id), + addSource(id) { + writes.addSource++; + sources.set(id, { setData() { writes.setData++; } }); + }, + addLayer(layer, before) { + writes.addLayer++; + layers.set(layer.id, layer); + const at = before ? order.indexOf(before) : -1; + if (at >= 0) order.splice(at, 0, layer.id); else order.push(layer.id); + }, + moveLayer(id, before) { + writes.moveLayer++; + order = order.filter((x) => x !== id); + const at = before ? order.indexOf(before) : -1; + if (at >= 0) order.splice(at, 0, id); else order.push(id); + }, + // A style swap drops every source and layer the SDK added. + reloadStyle() { + sources = new Map(); + order = order.filter((id) => !id.startsWith('mc-') && !id.startsWith('polyline')); + for (const id of [...layers.keys()]) if (!order.includes(id)) layers.delete(id); + }, + }; + return map; +} + +test('bringMarkerLayersToFront moves nothing when the marker layers are already on top', () => { + const map = fakeMap(['background', 'mc-marker-layer', 'mc-marker-drag-layer']); + bringMarkerLayersToFront(map); + assert.equal(map.writes.moveLayer, 0); +}); + +test('bringMarkerLayersToFront moves the marker layers once, then leaves them alone', () => { + const map = fakeMap(['background', 'mc-marker-layer', 'mc-marker-drag-layer']); + map.addLayer({ id: 'polyline-layer' }); + bringMarkerLayersToFront(map); + assert.deepEqual(map.order.slice(-2), ['mc-marker-layer', 'mc-marker-drag-layer']); + const moved = map.writes.moveLayer; + assert.ok(moved > 0); + bringMarkerLayersToFront(map); + bringMarkerLayersToFront(map); + assert.equal(map.writes.moveLayer, moved); +}); + +test('setSourceFeatures skips identical features but writes to a replaced source', () => { + let calls = 0; + const source = { setData() { calls++; } }; + const a = { type: 'Feature' }; + const b = { type: 'Feature' }; + setSourceFeatures(source, [a, b]); + setSourceFeatures(source, [a, b]); + assert.equal(calls, 1); + setSourceFeatures(source, [b, a]); + assert.equal(calls, 2, 'a different order is a change'); + setSourceFeatures(source, [b]); + assert.equal(calls, 3, 'a removed feature is a change'); + const replaced = { setData() { calls++; } }; + setSourceFeatures(replaced, [b]); + assert.equal(calls, 4, 'a new source object (style swap) is always written'); +}); + +test('resync with nothing changed writes nothing; after a style swap it rebuilds', async () => { + const map = fakeMap(['background', 'road-label', 'mc-marker-layer', 'mc-marker-drag-layer']); + const holder = { map }; + const layer = new MapTilerPolylineLayer({ holder, canEditStyle: () => true }); + const renderer = new MapTilerPolylineOverlayRenderer({ layer, polylineManager: new PolylineManager(), holder }); + const controller = new MapTilerPolylineController(renderer); + await controller.add([createPolylineState({ + id: 'PL', + points: [createGeoPoint({ latitude: 35, longitude: 139 }), createGeoPoint({ latitude: 36, longitude: 140 })], + })]); + const afterAdd = { ...map.writes }; + assert.ok(afterAdd.setData > 0); + + await controller.resync(); + await controller.resync(); + assert.deepEqual(map.writes, afterAdd, 'a resync with nothing to do must not touch the style'); + + map.reloadStyle(); + await controller.resync(); + assert.ok(map.getSource('polyline-source'), 'the lost source is added again'); + assert.ok(map.writes.setData > afterAdd.setData, 'and its data written again'); + assert.ok(map.order.includes('polyline-layer'), 'and its layer'); +});