Basic Sky Implementation working with actual codebase

[prozessor13]

This PR is a successor of #1713 which is working with the actual maplibre-codebase. Writing Tests is still an open issue for which i currently do not have any time :/

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 93.49315% with 19 lines in your changes are missing coverage. Please review.

Project coverage is 86.23%. Comparing base (3bd5b28) to head (b5b8793).
Report is 26 commits behind head on main.

Files	Patch %	Lines
src/render/mesh.ts	61.90%	8 Missing ⚠️
src/style/sky.ts	91.89%	1 Missing and 5 partials ⚠️
src/style/style.ts	89.74%	3 Missing and 1 partial ⚠️
src/style/light.ts	87.50%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #3645      +/-   ##
==========================================
- Coverage   86.62%   86.23%   -0.39%     
==========================================
  Files         240      244       +4     
  Lines       32262    32527     +265     
  Branches     2167     1988     -179     
==========================================
+ Hits        27946    28051     +105     
- Misses       3376     3542     +166     
+ Partials      940      934       -6     

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

[HarelM]

I've opened the following issue for the spec as I have forgot to add the diff part of the sky to the relevant place in the code.
maplibre/maplibre-style-spec#517

[HarelM]

Thanks for putting this together again!!
@acalcutt do you want to open a PR against this branch to add tests instead of this PR:

• Sky fog test #2974

@ibesora do we want to wait with this PR for GSoC or can we change plans in case of this being completed before the program starts?

[acalcutt]

Thanks for putting this together again!! @acalcutt do you want to open a PR against this branch to add tests instead of this PR:

• Sky fog test #2974

I've never been the best at adding test, but I added a few expect tests in
#3648

[HarelM]

@prozessor13 can I delete sky branch and leave only sky2?

[HarelM]

@acalcutt feel free to fix the unit tests as well while you are at it :-)

[acalcutt]

I'm not sure I understand those errors

