Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Disable fog (opacity = 0) on globe with terrain elevation #4963

Merged
merged 12 commits into from
Nov 3, 2024

Conversation

birkskyum
Copy link
Member

@birkskyum birkskyum commented Nov 2, 2024

Closes #4928

Towards

As mentioned here, the globe doesn't support Fog "calculateFogMatrix is not supported on globe projection":

warnOnce('calculateFogMatrix is not supported on globe projection.');

Unfortunately, it's still trying to render it, which consistently breaks the map whenever the map is pitched just a bit.

This tiny PR simply set the opacity of the fog/horizon to 0 in globe+terrain mode. When the globe does support fog with terrain, this can easily be removed again:

Fog is still active on Terrain3D without Globe

With Globe - zoom level before mercator transition

Before
Screenshot 2024-11-02 at 17 13 01

After - main branch
Screenshot 2024-11-02 at 17 18 01

After - local branch
Screenshot 2024-11-02 at 17 10 52

With Globe - high zoom, after transition to mercator

Before
Screenshot 2024-11-02 at 18 30 42

After
Screenshot 2024-11-02 at 18 27 40

Without Globe - Fog Still Works

Before/After
Screenshot 2024-11-02 at 17 10 29

Launch Checklist

  • Confirm your changes do not include backports from Mapbox projects (unless with compliant license) - if you are not sure about this, please ask!

@birkskyum birkskyum requested a review from HarelM November 2, 2024 16:26
@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2024

Codecov Report

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

Project coverage is 90.45%. Comparing base (51a2efd) to head (d6899ef).
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
src/render/program/terrain_program.ts 66.66% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4963      +/-   ##
==========================================
- Coverage   90.67%   90.45%   -0.23%     
==========================================
  Files         266      266              
  Lines       38163    38164       +1     
  Branches     3164     3254      +90     
==========================================
- Hits        34606    34521      -85     
- Misses       2611     2650      +39     
- Partials      946      993      +47     

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

@birkskyum birkskyum requested a review from ibesora November 2, 2024 16:31
@birkskyum birkskyum requested a review from HarelM November 2, 2024 18:19
@HarelM
Copy link
Collaborator

HarelM commented Nov 2, 2024

Can you add a unit test to cover this issue somehow?

@birkskyum
Copy link
Member Author

birkskyum commented Nov 2, 2024

@HarelM , I've added a unit tests, and a render test that can be used to help validate a fix to the fog

@birkskyum birkskyum changed the title Disable fog in Globe with Terrain3D Disable (opacity = 0) fog in Globe with Terrain3D Nov 2, 2024
@birkskyum birkskyum changed the title Disable (opacity = 0) fog in Globe with Terrain3D Disable (opacity = 0) fog on Globe with Terrain3D Nov 2, 2024
@birkskyum birkskyum changed the title Disable (opacity = 0) fog on Globe with Terrain3D Disable fog (opacity = 0) on Globe with Terrain3D Nov 2, 2024
@birkskyum birkskyum changed the title Disable fog (opacity = 0) on Globe with Terrain3D Disable fog (opacity = 0) on Globe with Terrain Nov 2, 2024
@birkskyum birkskyum changed the title Disable fog (opacity = 0) on Globe with Terrain Disable fog (opacity = 0) on Globe with Terrain Elevation Nov 2, 2024
@birkskyum birkskyum changed the title Disable fog (opacity = 0) on Globe with Terrain Elevation Disable fog (opacity = 0) on globe with terrain elevation Nov 2, 2024
@birkskyum birkskyum enabled auto-merge (squash) November 2, 2024 23:02
@HarelM
Copy link
Collaborator

HarelM commented Nov 3, 2024

Overall approved, let me know if the file I commented on is needed.

@HarelM
Copy link
Collaborator

HarelM commented Nov 3, 2024

Please add a changelog entry.

@birkskyum birkskyum disabled auto-merge November 3, 2024 13:27
@birkskyum birkskyum enabled auto-merge (squash) November 3, 2024 13:41
@birkskyum birkskyum merged commit 69664b8 into maplibre:main Nov 3, 2024
15 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

The unsupported Fog/Horizon is isn't disabled properly on Globe with Terrain3D
3 participants