Markers update on terrain change #10985
Conversation
SnailBones
changed the title
| @@ -100,7 +100,7 @@ | |||
| }); | |||
|
|
|||
| document.getElementById('terrain-checkbox').onclick = function() { | |||
| map.setTerrain(this.checked ? {"source": "mapbox-dem"} : null); | |||
| map.setTerrain(this.checked ? {"source": "mapbox-dem", "exaggeration": 10} : null); | |||
There was a problem hiding this comment.
Higher terrain exaggeration makes the bug more visible.
| @@ -1380,12 +1380,31 @@ test('Marker interaction and raycast', (t) => { | |||
| const bottomLngLat = tr.pointLocation3D(new Point(terrainTop.x, tr.height)); | |||
| // Raycast returns distance to closer point evaluates to occluded marker. | |||
| t.stub(tr, 'pointLocation3D').returns(bottomLngLat); | |||
| setTimeout(() => { | |||
There was a problem hiding this comment.
setTimeout in a test seems hacky
src/ui/map.js
Outdated
| @@ -2764,6 +2764,10 @@ class Map extends Camera { | |||
| this.painter._updateFog(this.style); | |||
| this._updateTerrain(); // Terrain DEM source updates here and skips update in style._updateSources. | |||
| this.style._updateSources(this.transform); | |||
| // Update positions of markers on enabling/disabling terrain | |||
| for (const marker of this._markers) { | |||
There was a problem hiding this comment.
TBH I don't immediately see why this is the correct location at which to update judiciously in order to fix the bug, but it looks reasonable, and if it fixes the issue, then it seems correct.
There was a problem hiding this comment.
could you put these three lines into _forceMarkerUpdate() function so that its easier to reason about and use in other cases as they come up?
There was a problem hiding this comment.
@arindam1993 Done!
@rreusser The update needs to happen after _updateSources is called, since the markers derive their position from map.transform which has no knowledge of terrain until that function is called.
In #10985 we removed setTimeout in test, which caused behaviour impacted by workload of host machine and becomes execution order unpredictable[1], because marker style modification uses Timers API, but rendering calls setImmediate [1] https://nodejs.org/en/guides/event-loop-timers-and-nexttick#setimmediate-vs-settimeout
Fixes #10982
Launch Checklist
mapbox-gl-jschangelog:<changelog>Fixed markers not updating position on terrain change</changelog>