Fix issues with setting sky to undefined #4587

HarelM · 2024-08-22T08:34:32Z

Launch Checklist

This is a fix to issues related to sky definition.

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

Fixes #4517

Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!
Briefly describe the changes in this PR.
Link to related issues.
Include before/after visuals or gifs if this PR includes visual changes.
Write tests for all new functionality.
Add an entry to CHANGELOG.md under the ## main section.

codecov-commenter · 2024-08-22T08:40:03Z

Codecov Report

Attention: Patch coverage is 95.65217% with 1 line in your changes missing coverage. Please review.

Project coverage is 87.63%. Comparing base (42d6847) to head (acfee84).
Report is 14 commits behind head on main.

✅ All tests successful. No failed tests found.

Files	Patch %	Lines
src/style/style.ts	91.66%	0 Missing and 1 partial ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #4587      +/-   ##
==========================================
- Coverage   87.74%   87.63%   -0.12%     
==========================================
  Files         247      247              
  Lines       33494    33504      +10     
  Branches     2414     2289     -125     
==========================================
- Hits        29390    29361      -29     
- Misses       3118     3164      +46     
+ Partials      986      979       -7

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

HarelM · 2024-08-22T08:43:24Z

~~I'm not sure what is the correct way to use animation for removing the sky and fog, but if someone would like to invest time in solving this they are welcome to do so...~~

HarelM · 2024-08-22T09:28:18Z

I've solved the animation issue in last commit.

sbachinin

Code looks ok except this one comment.
As for the overall design, it's difficult for me to judge

sbachinin · 2024-08-22T10:06:26Z

src/style/style.ts