Error: TypeError: painter.transform.calculateFogMatrix is not a function

    at drawTerrain (src/render/draw_terrain.ts:88:45)
    at RenderToTexture.renderLayer (src/render/render_to_texture.ts:[18](https://github.com/maplibre/maplibre-gl-js/actions/runs/7742575082/job/21112014472?pr=3645#step:6:19)7:24)
    at Object.<anonymous> (src/render/render_to_texture.test.ts:119:20)

isn't it a function?

maplibre-gl-js/src/geo/transform.ts

Lines 724 to 736 in 32ae12b

	calculateFogMatrix(unwrappedTileID: UnwrappedTileID): mat4 {
	const posMatrixKey = unwrappedTileID.key;
	const cache = this._fogMatrixCache;
	if (cache[posMatrixKey]) {
	return cache[posMatrixKey];
	}
	const fogMatrix = this.calculateTileMatrix(unwrappedTileID);
	mat4.multiply(fogMatrix, this.fogMatrix, fogMatrix);
	cache[posMatrixKey] = new Float32Array(fogMatrix);
	return cache[posMatrixKey];
	}

[HarelM]

The tests are mocking stuff, if the mock doesn't have a function that the test need you'll see this error.
I'll take a look later this week.

[ibesora]

@ibesora do we want to wait with this PR for GSoC or can we change plans in case of this being completed before the program starts?

I don’t think it’s worth waiting for GSoC if this is good to go… but we’ll need to find something else for GSoC. I didn’t realize this was so close to being done

[HarelM]

I've merged the tests fix, and merged main branch.
Let's see what the coverage is reporting and decide how to continue.

[HarelM]

but we’ll need to find something else for GSoC

Increasing coverage is always an option.
Helping the globe effort might be a good idea too.
Supporting multiple coordinate systems is always high up.
Improving performance.
Ping me on slack if you want to brain storm.

[HarelM]

@prozessor13 I moved the gl logic from sky class to draw_sky method, I'm not sure this is a correct approach, but generally speaking I think gl stuff should not be in the sky.ts file.
I also added some tests and fixes to the serialization and setStyle diff implementation.
I'll try and think about other places which might need fixing.
Feel free to review my changes and let me know if they are OK or not.

[HarelM]

I went over the last coverage report just now, I believe most of the relevant code is covered in tests.
There might be some bugs or things that will need to solve later on, but currently I can't think of any thing from the top of my head.
Waiting for @prozessor13 response for some of the comments here before I merge this.
@acalcutt let me know if you have any inputs on this PR.
Once merged I'll delete the sky branch as well as sky2 branch.

[acalcutt]

I liked this in my test page at https://wifidb.net/demo/sky/terrain.html . In practice I don't know if I would use fog, but it is a cool effect.

One thing I was a bit unclear on is that fog color is the second color in the sky gradient.

Right now I am using a css linear-gradient based approach for sky, which as a nice sunset color look.

#map {
	background: linear-gradient(0deg, rgba(36,0,0,1) 45%, rgb(249 209 89) 62%, rgb(81 151 173) 85%, rgb(2 92 179) 95%);
	height: 100%;
	width: 100%;
}

to get that same effect, you have to set 'fog-color' to what you want the of the bottom sky gradient to be.

  "sky": {
    "sky-color": "#199EF3",
    "fog-color": "#FFA500",
    "horizon-blend": 0.6,
    "fog-blend": 1
  }

Its not really an issue, and it is fine, but I just wasn't clear on that until I started testing it.

It does look a bit nicer than my css approach, since the gradient folllows the horizon better.

[acalcutt]

I was thinking though, if i was actually using a "fog-blend" less than 1, that orange color would probably be kind of strange, since a orange fog would cover the surface.

which reminds me, the style spec recommend a green color for fog, which did not look good at all.

  "sky": {
    "sky-color": "#199EF3",
    "fog-color": "#00ff00",
    "horizon-blend": 0.5,
    "fog-blend": 0.6
  }

[HarelM]

The green color is an example, the default is white/gray. I guess the example is green so that you'd know what it looks like and how it affects using a "weird" color.
Feel free to open a PR to change the spec to a better example.
If you think the spec needs to change in order to accommodate for a two color gradient for the sky that is unrelated to the fog color, now would the time to do so, the spec can change as long as there is no implementation, once an implementation exists we'll have a hard time changing it.

[acalcutt]

I was thinking maybe and addition color, maybe named "horizon-color" could be added.

then maybe it is this code where that would be used in place of 'u_fog_color'

maplibre-gl-js/src/shaders/sky.fragment.glsl

Line 11 in 3726ee9

gl_FragColor = mix(u_sky_color, u_fog_color, pow(1.0 - blend / u_horizon_blend, 2.0));

[HarelM]

I would suggest to open/continue the discussion in the style-spec repo.

[HarelM]

@acalcutt what's your take about the horizon color? Should we add something to the sky spec? Do you need my help to push this forward?
I want to move forward with this PR and feature and I think this is the only open issue ATM...

[acalcutt]

In my opinion I think it would be nice to separate the fog color from the sky color, because I don't know that they should always be the same. but I also don't know what that would actually look like. I was more wondering what other people thought here.

I'm not sure I have the time right now to submit a style spec PR.

[prozessor13]

The original code from BAschl BAschl@c788339 had a logic similar line-gradient when drawing the sky. I removed all that code for simplicity. For now i think it has priority to merge this PR, and create new features later on?

[prozessor13]

And that the fog is drawn over the sky has the intention to create an atmospheric effect for the fog. This is in my opinion completely separated from any (yet not existing) sky gradient.

[HarelM]

and create new features later on?

While I don't mind postponing the implementation of sky gradient, I do want to make sure we define it in the spec, or st least make sure it doesn't break the current definition.
If it can be done as an increment and not as a breaking change, I have no objections to wait with it.

[prozessor13]

I simply would suggest an additional sky-gradient property :)

[HarelM]

How would the spec be then?
sky: { ...?

[prozessor13]

It should be easy to implement a second sky color to create a 2-color gradient, and i think i can find time for this the next weeks.

proposal:

• sky-color: same as current definition
• horizon-color: the second sky color at the horizon, default is sky-color
• horizon-sky-blend: a value between 0 and 1. 0 is the horizon, 1 is map-height / 2
• fog-color: same as current definition
• horizon-fog-blend: same as horizon-blend in current definition
• fog-blend: same as current definition

I am really not sure how to name the parameters. please make better suggestions. For sure this proposal is very limited in features, but may sufficient in most scenarios? What do you think?

[wipfli]

Can you explain what horizon-sky-blend does?