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

Add ground atmosphere #6877

Merged
merged 39 commits into from
Sep 24, 2018
Merged

Add ground atmosphere #6877

merged 39 commits into from
Sep 24, 2018

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Aug 2, 2018

  • Can be toggled with Globe.showGroundAtmosphere and is enabled by default.
  • The fade in/out for the night side of the globe can be changes with Globe.nightFadeOutDistance and Globe.nightFadeOutDistance.
    image

@cesium-concierge
Copy link

cesium-concierge commented Aug 2, 2018

Thanks for the pull request @bagnell!

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

Reviewers, don't forget to make sure that:

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

I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@hpinkos
Copy link
Contributor

hpinkos commented Aug 3, 2018

How does this play with Globe.enableLighting? It looks like we're also defaulting that to true now and setting it to false seems to have no effect.

@bagnell
Copy link
Contributor Author

bagnell commented Aug 7, 2018

If showGroundAtmosphere is true, then enableLighting shows lighting with terrain vertex normals when available. When showGroundatmosphere is false, it works the same as before.

@pjcozzi
Copy link
Contributor

pjcozzi commented Aug 12, 2018

Code looks good at ultra quick glance!

@bagnell
Copy link
Contributor Author

bagnell commented Aug 20, 2018

The ground atmosphere is now computed per-fragment in GlobeFS for better lighting of the day/night transition. It is only enabled when the camera is above the nightFadeOutDistance, otherwise it is disabled with a #ifdef.

This is ready for review.

@bagnell bagnell mentioned this pull request Sep 6, 2018
4 tasks
@bagnell
Copy link
Contributor Author

bagnell commented Sep 6, 2018

Bump.

@lilleyse
Copy link
Contributor

This is a good example of what @pjcozzi means with looking washed out on different types of imagery.

vector-lighting-off
vector-lighting-on

@lilleyse
Copy link
Contributor

I also agree that a "Globe Lighting" Sandcastle that has toggles for

  • Ground Atmosphere
  • Enable lighting
  • Switch between shading with terrain vertex normals or not
  • Water shading
  • Sliders for controlling the fade in and fade out distances

would be pretty useful to have generally, especially as this change rolls out and people are interested in customizing it. We could just point them to the demo.

@lilleyse
Copy link
Contributor

I tweaked some of the parameters for the fade in - it's a little smoother now and looks a bit better when enableLighting is on. e1005fe

Though setting nightFadeOutDistance didn't seem to make a difference even when set to 0. Is there some hidden minimum?

fade-in

@bagnell
Copy link
Contributor Author

bagnell commented Sep 20, 2018

Though setting nightFadeOutDistance didn't seem to make a difference even when set to 0. Is there some hidden minimum?

The nightFadeOutDistance is the distance until it fades to a fully lit atmosphere. So if you set it less than lightingFadeOutDistance you won't see a difference. Set it to something greater than lightingFadeOutDistance but less than nightFadeInDistance and you'll see what I mean.

@bagnell
Copy link
Contributor Author

bagnell commented Sep 20, 2018

@lilleyse This is ready for another look.

@lilleyse
Copy link
Contributor

Looks really good - the water shading from far out is now much nicer now! I added a few last tweaks. Mainly just upped the specular brightness and changed the night fade in.

There is one bug in the atmosphere demo where clicking the reset button doesn't seem to update the sliders.

Does anyone else want to check this out before we merge? Demo of this branch here:

http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/atmosphere/Apps/Sandcastle/index.html?src=Atmosphere.html

@pjcozzi
Copy link
Contributor

pjcozzi commented Sep 21, 2018

Looks great at quick glance!

@bagnell
Copy link
Contributor Author

bagnell commented Sep 21, 2018

There is one bug in the atmosphere demo where clicking the reset button doesn't seem to update the sliders.

Fixed.

document.getElementById('lightingFadeInDistanceText').value = defaultLightFadeIn;
document.getElementById('nightFadeOutDistanceSlider').value = defaultNightFadeOut;
document.getElementById('nightFadeOutDistanceText').value = defaultNightFadeOut;
document.getElementById('nightFadeInDistanceSlider').value = defaultNightFadeOut;
Copy link
Contributor

Choose a reason for hiding this comment

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

I think the proper way to do this in knockout is:

viewModel.lightingFadeOutDistance = defaultLightFadeOut;
viewModel.lightingFadeInDistance = defaultLightFadeIn;
viewModel.nightFadeOutDistance = defaultNightFadeOut;
viewModel.nightFadeInDistance = defaultNightFadeIn;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks. Updated.

@bagnell
Copy link
Contributor Author

bagnell commented Sep 21, 2018

I also noticed that the fog was broken, but it's now fixed. This is ready for another look.

@pjcozzi pjcozzi mentioned this pull request Sep 21, 2018
@lilleyse
Copy link
Contributor

Looks good to me. Thanks @bagnell, this is a really nice visual quality improvement!

@lilleyse
Copy link
Contributor

Just added some last bits of documentation: b75d1cf

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.

6 participants