-                break;
+        if (!skyOptions && !sky) return;
+
+        if (skyOptions && !sky) {


I think a little shorter and more readable would be

if (skyOptions && sky) { for... } else { update = true }

But it's not enough - you need to check all 4 options...

In any case, I think it's OK, if you feel strongly about it let me know...

I think it's not a big deal )

HarelM · 2024-08-22T10:18:28Z

Oops... enabled auto-merge... sorry...

This PR contains the following updates: | Package | Type | Update | Change | |---|---|---|---| | [maplibre-gl](https://maplibre.org/) ([source](https://github.com/maplibre/maplibre-gl-js)) | dependencies | minor | [`4.5.2` -> `4.7.0`](https://renovatebot.com/diffs/npm/maplibre-gl/4.5.2/4.7.0) | --- ### Release Notes <details> <summary>maplibre/maplibre-gl-js (maplibre-gl)</summary> ### [`v4.7.0`](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#470) [Compare Source](maplibre/maplibre-gl-js@v4.6.0...v4.7.0) ##### ✨ Features and improvements - Support multiple layers in `map.on`, `map.once` and `map.off` methods ([#4570](maplibre/maplibre-gl-js#4570)) - Ensure GeoJSON cluster sources emit a console warning if `maxzoom` is less than or equal to `clusterMaxZoom` since in this case you may see unexpected results. ([#4604](maplibre/maplibre-gl-js#4604)) ##### 🐞 Bug fixes - Heatmap Fix for 3D terrain ([#4571](maplibre/maplibre-gl-js#4571)) - Fix Map#off to not remove listener with layer(s) registered with Map#once ([#4592](maplibre/maplibre-gl-js#4592)) - Improve types a bit for `addSource` and `getSource` ([#4616](maplibre/maplibre-gl-js#4616)) - Fix the color near the horizon when terrain is enabled without any sky ([#4607](maplibre/maplibre-gl-js#4607)) - Fix bug where `fitBounds` and `cameraForBounds` would not display across the 180th meridian (antimeridian) - Fix white flickering on map resize ([#4158](maplibre/maplibre-gl-js#4158)) - Fixed a performance regression related to symbol placement ([#4599](maplibre/maplibre-gl-js#4599)) - Fix a bug where cloning a Transform instance didn't include the `lngRange`. This caused a bug where using `transformCameraUpdate` caused the `maxBounds` to stop working just for east/west bounds. ([#4625](maplibre/maplibre-gl-js#4625)) ### [`v4.6.0`](https://github.com/maplibre/maplibre-gl-js/blob/HEAD/CHANGELOG.md#460) [Compare Source](maplibre/maplibre-gl-js@v4.5.2...v4.6.0) ##### ✨ Features and improvements - Prefer local glyph rendering for all CJKV characters, not just those in the CJK Unified Ideographs, Hiragana, Katakana, and Hangul Syllables blocks. ([#4560](maplibre/maplibre-gl-js#4560))) ##### 🐞 Bug fixes - Fix right-to-left layout of labels that contain characters in the Arabic Extended-B code block. ([#4536](maplibre/maplibre-gl-js#4536)) - Fix 3D map freezing when camera is adjusted against map bounds. ([#4537](maplibre/maplibre-gl-js#4537)) - Fix `getStyle()` to return a clone so the object cannot be internally changed ([#4488](maplibre/maplibre-gl-js#4488)) - Fix issues with setting sky to `undefined` ([#4587](maplibre/maplibre-gl-js#4587))) </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this PR and you won't be reminded about this update again. --- - [ ] If you want to rebase/retry this PR, check this box --- This PR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate).  Reviewed-on: https://git.tristess.app/alexandresoro/ouca/pulls/52 Reviewed-by: Alexandre Soro <code@soro.dev> Co-authored-by: renovate <renovate@git.tristess.app> Co-committed-by: renovate <renovate@git.tristess.app>

HarelM added 3 commits August 22, 2024 11:12

Fix issues with undefining a style

008c02e

Update changelog

1475e22

Fix test name

3b9c6d9

HarelM requested review from sbachinin and neodescis August 22, 2024 08:37

Merge branch 'main' into fix-sky-undefined

6b5bf83

HarelM enabled auto-merge (squash) August 22, 2024 08:37

This was referenced Aug 22, 2024

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

Closed

Globe: bugfixes: raster layer & projection change #4546

Merged

Add animations when turning off the sky

40bd035

fix failing test

acfee84

sbachinin approved these changes Aug 22, 2024

View reviewed changes

HarelM merged commit 76a01c1 into main Aug 22, 2024
15 checks passed

HarelM deleted the fix-sky-undefined branch August 22, 2024 10:08

HarelM mentioned this pull request Aug 27, 2024

Fix horizon-fog-belnd where sky is not set. #4607

Merged

6 tasks

sagararyal mentioned this pull request Sep 13, 2024

[Snyk] Upgrade maplibre-gl from 4.5.2 to 4.6.0 Plant-for-the-Planet-org/planet-webapp#2230

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issues with setting sky to undefined #4587

Fix issues with setting sky to undefined #4587

HarelM commented Aug 22, 2024 •

edited

Loading

codecov-commenter commented Aug 22, 2024 •

edited

Loading

HarelM commented Aug 22, 2024 •

edited

Loading

HarelM commented Aug 22, 2024

sbachinin left a comment

sbachinin Aug 22, 2024

HarelM Aug 22, 2024

HarelM Aug 22, 2024

sbachinin Aug 22, 2024

HarelM commented Aug 22, 2024

Fix issues with setting sky to undefined #4587

Fix issues with setting sky to undefined #4587

Conversation

HarelM commented Aug 22, 2024 • edited Loading

Launch Checklist

codecov-commenter commented Aug 22, 2024 • edited Loading

Codecov Report

HarelM commented Aug 22, 2024 • edited Loading

HarelM commented Aug 22, 2024

sbachinin left a comment

Choose a reason for hiding this comment

sbachinin Aug 22, 2024

Choose a reason for hiding this comment

HarelM Aug 22, 2024

Choose a reason for hiding this comment

HarelM Aug 22, 2024

Choose a reason for hiding this comment

sbachinin Aug 22, 2024

Choose a reason for hiding this comment

HarelM commented Aug 22, 2024

HarelM commented Aug 22, 2024 •

edited

Loading

codecov-commenter commented Aug 22, 2024 •

edited

Loading

HarelM commented Aug 22, 2024 •

edited

Loading