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

Conversation

Projects
None yet
6 participants
@bagnell
Member

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

This comment has been minimized.

Show comment
Hide comment
@cesium-concierge

cesium-concierge 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.

🌍 🌎 🌏

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.

🌍 🌎 🌏

bagnell added some commits Aug 2, 2018

@hpinkos

This comment has been minimized.

Show comment
Hide comment
@hpinkos

hpinkos Aug 3, 2018

Contributor

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.

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

This comment has been minimized.

Show comment
Hide comment
@bagnell

bagnell Aug 7, 2018

Member

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

Member

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

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Aug 12, 2018

Member

Code looks good at ultra quick glance!

Member

pjcozzi commented Aug 12, 2018

Code looks good at ultra quick glance!

@bagnell

This comment has been minimized.

Show comment
Hide comment
@bagnell

bagnell Aug 20, 2018

Member

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.

Member

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 referenced this pull request Sep 6, 2018

Open

HDR Rendering #7017

0 of 8 tasks complete
@bagnell

This comment has been minimized.

Show comment
Hide comment
@bagnell

bagnell Sep 6, 2018

Member

Bump.

Member

bagnell commented Sep 6, 2018

Bump.

@lilleyse

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Sep 20, 2018

Contributor

I turned down the specular highlight. Did you mean the diffuse as well?

Actually the specular highlight might have been okay, mainly it is the diffuse that seems too strong:

bright

Contributor

lilleyse commented Sep 20, 2018

I turned down the specular highlight. Did you mean the diffuse as well?

Actually the specular highlight might have been okay, mainly it is the diffuse that seems too strong:

bright

@lilleyse

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Sep 20, 2018

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

Contributor

lilleyse commented Sep 20, 2018

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

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Sep 20, 2018

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.

Contributor

lilleyse commented Sep 20, 2018

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

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Sep 20, 2018

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

Contributor

lilleyse commented Sep 20, 2018

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

This comment has been minimized.

Show comment
Hide comment
@bagnell

bagnell Sep 20, 2018

Member

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.

Member

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

This comment has been minimized.

Show comment
Hide comment
@bagnell

bagnell Sep 20, 2018

Member

@lilleyse This is ready for another look.

Member

bagnell commented Sep 20, 2018

@lilleyse This is ready for another look.

@lilleyse

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Sep 21, 2018

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

Contributor

lilleyse commented Sep 21, 2018

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

This comment has been minimized.

Show comment
Hide comment
@pjcozzi

pjcozzi Sep 21, 2018

Member

Looks great at quick glance!

Member

pjcozzi commented Sep 21, 2018

Looks great at quick glance!

@bagnell

This comment has been minimized.

Show comment
Hide comment
@bagnell

bagnell Sep 21, 2018

Member

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

Fixed.

Member

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.

bagnell added some commits Sep 21, 2018

@bagnell

This comment has been minimized.

Show comment
Hide comment
@bagnell

bagnell Sep 21, 2018

Member

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

Member

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 referenced this pull request Sep 21, 2018

Open

Improve atmosphere #1064

@lilleyse

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Sep 24, 2018

Contributor

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

Contributor

lilleyse commented Sep 24, 2018

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

@lilleyse

This comment has been minimized.

Show comment
Hide comment
@lilleyse

lilleyse Sep 24, 2018

Contributor

Just added some last bits of documentation: b75d1cf

Contributor

lilleyse commented Sep 24, 2018

Just added some last bits of documentation: b75d1cf

@lilleyse lilleyse merged commit 4bd8ae8 into master Sep 24, 2018

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@lilleyse lilleyse deleted the atmosphere branch Sep 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment