Sun-position based atmosphere color #3955
Conversation
|
@likangning93 sounds great. When you open up a visual PR like this, we often include a screenshot (just drag and drop or copy and paste into the GitHub comment). |
| @@ -22,6 +22,7 @@ Change Log | |||
| * Added `CullingVolume.fromBoundingSphere`. | |||
| * Added `debugShowShadowVolume` to `GroundPrimitive`. | |||
| * Fixed issue where `Matrix4.fromCamera` was taking eye/target instead of position/direction. [3927](https://github.com/AnalyticalGraphicsInc/cesium/issues/3927) | |||
| * Added `setDayNight` to `SkyAtmosphere` to enable sun position based atmosphere color. Response to request for changeable atmosphere color. [3439](https://github.com/AnalyticalGraphicsInc/cesium/issues/3439) | |||
There was a problem hiding this comment.
You can remove the line "Response to request for changeable atmosphere color."
|
That's all my comments. Also fix the merge conflict (probably just CHANGES.md related). |
|
|
|
Looks beautiful! |
| @@ -1876,6 +1876,9 @@ define([ | |||
| var environmentState = scene._environmentState; | |||
| var renderPass = frameState.passes.render; | |||
| environmentState.skyBoxCommand = (renderPass && defined(scene.skyBox)) ? scene.skyBox.update(frameState) : undefined; | |||
| if (defined(scene.skyAtmosphere) && defined(scene.globe)) { | |||
There was a problem hiding this comment.
Since we are reusing scene.skyAtmosphere a few times here and below, it would be better to write this as
var skyAtmosphere = scene.skyAtmosphere;(same-ish for scene.globe).
See "Avoid redundant nested property access" here - https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Documentation/Contributors/CodingGuide#basic-code-construction
| var innerRadius = ellipsoid.maximumRadius; | ||
| var rayleighScaleDepth = 0.25; | ||
| // camera height, outer radius, inner radius, dayNight flag | ||
| this._camAndRadiiAndDayNight = new Cartesian4(); |
There was a problem hiding this comment.
Usually we try not to abbreviate names, so name it camera instead. Also as suggested by @pjcozzi there may be a better name for DayNight. Possibly dynamicLighting?
…ayNight with DynamicAtmosphereColor
|
I pushed some changes. |
| @@ -118,7 +118,7 @@ define([ | |||
| * @private | |||
| */ | |||
| SkyAtmosphere.prototype.setDayNight = function(enableLighting) { | |||
|
Sorry about that. Updated. |
|
I fixed a couple last name changes, otherwise this all looks good to me. |
| var innerRadius = ellipsoid.maximumRadius; | ||
| var rayleighScaleDepth = 0.25; | ||
| // camera height, outer radius, inner radius, dynamic atmosphere color flag | ||
| this._cameraAndRadiiAndDynamicAtmosphereColor = new Cartesian4(); |
There was a problem hiding this comment.
Just a style comment: we would generally write:
var cameraAndRadiiAndDynamicAtmosphereColor = new Cartesian4();
cameraAndRadiiAndDynamicAtmosphereColor.w = // ...
// ...
this._cameraAndRadiiAndDynamicAtmosphereColor = cameraAndRadiiAndDynamicAtmosphereColor;
|
Just that trivial comment. Look great! |
|
Merge in master, there is probably a trivial conflict in CHANGES.md |
|
The conflicting code was from my other pull request... I thought it looked familiar. Updated. |
|
Thanks. @lilleyse merge when ready! |
As a partial response to #3439, enabled support for sun position based atmospheric color when drawing with
enableLighting.Also updated CHANGES.md.