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

Cleanup sandcastle and docs for ground atmosphere #7152

Merged
merged 5 commits into from
Oct 19, 2018
Merged

Cleanup sandcastle and docs for ground atmosphere #7152

merged 5 commits into from
Oct 19, 2018

Conversation

OmarShehata
Copy link
Contributor

What I did

I think the current Sandcastle names of "Atmosphere" and "Atmosphere color" are confusing, because it implies they're different aspects of the same thing. But "Atmosphere color" only changes the color of the sky as seen from the ground or the atmosphere around the ellipsoid. Whereas "Atmosphere" is specifically about the ground atmosphere.

I renamed them to "Ground Atmosphere" and "Sky Atmosphere" respectively. Both will show up when a user searches for "Atmosphere" so it will be obvious there are two different ways of affecting the atmosphere.

I also updated some things that were copied from the Sky Atmosphere example to the Ground one, and added links to the Sandcastle in the docs.

One problem left

I updated the showGroundAtmosphere doc to say that it's only visible when the camera is at a distance between lightingFadeOutDistance and lightingFadeInDistance but the doc for those say:

The distance where everything becomes lit. This only takes effect when enableLighting is true.

Which seems to contradict this, since you can see it does affect the ground atmosphere whether or not lighting is enabled:

https://cesiumjs.org/Cesium/Build/Apps/Sandcastle/?src=Atmosphere.html

So I'm not sure whether this is expected behavior that lightingFadeInDistance controls both lighting and ground atmosphere and if the doc just needs to be updated, or if there's supposed to be a separate control for ground atmosphere?

@bagnell @lilleyse

@cesium-concierge
Copy link

Thanks for the pull request @OmarShehata!

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

🌍 🌎 🌏

@lilleyse
Copy link
Contributor

So I'm not sure whether this is expected behavior that lightingFadeInDistance controls both lighting and ground atmosphere and if the doc just needs to be updated, or if there's supposed to be a separate control for ground atmosphere?

I think it's expected that it controls both, and the doc just needs to be updated to say it takes effect when enableLighting or showGroundAtmosphere are true.

* Enable the ground atmosphere.
* Enable the ground atmosphere, which is drawn over the globe when viewed from a distance greater than <code>lightingFadeOutDistance</code>.
*
* @demo {@link https://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=Ground%20Atmosphere.html|Ground atmosphere demo on Sandcastle}
Copy link
Contributor

Choose a reason for hiding this comment

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

on Sandcastle to in Sancastle?

@lilleyse
Copy link
Contributor

The renames makes sense to me. Just mention them in CHANGES.md (1.50 had a rename too).

CHANGES.md Outdated
@@ -3,6 +3,9 @@ Change Log

### 1.51 - 2018-11-01

##### Breaking Changes :mega:
* Renamed Atmosphere Sandcastle example to [Ground Atmosphere](https://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=Ground%20Atmosphere.html) and Atmosphere color Sandcastle example to [Sky Atmosphere](https://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=Sky%20Atmosphere.html).
Copy link
Contributor

Choose a reason for hiding this comment

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

Sandcastle examples do not qualify as breaking changes, they are completely outside of the API.

Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm that's true... @OmarShehata could you remove the similar breaking change note from 1.50?

@OmarShehata
Copy link
Contributor Author

@lilleyse I removed the Sandcastle renames from breaking changes, both for this and for the 1.50 release from CHANGES.md.

@OmarShehata
Copy link
Contributor Author

@lilleyse this should be good to merge.

@lilleyse
Copy link
Contributor

Ah thanks for the reminder!

@lilleyse lilleyse merged commit 50d10f5 into CesiumGS:master Oct 19, 2018
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