Add an Atmosphere layer for Globe (#3888) #4020

Pheonor · 2024-04-20T07:50:07Z

Launch Checklist

This PR add a realistic atmosphere layer to the Earth when using a globe projection.
Some options allows to change the zoom level to fade in and out the atmosphere.
By default, the sun position is computed based on current time.
An optionnal date and time allows to set the sun to any position.

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.
Document any changes to public APIs.
Add an entry to CHANGELOG.md under the ## main section.

HarelM · 2024-04-20T11:41:48Z

Can you please add some render tests (and maybe unit test) and resolve conflict before I review this thoroughly?

Pheonor · 2024-04-20T20:50:21Z

I resolve conflict. I will add some tests.

codecov-commenter · 2024-04-20T20:51:20Z

Codecov Report

Attention: Patch coverage is 83.21918% with 49 lines in your changes missing coverage. Please review.

Project coverage is 64.62%. Comparing base (a25074d) to head (31c2487).

Files	Patch %	Lines
src/style/style.ts	30.55%	25 Missing ⚠️
src/geo/projection/mercator.ts	67.85%	9 Missing ⚠️
src/ui/map.ts	20.00%	8 Missing ⚠️
src/style/sky.ts	88.88%	4 Missing and 3 partials ⚠️

Additional details and impacted files

@@             Coverage Diff             @@
##            globe    #4020       +/-   ##
===========================================
- Coverage   87.92%   64.62%   -23.30%     
===========================================
  Files         250      254        +4     
  Lines       34910    35178      +268     
  Branches     2262     1503      -759     
===========================================
- Hits        30694    22735     -7959     
- Misses       3211    11599     +8388     
+ Partials     1005      844      -161

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

kubapelc · 2024-04-21T06:45:00Z

src/geo/projection/globe.ts

+ private _globePosition: vec3 = [0, 0, 0];
+ private _globeRadiusPixels: number = 0.0;
+
+ private _projMatrix: mat4 = mat4.create();


I think the atmosphere math can be done with matrices that were already present.

The _globeProjMatrix matrix assumes the globe is a unit sphere located at (0,0,0), unrotated (so the null island is always at the unit sphere's +Z (0,0,1), see _angularCoordinatesToVector ) and projects it to screen. The already present cameraPosition property is the camera position relative to the unit-sphere globe.

The atmosphere math are performed into camera frame. I need the real globe position into the camera frame and to project each pixel as a ray into this frame also.
This is why I extract the projection matrix and compute the globe position.
The previously computed cameraPosition do not give me the expected value. I will try to reuse some other value to avoid a new computation.

I try to view if I could use cameraPositiondirectly, but this position include the projection effect. I need the globe position in the camera frame without the projection effect.

Maybe this shader could be helpful: https://github.com/maplibre/maplibre-gl-js/blob/globe/src/shaders/fill_extrusion.fragment.glsl

It computes ray-globe intersection for large fill-extrusions, which is very close to what atmopshere needs. The only part the linked shader doesn't do is getting the ray from a pixel, but your approach with the inverse matrix should work fine.

(The varying v_sphere_pos contains the current pixel's position on the globe, relative to the unit sphere, so a point on the surface would have distance 1 from 0,0,0, a point higher up would have greater distance. See the matching vertex shader for more details.)

kubapelc · 2024-04-21T06:56:40Z

src/render/program/projection_program.ts


 export type ProjectionPreludeUniformsType = {
 'u_projection_matrix': UniformMatrix4f;
 'u_projection_tile_mercator_coords': Uniform4f;
 'u_projection_clipping_plane': Uniform4f;
 'u_projection_transition': Uniform1f;
 'u_projection_fallback_matrix': UniformMatrix4f;
+ 'u_globe_position': Uniform3f;
+ 'u_globe_radius': Uniform1f;
+ 'u_inv_proj_matrix': UniformMatrix4f;


I think these new uniforms would be better suited for atmosphereUniformsType. ProjectionData and ProjectionPreludeUniformsType is passed to every single shader, but the uniforms added here are only used by atmosphere.

Ok, I will do like that.

Done.
To be clean, it could be better to centralize all the projection code from transform.tsinto mercator.ts.
But I propose to not do that in the frame of this PR and do this into a refactoring PR.

I would also like to do that, I already started looking at adapting interactions (map panning, etc.) and animations (easeTo, etc.) for globe, changing the transform and refactoring this will definitely be required. As for controls, I would like to have a "globe" and "flat" control scheme (as opposed to "mercator"), since the flat controls would likely get used by all projections except globe. As suggested at the end of this comment: maplibre/maplibre-style-spec#568 (comment)

… inv projection matrix.

Pheonor · 2024-04-21T08:49:07Z

I propose to wait the dicussion on maplibre/maplibre-style-spec#163 before adding some render test with the proposed style.

HarelM · 2024-04-21T19:21:22Z

Sure, that's a good idea.
I hope this discussion won't take too long...

kubapelc · 2024-04-22T08:22:45Z

I finally had the time to play with the example. It looks amazing!

I'm worried about performance though, I also tried to run it on my phone (Snapdragon 636, 1080x2280 pixel screen) and got framerate in the single digits, and the sunlight was missing for some reason.

Edit: also runs at low framerate on Intel HD 630.

Pheonor · 2024-04-22T19:37:27Z

I finally had the time to play with the example. It looks amazing!

I'm worried about performance though, I also tried to run it on my phone (Snapdragon 636, 1080x2280 pixel screen) and got framerate in the single digits, and the sunlight was missing for some reason.

Edit: also runs at low framerate on Intel HD 630.

Thank you :)
Effectively, I need to check if I could tune step parameters to improve performance.

HarelM · 2024-05-30T11:55:50Z

Can this PR be updated?
I think we have finalized (hopefully) the sky definitions, so I think this can be pushed forward, right?
I'll work on merging the style spec changes for both projection and sky so that the style spec package will be up to date with latest definitions.

Pheonor · 2024-05-30T20:56:03Z

Based on the updated atmosphere style spec, sure !
@HarelM How to retrieve the style spec update from maplibre-gl-js ? Do I need to wait a new release ?

Pheonor · 2024-06-03T19:17:12Z

@HarelM I update with the new style definition and I use the sky/atmosphere-blend parameter to decide if the atmosphere should be visible or not. The default value of 0.8 add an atmosphere to any project even if not required. Do you think a default value of 0.0 could be used to ensure no atmosphere is displayed until required ?

HarelM · 2024-06-03T19:44:44Z

Yes, the default should be 0 to keep rendering backward compatible.
We can change that in the next breaking change version, but I prefer to keep things backward compatible right now.

HarelM · 2024-06-03T19:46:19Z

Unless you are talking about the default when globe is "on", which is not the default.
In that case I have no good opinion, and a way to present which are the options would be the best approach, IMHO.

Pheonor · 2024-06-03T19:47:30Z

Yes, the default should be 0 to keep rendering backward compatible. We can change that in the next breaking change version, but I prefer to keep things backward compatible right now.

The default value of 0.8 is define directly into the style spec. Could you made the change please ?

Pheonor · 2024-06-03T19:48:12Z

Unless you are talking about the default when globe is "on", which is not the default. In that case I have no good opinion, and a way to present which are the options would be the best approach, IMHO.

No, I render the atmosphere only when a globe projection is selected.

HarelM · 2024-06-04T06:29:12Z

I see, if we only render the atmosphere when globe projection is selected then there's no problem, and I think atmosphere should be on by default so that it will look good when someone uses a globe (using globe is not the default).
I'm not sure what should be the default, but let's play with it a bit and better understand what the default should be, and then set this default.
Sounds like a good plan?

Pheonor · 2024-06-04T11:33:28Z

I see, if we only render the atmosphere when globe projection is selected then there's no problem, and I think atmosphere should be on by default so that it will look good when someone uses a globe (using globe is not the default). I'm not sure what should be the default, but let's play with it a bit and better understand what the default should be, and then set this default. Sounds like a good plan?

Ok, sounds like a good plan.
I will update existing test with globe to hide the atmosphere and ensure results are good.

Pheonor · 2024-06-05T21:42:59Z

@HarelM I update this PR with new style spec definition for sky and use the light position definition. I also add some tests. I think you could review it now.

HarelM · 2024-06-06T05:38:03Z

@Pheonor did you address the comments made by @kubapelc?
Also, it seems that there are more tests to be added as the coverage seems a bit low...

HarelM · 2024-06-06T05:43:55Z

test/examples/globe-atmosphere.html

+ }
+ },
+ atmosphere: true,
+ atmosphereOptions: {


Is this still relevant?

Effectively, I forget to remove this elements.

HarelM · 2024-06-06T05:44:24Z

test/examples/globe-atmosphere.html

+<html lang="en">
+<head>
+ <title>Display a globe with a satellite map</title>
+ <meta property="og:description" content="Display a globe with a satellite raster baselayer." />


Maybe add a word about the atmosphere here?

HarelM · 2024-06-06T05:47:09Z

src/ui/map.ts

@@ -443,6 +445,7 @@ export class Map extends Camera {
 painter: Painter;
 handlers: HandlerManager;
 projection: Projection;
+ atmosphere: Atmosphere;


Shouldn't this be part of the sky object too?

For the moment, Sky class contains just the style definition, not render elements as mesh that need a Context from the painter.
The Atmosphere class contains the render stuffs.
What is your proposition ? merge the two classes ? instantiate the atmosphere class into sky class ?

Check out the sky PR, I don't see any addition to the map object besides the sky specification API.
I'm not sure what is the correct solution here though.

Ok, I will use the same approach.

HarelM · 2024-06-06T05:55:30Z

I know this is a bit out of scope, but can we incorporate the sky implementation as part of this PR?
I see a lot of similarity, which is not a surprise.
I can push the sky PR to main and then merge it to the globe, but I'm not sure it's worth the complexity, what do you think?

Basic Sky Implementation working with actual codebase #3645

HarelM · 2024-06-06T05:57:06Z

src/geo/projection/projection.ts

+ * @param lngLat - The longitude and latitude coordinate to transform.
+ * @param elev - The elevation coordinate to transform.
+ */
+ transformPosition(lngLat: LngLat, elev: number): vec3;


use elevation instead of elev.

HarelM · 2024-06-06T05:57:53Z

src/geo/projection/projection.ts

+ /**
+ * Globe position in camera frame.
+ */
+ get globePosition(): vec3;


These are not great as this is an interface for all the projections, some of them are not globe.
Can you think of a better way to solve this?

Ok, I propose to use worldCenterPosition and worldSize as more general properties. So, now, the information is available for any projection.

See my previous comment, no new projection properties should be needed for atmosphere, except maybe inverse projection matrix. Lighting can be done in world space instead of camera space, it is common practice in games. In world space, the globe is always a unit sphere placed at 0,0,0. I can suggest the code changes for this if you want.

#4020 (comment)

I change the parameter to be more generic.
I do not understand what it is so important to do the render in world space instead of camera space.
What improvement could be expected ?

Good point, the benefits aren't very significant. It would make the code a bit smaller and maybe help with maintainability, since computing lighting is usually done in world space (or at least I have the impression that is the case).

Ok. In fact, the shader computes atmospheric transmittance. Many implementations do it in camera space. If it is for you, I propose to let the shader like that.

HarelM · 2024-06-06T06:01:10Z

src/render/atmosphere.ts

+import {Projection} from '../geo/projection/projection';
+import {LngLat} from '../geo/lng_lat';
+
+export class Atmosphere {


This class needs more tests to cover its logic.

I simplify this class to remove the Sun position computation from the current time as with the style default value, this code could not be used. I also remove this class to split the code between Sky ans Light Style classes and draw_atmosphere method.

Pheonor · 2024-06-06T20:22:13Z

I know this is a bit out of scope, but can we incorporate the sky implementation as part of this PR? I see a lot of similarity, which is not a surprise. I can push the sky PR to main and then merge it to the globe, but I'm not sure it's worth the complexity, what do you think?
* [Basic Sky Implementation working with actual codebase #3645](https://github.com/maplibre/maplibre-gl-js/pull/3645)

I look at the Sky PR during the implementation of this PR. It could propably be merged after this one also.

Pheonor · 2024-06-06T21:24:10Z

@Pheonor did you address the comments made by @kubapelc? Also, it seems that there are more tests to be added as the coverage seems a bit low...

I tune a little the atmosphere parameters. Render seems style Ok. I hope performance are better.

kubapelc · 2024-06-07T03:27:16Z

I've also tried how well atmosphere runs with the reduced sample counts back when the PR was first opened, and it doesn't help all that much with performance on integrated gpus. I'm not sure such a performance-heavy feature should be on by default. Sorry for mentioning this after you already did the style spec.

Pheonor · 2024-06-08T21:06:55Z

@HarelM I add some Unit Tests to improve the coverage. I hope its better now.

I have some problem to test the -transition option on the atmosphere-blend. As far I understand, the -transition option should be check into the Style Spec. Do you confirm ?

Pheonor added 10 commits April 15, 2024 22:53

Port of PoC atmosphere layer.

c087428

Fix resize for draw_atmosphere

82ec89b

Add some options.

879cf5e

Allow to change sun date and time

1b93979

Fix import warning

b884187

Render atmosphere only when a Globe projection is selected

f5148c3

Add some comments

8b9c78f

Add some comments

c00844d

Change key

5d17da9

Update changelog

46214c9

Pheonor added 2 commits April 20, 2024 22:47

Merge remote-tracking branch 'upstream/globe' into atmosphere

4024626

Fix merge with globe branch

4260dc2

Pheonor added 2 commits April 20, 2024 23:23

Fix documentation and default background color.

1c96fe0

Use black clear color only when atmosphere is on

ffc1530

kubapelc reviewed Apr 21, 2024

View reviewed changes

Use atmosphere uniform for globe position, raidus in camera frame and…

e933a78

… inv projection matrix.

Pheonor added 3 commits May 31, 2024 22:44

Merge branch 'globe' into atmosphere

c2a4ab3

Remove unused project method

23a7af3

Update maplibre-gl-style-spec to 20.3.0 and use sky atmosphere parameter

6e26afc

Pheonor added 5 commits June 4, 2024 22:55

Fix globe tests and use light position as Sun position.

b561d2a

Merge remote-tracking branch 'origin/globe' into atmosphere

aac987e

Avoid type name collisions.

e105a71

Add atmosphere test for globe projection.

9fc5aa5

Update expectedBytes for build test.

5a6e304

HarelM reviewed Jun 6, 2024

View reviewed changes

Fix PR comments.

53c2ef9

Pheonor added 4 commits June 7, 2024 21:57

Update Style test.

bef589a

Remove unused method on projection

31c2487

Add Sky Test.

2f9d2af

Fix style test and add sky unit test.

a90a068

Add an Atmosphere layer for Globe (#3888) #4020

Are you sure you want to change the base?

Add an Atmosphere layer for Globe (#3888) #4020

Conversation

Pheonor commented Apr 20, 2024

Launch Checklist

HarelM commented Apr 20, 2024

Pheonor commented Apr 20, 2024 • edited

codecov-commenter commented Apr 20, 2024 • edited

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pheonor commented Apr 21, 2024

HarelM commented Apr 21, 2024

kubapelc commented Apr 22, 2024 • edited

Pheonor commented Apr 22, 2024

HarelM commented May 30, 2024

Pheonor commented May 30, 2024 • edited

Pheonor commented Jun 3, 2024

HarelM commented Jun 3, 2024

HarelM commented Jun 3, 2024

Pheonor commented Jun 3, 2024

Pheonor commented Jun 3, 2024

HarelM commented Jun 4, 2024

Pheonor commented Jun 4, 2024

Pheonor commented Jun 5, 2024

HarelM commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

HarelM commented Jun 6, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Pheonor commented Jun 6, 2024

Pheonor commented Jun 6, 2024

kubapelc commented Jun 7, 2024

Pheonor commented Jun 8, 2024

Pheonor commented Apr 20, 2024 •

edited

codecov-commenter commented Apr 20, 2024 •

edited

kubapelc commented Apr 22, 2024 •

edited

Pheonor commented May 30, 2024 •

edited