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

Globe underground color #8867

Merged
merged 13 commits into from
May 21, 2020
Merged

Globe underground color #8867

merged 13 commits into from
May 21, 2020

Conversation

lilleyse
Copy link
Contributor

Adds globe.undergroundColor and globe.undergroundColorByDistance for controlling how the back side of the globe is rendered when the camera is underground or the globe is translucent.

With the default settings it creates an underground fog type of look

fog

@lilleyse lilleyse requested a review from IanLilleyT May 18, 2020 01:03
@cesium-concierge
Copy link

Thanks for the pull request @lilleyse!

  • ✔️ Signed CLA found.

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
@lilleyse
Copy link
Contributor Author

lilleyse commented May 18, 2020

Feedback from #8726 (comment)

  • Rename undergroundColorByDistance to something else
  • When above ground, make undergroundColorByDistance relative to globe surface rather than camera

@mramato
Copy link
Contributor

mramato commented May 18, 2020

There seems to be a lot of undeground specific properties going in. Should we consider grouping these into their own globe.undeground property for easier documentation and discovery?

@lilleyse
Copy link
Contributor Author

@mramato that's a good idea, especially for the five new translucency options in #8726. This PR only adds two properties so maybe they can be grouped, but maybe it's not worth the trouble.

@mramato
Copy link
Contributor

mramato commented May 18, 2020

Right, I was kind of referring to the effort as a whole (everything going into the next release on June 1st). I haven't spent enough time to understand how many are truly undeground specific, but seems like something we might want to consider looking at.

Another thought, would be whether subsurface makes more sense than undeground? The first includes water, the second is only for the earth. I guess it all depends on how we handle underwater-specific features if/when that happens and if these features are generally meant for "undeground" specifically (maybe they are).

@lilleyse
Copy link
Contributor Author

lilleyse commented May 18, 2020

These two options are underground specifically. The underground color only shows up once you're below the the bathymetry shell, or the earth is translucent. underwater may have different options.

I need to think more about whether it makes sense to group underground and translucent options together.

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.

Other than the comments already brought up in the PR, the code looks good

Apps/Sandcastle/gallery/Underground Color.html Outdated Show resolved Hide resolved
Source/Scene/GlobeSurfaceTileProvider.js Outdated Show resolved Hide resolved
@lilleyse
Copy link
Contributor Author

lilleyse commented May 20, 2020

  • When above ground, make undergroundColorByDistance relative to globe surface rather than camera

Fixed, and in the process created a cool image.

ring

Sandcastle

@IanLilleyT IanLilleyT self-requested a review May 20, 2020 14:04
@lilleyse
Copy link
Contributor Author

@IanLilleyT updated

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, thanks @lilleyse !

@IanLilleyT IanLilleyT merged commit c9dd6f6 into master May 21, 2020
@IanLilleyT IanLilleyT deleted the underground-color branch May 21, 2020 18:48
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