search

maplibre / maplibre-gl-js

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

Can't disable all sky rendering when terrain is enabled #4517

Closed UberMouse closed 2 months ago

UberMouse commented 2 months ago

maplibre-gl-js version: 4.5.1

browser: Vivaldi 6.8.3381.48

Steps to Trigger Behavior

  1. Open demonstration
  2. Disable sky with checkbox (this calls map.setSky(undefined))
  3. Notice the sky rendering turns off, but the horizon and fog colouring persists
  4. Disable terrain via the control in the top right, notice that horizon/fog colouring is now gone

Link to Demonstration

https://jsbin.com/hibilakuno/edit?html,output

Expected Behavior

The sky/horizon/fog rendering is disabled when terrain is enabled when calling setSky(undefined) or sky: undefined in a style spec

Actual Behavior

Only sky rendering is disabled when terrain is enabled

HarelM commented 2 months ago

It also seems like when trying to enable again the sky using the checkbox it doesn't bring them back. It might be related to this code, but I'm not sure this is the only part that doesn't take sky as undefined correctly: https://github.com/maplibre/maplibre-gl-js/blob/e6b44367af5c960f1737989bb106fe572263e56c/src/style/sky.ts#L60

Feel free to investigate.

HarelM commented 2 months ago

Should be fixed by #4587 and released shortly after this is merged.

UberMouse commented 2 months ago

Just managed to check this out. Maybe I am misunderstanding something but this does not appear to fix the issue. I've attached two screenshots of the same strip of horizon, one with terrain enabled, one without terrain and otherwise identical. Both have sky disabled (via setStyle with an object that has sky: undefined), but there is a clear difference in hazyness near the horizon in the one with terrain on vs the one with terrain off. I would expect them both to look the same.

Terrain enabled image

Terrain disabled image

HarelM commented 2 months ago

Can you check how this looks before sky was added (version 4.0 for example)

UberMouse commented 2 months ago

Yea here's what it looked like on 4.3.2

Terrain enabled image

Terrain disabled image

No haze

HarelM commented 2 months ago

Can you open a different issue then? Jsbin would be helpful to see the difference between versions and rendering.

UberMouse commented 2 months ago

You want me to open the same issue again? I opened this issue to fix the bug I'm highlighting and it has a JSBin to enable switching the version to compare

HarelM commented 2 months ago

Sorry, my mistake, no need, I reread the original post...

HarelM commented 2 months ago

There were multiple issues in this report I guess...

HarelM commented 2 months ago

Should be fixed in #4607. I'm not sure if the fix should be in the shader itself, but I think this is good enough...