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

Improve sky atmosphere #8866

Merged
merged 21 commits into from
May 25, 2020
Merged

Improve sky atmosphere #8866

merged 21 commits into from
May 25, 2020

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented May 17, 2020

This PR tweaks a few things about how the sky atmosphere is rendered so that there's less artifacts. These artifacts usually show up as white/black (and sometimes red/yellow) patches below the horizon point.

red-yellow

The two main changes are

  • Compute atmosphere per-fragment instead of per-vertex. This creates a smooth transition line rather than the stair-step effect caused by large discontinuities in per-vertex rayleigh color and mie color.
  • Compute the ellipsoid intersection in the shader and use the second intersection point as the start of the atmosphere ray. This limits how far a ray traverses so the colors don't get too extreme. This fix works for both per-fragment and per-vertex modes and makes the sky atmosphere look much better when the globe is translucent. Credit for this idea goes to @IanLilleyT.

Both the per-fragment and per-vertex code paths look nearly identical when the globe is rendered. The per-vertex path still has some stair-step artifacts that become more obvious when globe tiles are loading in or the globe is hidden. The code defaults to per-fragment shading because it looks good all the time. The code always uses per-fragment shading when the globe is translucent in globe-alpha.

There is small runtime cost. On my 2015 Macbook, with viewer.browserRecommendedResolution = false there is a 2-3 fps between per-fragment and per-vertex. Performance critical applications can set viewer.scene.skyAtmosphere.perFragmentAtmosphere to false.

Comparison of the different modes:

master
after

per-vertex
optional

per-fragment
before

master

lighting-bad

per-fragment

lighting-better

(Note the camera is momentarily considered underground while tiles are loading which causes the globe to look dark, this is a separate problem)

Sandcastle for testing

@lilleyse lilleyse requested a review from IanLilleyT May 17, 2020 23:33
@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.
  • Works (or fails gracefully) in IE11.

@lilleyse lilleyse mentioned this pull request May 18, 2020
13 tasks
Copy link
Contributor

@IanLilleyT IanLilleyT left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good. I liked how most of the lighting calculations have moved to their own file. Main issue is the edge artifact (screenshots inside). This is a tricky problem because you don't want the atmosphere to get ultra bright like it used to, but you also don't want it to fade to the more neutral blue color too early either, which is what's happening in those screenshots.

#ifdef PER_FRAGMENT_ATMOSPHERE
vec3 mieColor;
vec3 rayleighColor;
calculateMieColorAndRayleighColor(v_outerPositionWC, 1.0, mieColor, rayleighColor);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

At the horizon it can show a thin sliver where the atmosphere starts to transition back to blue. This is because the globe triangles dip slightly below the pixel-perfect ray ellipsoid intersection. Changing the 1.0 to 1.003 fixes the problem

Sandcastle

Screen Shot 2020-05-18 at 8 37 30 PM

There is also a small difference when zoomed out due to lower detail triangles which might force the constant to be even bigger:

Master:
Screen Shot 2020-05-18 at 9 50 53 PM

Branch:
Screen Shot 2020-05-18 at 9 50 46 PM

Source/Scene/SkyAtmosphere.js Outdated Show resolved Hide resolved
Source/Scene/SkyAtmosphere.js Show resolved Hide resolved
Source/Shaders/SkyAtmosphereFS.glsl Outdated Show resolved Hide resolved
Source/Shaders/SkyAtmosphereFS.glsl Outdated Show resolved Hide resolved
Source/Shaders/SkyAtmosphereCommon.glsl Outdated Show resolved Hide resolved
Source/Shaders/SkyAtmosphereCommon.glsl Outdated Show resolved Hide resolved
@lilleyse
Copy link
Contributor Author

@IanLilleyT updated

@lilleyse
Copy link
Contributor Author

lilleyse commented May 20, 2020

Got per-vertex rendering to look a lot better, almost as good as per-fragment. It's good enough that I'm switching back to per-vertex by default and not taking the performance hit.

The trick was to align the ellipsoid geometry so it always faces the same direction as the camera. So wherever you're looking the tessellation should look the same.

Another thing that helps is 1409f1d adds a smoother gradient falloff. I'm still deciding whether this is better than doing the original approach.

per-vertex
1
per-fragment
2
master
master

Sandcastle

@IanLilleyT IanLilleyT self-requested a review May 20, 2020 14:57
Source/Scene/SkyAtmosphere.js Outdated Show resolved Hide resolved
Source/Shaders/SkyAtmosphereCommon.glsl Outdated Show resolved Hide resolved
@lilleyse
Copy link
Contributor Author

It's possible the results are system dependent. The early discard check if (rgb.b > 1000000.0) is supposed to throw away those pixels but the value might need to be tweaked, or the approach reconsidered.

@lilleyse
Copy link
Contributor Author

While digging deep into the atmosphere code I noticed that the sky atmosphere gets yellow-er the further you are from the equator which has always bothered me and doesn't really physically make sense. The atmosphere start position now takes into account the camera's height from the ellipsoid rather than a straight up length(positionWC) to produce the same blueness wherever you are on the globe.

Somewhere in Alaska

master
Untitled3
this branch
Untitled4

Somewhere in the Amazon near the equator

master
Untitled1
this branch
Untitled2

@lilleyse lilleyse force-pushed the sky-atmosphere-per-fragment branch from b023803 to 42d08d5 Compare May 21, 2020 22:08
@lilleyse
Copy link
Contributor Author

Also comparisons with globe.enableLighting = true. Pretty much the same except that the noon sky is full blue.

master
sunset2

this branch
sunset1

@lilleyse lilleyse force-pushed the sky-atmosphere-per-fragment branch from 109da35 to 1234027 Compare May 22, 2020 03:21
@lilleyse
Copy link
Contributor Author

@IanLilleyT updated. This also includes a fix to a globe lighting / ground atmosphere crash in master.

Source/Scene/SkyAtmosphere.js Outdated Show resolved Hide resolved
@IanLilleyT
Copy link
Contributor

IanLilleyT commented May 25, 2020

One (future) idea for fixing artifacts when the view is pointing through the ellipsoid and the globe isn't visible is to find a tangent line to the ellipsoid from the camera's position and run the atmosphere calculations relative to that. This will (hopefully) make the horizon color consistent throughout the center of the earth and will let use remove some hacks like clamping strength, fading to alpha, etc.

The tangent line could come from intersecting a plane containing the camera view and up vectors against the ellipsoid, giving an ellipse which will have only two tangent lines, and pick one of the two tangent lines based on which is closer to the view direction.

Not sure how this should work if the camera is below the ellipsoid...

@IanLilleyT IanLilleyT merged commit 565d6eb into master May 25, 2020
@IanLilleyT
Copy link
Contributor

Looks good, thanks @lilleyse !

@IanLilleyT IanLilleyT deleted the sky-atmosphere-per-fragment branch May 25, 2020 15:53
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.

3 participants