[GLJS-1527] Simplify update placement method (internal-8745)

GitOrigin-RevId: 3b22b3f77cc0952f674d9c07f2865c52dda42494

Author: underoot

src/style/pauseable_placement.ts

@@ -72,21 +72,34 @@ class PauseablePlacement {
     _forceFullPlacement: boolean;
     _showCollisionBoxes: boolean;
     _inProgressLayer: LayerPlacement | null | undefined;
+    _fadeDuration: number;
 
-    constructor(transform: Transform, order: Array<string>,
-                forceFullPlacement: boolean,
-                showCollisionBoxes: boolean,
-                fadeDuration: number,
-                crossSourceCollisions: boolean,
-                prevPlacement?: Placement,
-                fogState?: FogState | null,
-                buildingIndex?: BuildingIndex | null
-    ) {
+    startNewPlacement(
+        transform: Transform,
+        order: Array<string>,
+        showCollisionBoxes: boolean,
+        fadeDuration: number,
+        crossSourceCollisions: boolean,
+        prevPlacement?: Placement,
+        fogState?: FogState | null,
+        buildingIndex?: BuildingIndex | null
+    ): PauseablePlacement {
         this.placement = new Placement(transform, fadeDuration, crossSourceCollisions, prevPlacement, fogState, buildingIndex);
         this._currentPlacementIndex = order.length - 1;
-        this._forceFullPlacement = forceFullPlacement;
+        this._forceFullPlacement = false;
         this._showCollisionBoxes = showCollisionBoxes;
+        this._fadeDuration = fadeDuration;
         this._done = false;
+        this._inProgressLayer = null;
+        return this;
+    }
+
+    requestFullPlacement(): void {
+        this._forceFullPlacement = true;
+    }
+
+    isFullPlacementRequested(): boolean {
+        return this._forceFullPlacement;
     }
 
     isDone(): boolean {
@@ -98,7 +111,7 @@ class PauseablePlacement {
 
         const shouldPausePlacement = () => {
             const elapsedTime = browser.now() - startTime;
-            return this._forceFullPlacement ? false : elapsedTime > 2;
+            return this.isFullPlacementRequested() || this._fadeDuration === 0 ? false : elapsedTime > 2;
         };
 
         while (this._currentPlacementIndex >= 0) {
@@ -143,6 +156,7 @@ class PauseablePlacement {
             this._currentPlacementIndex--;
         }
         PerformanceUtils.recordPlacementTime(browser.now() - startTime);
+        this._forceFullPlacement = false;
         this._done = true;
     }
 

src/style/style.ts

@@ -73,7 +73,6 @@ import type {PropertyValidatorOptions} from '../style-spec/validate/validate_pro
 import type Tile from '../source/tile';
 import type GeoJSONSource from '../source/geojson_source';
 import type {ReplacementSource} from "../../3d-style/source/replacement_source";
-import type Painter from '../render/painter';
 import type SymbolStyleLayer from '../style/style_layer/symbol_style_layer';
 import type {
     ColorThemeSpecification,
@@ -341,7 +340,6 @@ class Style extends Evented<MapEvents> {
     _rtlTextPluginCallback: (state: {pluginStatus: string; pluginURL: string | null | undefined}) => void;
     _changes: StyleChanges;
     _optionsChanged: boolean;
-    _layerOrderChanged: boolean;
     _availableImages: ImageId[];
     _availableModels: StyleModelMap;
     _markersNeedUpdate: boolean;
@@ -2717,7 +2715,7 @@ class Style extends Evented<MapEvents> {
         }
 
         this._order.splice(index, 0, id);
-        this._layerOrderChanged = true;
+        this._handleLayerOrderChange();
         this._layers[id] = layer;
 
         const sourceCache = this.getOwnLayerSourceCache(layer);
@@ -2794,7 +2792,7 @@ class Style extends Evented<MapEvents> {
         this._order.splice(newIndex, 0, id);
 
         this._changes.setDirty();
-        this._layerOrderChanged = true;
+        this._handleLayerOrderChange();
 
         this.mergeLayers();
     }
@@ -2821,7 +2819,7 @@ class Style extends Evented<MapEvents> {
         delete this._layers[id];
 
         this._changes.setDirty();
-        this._layerOrderChanged = true;
+        this._handleLayerOrderChange();
 
         this._configDependentLayers.delete(layer.fqid);
         this._indoorDependentLayers.delete(layer.fqid);
@@ -4117,17 +4115,40 @@ class Style extends Evented<MapEvents> {
         }
     }
 
+    _handleLayerOrderChange() {
+        this._requestFullLabelPlacement();
+        this.fire(new Event('neworder'));
+    }
+
+    _requestFullLabelPlacement() {
+        if (!this.pauseablePlacement) {
+            this.pauseablePlacement = new PauseablePlacement();
+        }
+
+        // Anything that changes our "in progress" layer and tile indices requires us
+        // to start over. When we start over, we do a full placement instead of incremental
+        // to prevent starvation.
+        // We need to restart placement to keep layer indices in sync.
+        this.pauseablePlacement.requestFullPlacement();
+    }
+
+    _setLabelPlacementStale() {
+        if (this.placement) {
+            this.placement.setStale();
+        }
+    }
+
     _updatePlacement(
-        painter: Painter,
         transform: Transform,
         showCollisionBoxes: boolean,
         fadeDuration: number,
         crossSourceCollisions: boolean,
         replacementSource: ReplacementSource,
-        forceFullPlacement: boolean = false,
-    ): {
-        needsRerender: boolean;
-    } {
+    ): boolean {
+        if (!this.pauseablePlacement) {
+            this.pauseablePlacement = new PauseablePlacement();
+        }
+
         let symbolBucketsChanged = false;
         let placementCommitted = false;
 
@@ -4156,16 +4177,6 @@ class Style extends Evented<MapEvents> {
         }
         this.crossTileSymbolIndex.pruneUnusedLayers(this._mergedOrder);
 
-        // Anything that changes our "in progress" layer and tile indices requires us
-        // to start over. When we start over, we do a full placement instead of incremental
-        // to prevent starvation.
-        // We need to restart placement to keep layer indices in sync.
-        forceFullPlacement = forceFullPlacement || this._layerOrderChanged;
-
-        if (this._layerOrderChanged) {
-            this.fire(new Event('neworder'));
-        }
-
         const transformChanged = Boolean(this.placement && !transform.equals(this.placement.transform));
         const replacementSourceChanged = Boolean(this.placement && ((this.placement.lastReplacementSourceUpdateTime !== 0 && !replacementSource) || this.placement.lastReplacementSourceUpdateTime !== replacementSource.updateTime));
 
@@ -4175,19 +4186,12 @@ class Style extends Evented<MapEvents> {
         // we will do expensive full placements on every frame.
         const fullFrameUpdateRequired = (transformChanged || replacementSourceChanged || symbolBucketsChanged || (this.placement && this.placement.isStale())) && fadeDuration === 0;
 
-        if (forceFullPlacement || !this.pauseablePlacement || fullFrameUpdateRequired || (fadeDuration !== 0 && this.pauseablePlacement.isDone() && !this.placement.stillRecent(browser.now(), transform.zoom))) {
+        if (this.pauseablePlacement.isFullPlacementRequested() || !this.pauseablePlacement.placement || fullFrameUpdateRequired || (fadeDuration !== 0 && this.pauseablePlacement.isDone() && !this.placement.stillRecent(browser.now(), transform.zoom))) {
             const fogState = this.fog && transform.projection.supportsFog ? this.fog.state : null;
-            this.pauseablePlacement = new PauseablePlacement(transform, this._mergedOrder, forceFullPlacement || fadeDuration === 0, showCollisionBoxes, fadeDuration, crossSourceCollisions, this.placement, fogState, this._buildingIndex);
-            this._layerOrderChanged = false;
+            this.pauseablePlacement = this.pauseablePlacement.startNewPlacement(transform, this._mergedOrder, showCollisionBoxes, fadeDuration, crossSourceCollisions, this.placement, fogState, this._buildingIndex);
         }
 
-        if (this.pauseablePlacement.isDone()) {
-            // the last placement finished running, but the next one hasn’t
-            // started yet because of the `stillRecent` check immediately
-            // above, so mark it stale to ensure that we request another
-            // render frame
-            this.placement.setStale();
-        } else {
+        if (!this.pauseablePlacement.isDone()) {
             this.pauseablePlacement.continuePlacement(this._mergedOrder, this._mergedLayers, layerTiles, layerTilesInYOrder, this.map.painter.scaleFactor);
 
             if (this.pauseablePlacement.isDone()) {
@@ -4216,8 +4220,7 @@ class Style extends Evented<MapEvents> {
         }
 
         // needsRender is false when we have just finished a placement that didn't change the visibility of any symbols
-        const needsRerender = !this.pauseablePlacement.isDone() || this.placement.hasTransitions(browser.now());
-        return {needsRerender};
+        return !this.pauseablePlacement.isDone() || this.placement.isStale() || this.placement.hasTransitions(browser.now());
     }
 
     _releaseSymbolFadeTiles() {

src/symbol/placement.ts

@@ -1393,8 +1393,7 @@ export class Placement {
     }
 
     hasTransitions(now: number): boolean {
-        return this.stale ||
-            now - this.lastPlacementChangeTime < this.fadeDuration;
+        return now - this.lastPlacementChangeTime < this.fadeDuration;
     }
 
     stillRecent(now: number, zoom: number): boolean {

src/ui/map.ts

@@ -466,7 +466,6 @@ export class Map extends Camera {
     _styleDirty?: boolean;
     _sourcesDirty?: boolean;
     _placementDirty?: boolean;
-    _scaleFactorChanged?: boolean;
     _loaded: boolean;
     _fullyLoaded: boolean; // accounts for placement finishing as well
     _trackResize: boolean;
@@ -638,7 +637,6 @@ export class Map extends Camera {
         this._containerHeight = 0;
         this._showParseStatus = true;
         this._precompilePrograms = options.precompilePrograms;
-        this._scaleFactorChanged = false;
 
         this._averageElevationLastSampledAt = -Infinity;
         this._averageElevationExaggeration = 0;
@@ -1239,7 +1237,9 @@ export class Map extends Camera {
         this.painter.scaleFactor = scaleFactor;
         DevTools.refresh();
 
-        this._scaleFactorChanged = true;
+        if (this.style) {
+            this.style._setLabelPlacementStale();
+        }
 
         this.style._updateFilteredLayers((layer) => layer.type === 'symbol');
         this._update(true);
@@ -4257,6 +4257,12 @@ export class Map extends Camera {
         this.painter = new Painter(gl, this._contextCreateOptions, this.transform, this._scaleFactor, this._worldview);
         this.on('data', (event) => {
             if (event.dataType === 'source') {
+                const elevationSource = this.transform.elevation ? this.transform.elevation._source() : null;
+                // Force a new label placement when a DEM source was updated,
+                // to ensure that labels are placed according to the updated terrain.
+                if (elevationSource && event.sourceCacheId === elevationSource.id && this.style) {
+                    this.style._setLabelPlacementStale();
+                }
                 this.painter.setTileLoadedFlag(true);
             }
         });
@@ -4488,12 +4494,8 @@ export class Map extends Camera {
             averageElevationChanged = this._updateAverageElevation(frameStartTime);
         }
 
-        const updatePlacementResult = this.style && this.style._updatePlacement(this.painter, this.painter.transform, this.showCollisionBoxes, fadeDuration, this._crossSourceCollisions, this.painter.replacementSource, this._scaleFactorChanged);
-        if (this._scaleFactorChanged) {
-            this._scaleFactorChanged = false;
-        }
-        if (updatePlacementResult) {
-            this._placementDirty = updatePlacementResult.needsRerender;
+        if (this.style) {
+            this._placementDirty = this.style._updatePlacement(this.painter.transform, this.showCollisionBoxes, fadeDuration, this._crossSourceCollisions, this.painter.replacementSource);
         }
 
         // Actually draw

test/unit/style/style.test.ts

@@ -2004,7 +2004,7 @@ test('Style defers expensive methods', async () => {
     style.update({});
 
     // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
-    expect(style.fire.mock.calls[0][0].type).toEqual('data');
+    expect(style.fire.mock.calls[0][0].type).toEqual('neworder');
 
     // called per source
     expect(style.reloadSource).toHaveBeenCalledTimes(2);
@@ -2797,3 +2797,63 @@ test('Occlusion ordering & draped layers', async () => {
         makeFQID('symbol-occlusion-3'),
     ]);
 });
+
+describe('Style#_updatePlacement', () => {
+    test('does not re-trigger placement on repaint with fadeDuration: 0 when nothing changed', async () => {
+        const map = new StubMap();
+        // Mock painter with scaleFactor (required by _updatePlacement)
+        // @ts-expect-error - painter is not part of StubMap but required for _updatePlacement
+        map.painter = {scaleFactor: 1};
+
+        // Mock replacement source with updateTime (required by _updatePlacement)
+        const replacementSource = {updateTime: 0};
+
+        const style = new Style(map);
+        style.loadJSON({
+            "version": 8,
+            "sources": {
+                "geojson": {
+                    "type": "geojson",
+                    "data": {"type": "FeatureCollection", "features": []}
+                }
+            },
+            "layers": [{
+                "id": "symbol",
+                "type": "symbol",
+                "source": "geojson"
+            }]
+        });
+
+        await waitFor(style, 'style.load');
+
+        const tr = map.transform;
+        tr.resize(512, 512);
+
+        // Compile the style layers before calling _updatePlacement
+        style.update({zoom: tr.zoom, fadeDuration: 0});
+
+        // First call to _updatePlacement - initializes placement
+        style._updatePlacement(tr, false, 0, false, replacementSource);
+
+        // Placement should be done after first call with fadeDuration: 0
+        expect(style.pauseablePlacement.isDone()).toBeTruthy();
+
+        // Spy on placement methods AFTER initial placement
+        const startNewPlacementSpy = vi.spyOn(style.pauseablePlacement, 'startNewPlacement');
+        const continuePlacementSpy = vi.spyOn(style.pauseablePlacement, 'continuePlacement');
+
+        // Second call to _updatePlacement - should NOT trigger new placement
+        style._updatePlacement(tr, false, 0, false, replacementSource);
+
+        // Assert placement methods were NOT called
+        expect(startNewPlacementSpy).not.toHaveBeenCalled();
+        expect(continuePlacementSpy).not.toHaveBeenCalled();
+
+        // Third call to _updatePlacement - verify consistent behavior
+        style._updatePlacement(tr, false, 0, false, replacementSource);
+
+        // Assert placement methods STILL were not called (verifies it's not a one-time skip)
+        expect(startNewPlacementSpy).not.toHaveBeenCalled();
+        expect(continuePlacementSpy).not.toHaveBeenCalled();
+    });
+});

test/unit/terrain/terrain.test.ts

@@ -290,7 +290,6 @@ describe('Elevation', () => {
         test('disable terrain', async () => {
             // eslint-disable-next-line @typescript-eslint/no-unsafe-member-access
             expect(map.painter.terrain).toBeTruthy();
-            await waitFor(map, "idle");
             // eslint-disable-next-line @typescript-eslint/no-unsafe-call, @typescript-eslint/no-unsafe-member-access
             map.setTerrain(null);
             await waitFor(map, "render");