Fix issues with setting sky to undefined (#4587)

* Fix issues with undefining a style * Update changelog * Fix test name
maplibre · Aug 22, 2024 · 76a01c1 · 76a01c1
1 parent c8b97c9
commit 76a01c1
Show file tree

Hide file tree

Showing 8 changed files with 57 additions and 16 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -12,7 +12,8 @@
 - Fix 3D map freezing when camera is adjusted against map bounds. ([#4537](https://github.com/maplibre/maplibre-gl-js/issues/4537))
 - Fix `getStyle()` to return a clone so the object cannot be internally changed ([#4488](https://github.com/maplibre/maplibre-gl-js/issues/4488))
 - Prefer local glyph rendering for all CJKV characters, not just those in the CJK Unified Ideographs, Hiragana, Katakana, and Hangul Syllables blocks. ([#4560](https://github.com/maplibre/maplibre-gl-js/pull/4560)))
-- - _...Add new stuff here..._
+- Fix issues with setting sky to undefined ([#4587](https://github.com/maplibre/maplibre-gl-js/pull/4587)))
+- _...Add new stuff here..._
 
 ## 4.5.2
 

diff --git a/src/render/painter.ts b/src/render/painter.ts
@@ -409,7 +409,7 @@ export class Painter {
         this.clearStencil();
 
         // draw sky first to not overwrite symbols
-        if (this.style.stylesheet?.sky) drawSky(this, this.style.sky);
+        if (this.style.sky) drawSky(this, this.style.sky);
 
         this._showOverdrawInspector = options.showOverdrawInspector;
         this.depthRangeFor3D = [0, 1 - ((style._order.length + 2) * this.numSublayers * this.depthEpsilon)];

diff --git a/src/style/sky.ts b/src/style/sky.ts
@@ -55,11 +55,20 @@ export class Sky extends Evented {
         this._transitionable = new Transitionable(properties);
         this.setSky(sky);
         this._transitioning = this._transitionable.untransitioned();
+        this.recalculate(new EvaluationParameters(0));
     }
 
     setSky(sky?: SkySpecification, options: StyleSetterOptions = {}) {
         if (this._validate(validateSky, sky, options)) return;
 
+        if (!sky) {
+            sky = {
+                'sky-color': 'transparent',
+                'horizon-color': 'transparent',
+                'fog-color': 'transparent',
+            };
+        }
+
         for (const name in sky) {
             const value = sky[name];
             if (name.endsWith(TRANSITION_SUFFIX)) {

diff --git a/src/style/style.test.ts b/src/style/style.test.ts
@@ -2592,6 +2592,24 @@ describe('Style#serialize', () => {
         expect(style.getSky()).toBeUndefined();
     });
 
+    test('sky should be defined even after setting it to undefined and back', async () => {
+        const sky: SkySpecification = {
+            'horizon-fog-blend': 0.5,
+            'fog-color': '#fff'
+        };
+        const styleJson = createStyleJSON({sky});
+        const style = new Style(getStubMap());
+        style.loadJSON(styleJson);
+
+        await style.once('style.load');
+        style.setSky(undefined);
+        expect(style.serialize().sky).toBeUndefined();
+        style.setSky(sky);
+        expect(style.serialize().sky).toBeDefined();
+        style.setSky(undefined);
+        expect(style.serialize().sky).toBeUndefined();
+    });
+
     test('do not include sky property after removing sky from the map', async () => {
         const sky: SkySpecification = {
             'horizon-fog-blend': 0.5,

diff --git a/src/style/style.ts b/src/style/style.ts
@@ -1496,17 +1496,20 @@ export class Style extends Evented {
     }
 
     setSky(skyOptions?: SkySpecification, options: StyleSetterOptions = {}) {
-        const sky = this.sky.getSky();
+        const sky = this.getSky();
         let update = false;
-        if (!skyOptions) {
-            if (sky) {
-                update = true;
-            }
-        }
-        for (const key in skyOptions) {
-            if (!deepEqual(skyOptions[key], sky[key])) {
-                update = true;
-                break;
+        if (!skyOptions && !sky) return;
+
+        if (skyOptions && !sky) {
+            update = true;
+        } else if (!skyOptions && sky) {
+            update = true;
+        } else {
+            for (const key in skyOptions) {
+                if (!deepEqual(skyOptions[key], sky[key])) {
+                    update = true;
+                    break;
+                }
             }
         }
         if (!update) return;

diff --git a/src/ui/map.ts b/src/ui/map.ts
@@ -2665,7 +2665,7 @@ export class Map extends Camera {
     /**
      * Loads sky and fog defined by {@link SkySpecification} onto the map.
      * Note: The fog only shows when using the terrain 3D feature.
-     * @param sky - Sky properties to set. Must conform to the [MapLibre Style Specification](https://maplibre.org/maplibre-gl-js-docs/style-spec/#sky).
+     * @param sky - Sky properties to set. Must conform to the [MapLibre Style Specification](https://maplibre.org/maplibre-style-spec/sky/).
      * @returns `this`
      * @example
      * ```ts
@@ -2681,7 +2681,11 @@ export class Map extends Camera {
     /**
      * Returns the value of the sky object.
      *
-     * @returns sky Sky properties of the style.
+     * @returns the sky properties of the style.
+     * @example
+     * ```ts
+     * map.getSky();
+     * ```
      */
     getSky() {
         return this.style.getSky();

diff --git a/src/util/test/util.ts b/src/util/test/util.ts
@@ -172,7 +172,7 @@ export function createStyleSource() {
     } as SourceSpecification;
 }
 
-export function createStyle() {
+export function createStyle(): StyleSpecification {
     return {
         version: 8,
         center: [-73.9749, 40.7736],
@@ -181,5 +181,5 @@ export function createStyle() {
         pitch: 50,
         sources: {},
         layers: []
-    } as StyleSpecification;
+    };
 }
diff --git a/test/examples/sky-with-fog-and-terrain.html b/test/examples/sky-with-fog-and-terrain.html
@@ -25,6 +25,7 @@
 <body>
 <div id="map"></div>
 <table id="listing-group" class="listing-group">
+    <tr><td><label for="sky-enabled">enabled</label></td><td><input type="checkbox" id="sky-enabled" checked/></td></tr>
     <tr><td><label for="sky-color-picker">sky-color</label></td><td><input type="color" id="sky-color-picker" value="#0000ff" style="width: 100%"/></td></tr>
     <tr><td><label for="horizon-color-picker">horizon-color</label></td><td><input type="color" id="horizon-color-picker" value="#00ff00" style="width: 100%"/></td></tr>
     <tr><td><label for="fog-color-picker">fog-color</label></td><td><input type="color" id="fog-color-picker" value="#ff0000" style="width: 100%"/></td></tr>
@@ -34,6 +35,10 @@
 </table>
 <script>
     function setSkyFromUi() {
+        if (!document.getElementById('sky-enabled').checked) {
+            map.setSky(undefined);
+            return;
+        }
         map.setSky({
             'sky-color': document.getElementById('sky-color-picker').value,
             'sky-horizon-blend': +document.getElementById('sky-horizon-blend-slider').value,
@@ -116,6 +121,7 @@
         document.getElementById('sky-horizon-blend-slider').addEventListener('change', setSkyFromUi);
         document.getElementById('horizon-fog-blend-slider').addEventListener('change', setSkyFromUi);
         document.getElementById('fog-ground-blend-slider').addEventListener('change', setSkyFromUi);
+        document.getElementById('sky-enabled').addEventListener('change', setSkyFromUi);
         setSkyFromUi();
     });
 </script>