Add an Atmosphere layer for Globe (#3888) #4020
Conversation
|
Can you please add some render tests (and maybe unit test) and resolve conflict before I review this thoroughly? |
|
I resolve conflict. I will add some tests. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## globe #4020 +/- ##
==========================================
- Coverage 87.92% 87.78% -0.15%
==========================================
Files 250 254 +4
Lines 34910 35186 +276
Branches 2262 2446 +184
==========================================
+ Hits 30694 30887 +193
- Misses 3211 3297 +86
+ Partials 1005 1002 -3 ☔ View full report in Codecov by Sentry. |
… inv projection matrix.
|
I propose to wait the dicussion on maplibre/maplibre-style-spec#163 before adding some render test with the proposed style. |
|
Sure, that's a good idea. |
|
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 :) |
|
Can this PR be updated? |
|
Based on the updated atmosphere style spec, sure ! |
|
@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 I did not understand why a test is in time out. Could you help me please ? |
|
If it is a render test, then a timeout is usually an error in the json, like a missing comma, wrong property somewhere, etc... No idea how to debug them efficiently,I usually just remove parts of the json until it starts working to find where the error roughly is :D |
|
Render test are ran in the browser, you can open the browser and see what went wrong usually by changing the headless to false. |
|
There are still two open comments above which I don't know if they are resolved or not. Other than that, I believe this is in a good shape. |
|
I might have found a bug, there is a weird black edge around the atmosphere. It is visible when enabling globe on a maptiler map (with no other style changes). This might just be weird default values in the style, or maybe a bug with alpha blending?
Also atmosphere doesn't respect large fill extrusions. But honestly I don't think this needs addressing, at least not right now. I doubt that it would be a frequent use-case and fixing it would probably be hard or cost a lot of performance.
|
|
I'm not sure what two comments @HarelM means, but what I would like to see addressed are default style values and performance. I'll post about performance shortly, but I think atmosphere can be enabled by default from a perfromance point of view, as current state is much better! Style defaultsAs seen in the screenshots in the previous post, current default atmosphere seems to obscure the map by darkening it. I think having a default that obscures vision is not good. I also think that changing the default sun position to make the visible planet be lit would not help, since the map would just be tinted blue instead of gray. I suggest adding a new feature to atmosphere, some kind of blending approach, where the globe center would have atmosphere blended out entirely, thus preserving visibility, but atmosphere would still be present on the horizons, where most of the pretty colors happen. Alternatively the default atmosphere can just be made more subtle in general, which would also solve this problem and probably be much easier. |
|
The two open review comments: I don't mind changing the default, making atmosphere on is probably mainly for raster to mimic reality better, which is not the main usage for maplibre I believe, I might be wrong, I don't really have the relevant statistics. There is a way to change the blending as you zoom in that can be defined in the style using expressions, it's just not the default. |
|
I think you can mark both of the linked comments as resolved. |
|
Done, thanks. |
I could try to merge the Sky PR. I hope it will be Ok. |
Effectively, the render is strange. |
|
Maybe we should do the sky in a different PR. |
|
I've added some last nit picking... overall I believe this is ready to be merged. |
|
As for me, this can be merged, the black edge bug can be resolved with another PR. I also did some more performance eyeballing, so I might as well post the results:
Context: I'm looking at an atmosphere'd globe so that every screen pixel is covered by it and executed the atmosphere pixel shader, so that I get the worst case results. A globe without atmosphere runs at 60 fps (120 on the faster monitor) on all these GPUs. Right now the most powerful available integrated GPUs are roughly comparable to the GTX 1060 in performance (eg. AMD Radeon 780M, and excluding Apple chips). My takeaway: disregard my previous comment about reducing sample counts not working all that well :) At least on current hardware, current status of performance is mostly fine. Performance on a phone looks somewhat suboptimal, but also usable. But atmosphere is not usable for people who pair a big screen with an old iGPU - but I'm not sure how large that group is. But I think it is still worth it to pursue optimization for atmosphere before globe release - both to make it more pleasant on old/slow hardware, and to improve power consumption when using the map on any battery powered device. I also have some gamedev-inspired ideas on how to improve the performance of atmosphere. I can make a PR about this later, or post the ideas here if you want. (I want to focus on other globe functionality first.) |
|
I've merged this. I'm looking into updating the code about the fog, horizon and sky colors and blending. |
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.
CHANGELOG.mdunder the## mainsection.