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

Ground atmosphere/fog hue, saturation and brightness shifts #7157

Merged
merged 7 commits into from
Oct 19, 2018

Conversation

bagnell
Copy link
Contributor

@bagnell bagnell commented Oct 16, 2018

Added atmosphereHueShift, atmosphereSaturationShift, and atmosphereBrightnessShift properties to Globe which shift the color of the ground atmosphere to match the hue, saturation, and brightness shifts of the sky atmosphere.

Fixes #4195.

@cesium-concierge
Copy link

Thanks for the pull request @bagnell!

  • ✔️ Signed CLA found.

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.

🌍 🌎 🌏

@lilleyse
Copy link
Contributor

@OmarShehata would you like to review?

@OmarShehata
Copy link
Contributor

@bagnell this looks good to me! I like the updated Sandcastle example.

The only failing test is a color test in ModelSpec loads a glTF 2.0 model. It might just need a bigger epsilon?

@bagnell
Copy link
Contributor Author

bagnell commented Oct 19, 2018

The only failing test is a color test in ModelSpec loads a glTF 2.0 model. It might just need a bigger epsilon?

These code changes don't affect anything in model so something else is causing that to fail.

@OmarShehata
Copy link
Contributor

I'm still seeing a failure on that test. It seems like the epsilon was changed from 5 to 10 here:

13e9f12#diff-dfb89d32e0b83bc7b650d1c64c56bfb5

But even when I check out that commit, npm run build and npm run test that test is still failing.

@OmarShehata
Copy link
Contributor

I should mention that test is also failing in master.

@bagnell
Copy link
Contributor Author

bagnell commented Oct 19, 2018

I'm still seeing a failure on that test. It seems like the epsilon was changed from 5 to 10 here:

Right, so it shouldn't impact this PR and perhaps @lilleyse can take a look at it.

@OmarShehata
Copy link
Contributor

OmarShehata commented Oct 19, 2018

I think we should merge this then since that failing test is unrelated and everything else looks good (I don't have commit access so I can't merge).

@lilleyse
Copy link
Contributor

Thanks @OmarShehata for the review. I had fun trying out the updated demo. Once CI passes I'll merge.

@lilleyse lilleyse merged commit e9f795e into master Oct 19, 2018
@lilleyse lilleyse deleted the atmosphere-hsb branch October 19, 2018 20:59
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.

4 participants