maplibre / maplibre-gl-js

MapLibre GL JS - Interactive vector tile maps in the browser
https://maplibre.org/maplibre-gl-js/docs/
Other
6.27k stars 683 forks source link

setPaintProperty does not work correctly when terrain is used. #3001

Open boeckMt opened 1 year ago

boeckMt commented 1 year ago

maplibre-gl-js version: 3.3.0

browser: Chrome

Steps to Trigger Behavior

  1. add a terrain and a fill-layer to the map
  2. use map.setPaintProperty to update fill-opacity

Link to Demonstration

codesandbox: https://codesandbox.io/s/maplibre-terrain-set-paint-test-9jpnwl?file=/main.js:2463-2475

Use the input slider to update fill-opacity from the water layer between 0 to 1. Sometimes higher values are not painted correctly, and paint is only applied when the map is moved.

If the terrain is deactivated by the Terrain Control, it will work as expected.

Expected Behavior

If map.setPaintProperty is used it should update the paint correctly even if a terrain is used.

Actual Behavior

It looks like `map.setPaintProperty' is only applied every second time, if a terrain is used.

HarelM commented 1 year ago

Thanks for taking the time to report this issue! Seems like the change to the propery is not as smooth without and with terrain.

boeckMt commented 1 year ago

https://github.com/maplibre/maplibre-gl-js/blob/95d6f43ed396555ca710445cb9cb25a378c84cc4/src/ui/map.ts#L2574

Other things I found out:

Unfortunately I don't have enough knowledge in this library, but for me it looks like updating the terrain does take some time and then map.style._changed is set to false and layer changes are canceled.

prozessor13 commented 1 year ago

Hi, because renderToTexture is expensive, there exists a texture-pool of previous rendered tiles. I think this caching logic is responsible for your problem. There exists a callback, which frees all cached textures here https://github.com/maplibre/maplibre-gl-js/blob/main/src/ui/map.ts#L1997. May this logic is not sufficient for setPaintProperty. You can try to call freeRtt(), after setPaintProperty by hand. Would be great if you can test this, and tell us if this helps.

prozessor13 commented 1 year ago

Ah, and i have another idea: setPaintProperty changes the values with a transition of several hundred milliseconds, but i think the style event is sent immediatly. So the render-to-texture cache is setted up to early. Either a rerender-to-texture has to be done multiple times during transition, or because of performance, transitions should be disabled in terrain. dont know, something to test.

HarelM commented 1 year ago

Interesting, thanks for this input @prozessor13! At the very least I think the final state should be reflected, i.e. there shouldn't be a discrepancy between the last value that was set and how the map looks. Even if we throttle the events so that we don't RTT all the time, we should honor the last value. My two cents I guess.

boeckMt commented 1 year ago

Hi, because renderToTexture is expensive, there exists a texture-pool of previous rendered tiles. I think this caching logic is responsible for your problem. There exists a callback, which frees all cached textures here https://github.com/maplibre/maplibre-gl-js/blob/main/src/ui/map.ts#L1997. May this logic is not sufficient for setPaintProperty. You can try to call freeRtt(), after setPaintProperty by hand. Would be great if you can test this, and tell us if this helps.

Thank you for the information @prozessor13! Calling map.terrain.sourceCache.freeRtt() immediately after setPaintProperty unfortunately does not work. If it is called several hundred milliseconds later it works, but you have to call it at a specific time, otherwise it will not work.

https://codesandbox.io/s/maplibre-terrain-set-paint-test-9jpnwl?file=/main.js:2497-2536


For me, the only differences between style.setPaintProperty and style.setLayoutProperty (which works as expected) are the check if it requiresRelayout

https://github.com/maplibre/maplibre-gl-js/blob/3e6aa0dbd6794831fba689b6ec07a1091c1b8e59/src/style/style.ts#L1115-L1118


The following are the codes for setPaintProperty

https://github.com/maplibre/maplibre-gl-js/blob/3e6aa0dbd6794831fba689b6ec07a1091c1b8e59/src/style/style.ts#L1104-L1122

https://github.com/maplibre/maplibre-gl-js/blob/3e6aa0dbd6794831fba689b6ec07a1091c1b8e59/src/style/style_layer.ts#L150-L178

Is this._transitionablePaint.setValue(name, value); the transition you mentioned? I cannot find a timing that is set.


The following are the codes for setLayoutProperty

https://github.com/maplibre/maplibre-gl-js/blob/3e6aa0dbd6794831fba689b6ec07a1091c1b8e59/src/style/style.ts#L1073-L1086

https://github.com/maplibre/maplibre-gl-js/blob/3e6aa0dbd6794831fba689b6ec07a1091c1b8e59/src/style/style_layer.ts#L126-L140

boeckMt commented 1 year ago

Another thing I found out: It looks like layer.setPaintProperty is not called for the first time, I tested this by overriding

layer._handleSpecialPaintPropertyUpdate = () => {
      console.log('test');
    };

https://codesandbox.io/s/maplibre-terrain-set-paint-test-9jpnwl?file=/main.js:2577-2627

prozessor13 commented 1 year ago

When i remember right, the Map._render method has in internal check if currently transitions are ongoing. If yes, the _render method calles itself recursively. I think this is may a good place to hook into. Either freeRtt every time, if performacne allowes it, or after transition is finished make another final render-call with freeRtt.

prozessor13 commented 1 year ago

and i think the transitions are completely not connected to any setPaintProperty logic. Updating any properties only trigger the transitions.

prozessor13 commented 1 year ago

https://github.com/maplibre/maplibre-gl-js/blob/main/src/ui/map.ts#L3199

And sorry, currently i do not have time to test things, i can only thinking a bit around the problem..

craigsc commented 4 months ago

Hit this issue myself today on 4.1.3. Same thing as described by @boeckMt, using setPaintProperty when terrain is unset works as expected but when terrain is set it seems to have some timing issue with using previously cached tiles which results in the wrong final values being displayed for fill-opacity until I zoom in/out to refresh the cache

I did find a workaround for anyone else that hits this that seems to fix it for 99% of the cases in my own usage: call this following the call to setPaintProperty:

          setTimeout(() => {
            map.terrain.sourceCache.sourceCache.reload();
          }, 200);