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

Added a post processing stage for atmosphere light scattering #10063

Merged
merged 79 commits into from
Apr 25, 2022

Conversation

xtassin
Copy link
Contributor

@xtassin xtassin commented Feb 2, 2022

Also added a Sandcastle example for this new post processing stage

Also added a Sandcastle example for this new post processing stage
@cesium-concierge
Copy link

Thanks for the pull request @xtassin!

  • ✔️ 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.

…then fails with <code>39:9 error Parsing error: The keyword 'let' is reserved</code>
…nColor variable in AtmospherePostProcessing.html is clearly a variable and is reassigned in the call to the Color.lerp method. Eslint insist on making it a const but I, as a human being, disagree, even though I am forced to comply to make the build work.
@ggetz
Copy link
Contributor

ggetz commented Feb 2, 2022

Thanks @xtassin! This looks great! @sanjeetsuhag Would you be able to do a review?

Copy link
Contributor

@sanjeetsuhag sanjeetsuhag left a comment

Choose a reason for hiding this comment

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

Thanks for the contribution, @xtassin. The effect in the Sandcastle looks amazing! I have a few comments: mostly around the documentation and the public API side of things.

Apps/Sandcastle/gallery/AtmospherePostProcessing.html Outdated Show resolved Hide resolved
Apps/Sandcastle/gallery/AtmospherePostProcessing.html Outdated Show resolved Hide resolved
Source/Scene/PostProcessStageLibrary.js Outdated Show resolved Hide resolved
Source/Scene/PostProcessStageLibrary.js Outdated Show resolved Hide resolved
Source/Scene/PostProcessStageLibrary.js Outdated Show resolved Hide resolved
Source/Scene/PostProcessStageLibrary.js Outdated Show resolved Hide resolved
Source/Scene/PostProcessStageLibrary.js Outdated Show resolved Hide resolved
Apps/Sandcastle/gallery/AtmospherePostProcessing.html Outdated Show resolved Hide resolved
@sanjeetsuhag
Copy link
Contributor

sanjeetsuhag commented Feb 18, 2022

I've pushed a pretty significant refactor to this PR - incorporating the concepts from the papers mentioned in the source here into our own SkyAtmosphere and GroundAtmosphere shaders. The SkyAtmosphere is looking better, but the GroundAtmosphere might need some more fine tuning to look correct and clear.

Here's a running list of tasks remaining:

  • Fix SkyAtmosphere translucency.
  • Fix fog calculation in GlobeFS
  • Fix phase function calculation in vertex shader
  • Expose LIGHT_INTENSITY to the public API
  • Investigate why using the value passed in u_radiiAndDynamicAtmosphereColor causes issues with SkyAtmosphere
  • Verify all types of dynamic lighting are still working
  • Add references to papers
  • Investigate polar region sky lighting issues
    image

@lilleyse
Copy link
Contributor

Nice that fixed it for me.

That's all the feedback I have on the visuals. @IanLilleyT can you do a final code review by early next week?

@sanjeetsuhag
Copy link
Contributor

@IanLilleyT Since I'll be out next week, is there a chance you could take a look at this today? It would be great to get this in this release.

@lilleyse
Copy link
Contributor

@sanjeetsuhag Ian is out today but I can take a look

@lilleyse
Copy link
Contributor

I maybe could've dived deeper into the math, but the code looks very clean and well organized. Fog culling is slightly more pronounced because the fog is darker than before, but I don't see a reason to hold this up any longer. The visuals have turned out really nice here.

@lilleyse
Copy link
Contributor

Just wanted to capture a nice screenshot to celebrate this getting merged.

Selection_359

@lilleyse
Copy link
Contributor

Could you update CHANGES.md as well?

@ggetz
Copy link
Contributor

ggetz commented Apr 22, 2022

@sanjeetsuhag Can we swap the dropdown out for two buttons ("Sky Atmosphere" and "Ground Atmosphere") that work like tabs? The dropdown to switch screens is not a typical UI pattern.

image

@sanjeetsuhag
Copy link
Contributor

@ggetz I added a tabbed view

sand

@ggetz
Copy link
Contributor

ggetz commented Apr 22, 2022

Great, thanks @sanjeetsuhag! I'm happy to merge as long as @lilleyse is satisfied.

@lilleyse lilleyse merged commit 4a37d34 into CesiumGS:main Apr 25, 2022
@lilleyse
Copy link
Contributor

@ggetz all good from my side so I merged. Thanks @xtassin and @sanjeetsuhag.

@xtassin
Copy link
Contributor Author

xtassin commented May 5, 2022

Could you please mention @sanjeetsuhag instead of me on the release page https://cesium.com/blog/2022/05/02/cesium-releases-in-may-2022/

I don't believe I deserve any credit for this PR.

Thanks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
No open projects
Status: Issue/PR closed
Development

Successfully merging this pull request may close these issues.

6 participants