Added HSB color adjustment to the atmosphere and a sandcastle demo

[likangning93]

Resolves #3439.

I added hue/saturation/brightness color adjustment to the atmosphere and a sandcastle demo for this.
Sean and I thought color post processing would be the best approach for adjusting the atmosphere color, and it seems like a lot of photo manipulation tools already have HSB color adjustment.

Also updated CHANGES.md.

[likangning93]

I made a small change to help with the sun-position based atmosphere mode. Computing luminance from the shifted color would obscure the night sky when the color shift increased brightness, so now the shader does luminance from the unshifted color.

[lilleyse]

The Sandcastle name is fine as just Atmosphere Color.html. Some other things to add to this demo might be a fog enabled button (like http://localhost:8080/Apps/Sandcastle/index.html?src=Terrain.html&label=Showcases), and set a different initial view, such as a view like your screenshot.

[lilleyse]

That's all my comments right now. The Sandcastle demo is a really nice touch, it's looking good.

[lilleyse]

Ah one more thing, make sure to add a test for this. Something simple like rendering the atmosphere normally, then rendering after changing the shifts, and checking that the resulting colors are different.

[pjcozzi]

Minor, should be: #3439.

Can you give @lilleyse and I commit access to your fork so we can make minor tweaks like this?

[pjcozzi]

@likangning93 can you make a nice animated gif of this so we can tweet it? Use http://www.cockos.com/licecap/ and just paste it into a comment in this PR.

[likangning93]

I'm having some trouble writing the test case that @lilleyse suggested. Here's what I have right now:

it('draws sky with color correction active', function() {
    var s = new SkyAtmosphere();
    var previousSkyAtmosphere = scene.skyAtmosphere;
    scene.skyAtmosphere = s;
    scene.isReadyForAtmosphere = true;

    scene.camera.setView({
        destination : Cartesian3.fromDegrees(-75.5847, 40.0397, 1000.0),
        orientation: {
            heading : -Math.PI_OVER_TWO,
            pitch : 0.2,
            roll : 0.0
        }
    });

    var command = s.update(scene.frameState);
    expect(command).toBeDefined();

    var noCorrectedPixels = scene.renderForSpecs();
    expect(noCorrectedPixels).toEqual([0, 0, 0, 255]);

    s.colorCorrect = true;
    expect(scene.renderForSpecs()).toEqual(noCorrectedPixels);
    s.hueShift = 1.0;
    expect(scene.renderForSpecs()).toEqual(noCorrectedPixels);
    s.hueShift = 0.5;
    expect(scene.renderForSpecs()).not.toEqual(noCorrectedPixels);

    scene.skyAtmosphere = previousSkyAtmosphere;
    scene.isReadyForAtmosphere = false;
});

I'm pretty sure the command for the skyAtmosphere is getting to the scene (when I console.log it from scene it's defined) but the last expect here keeps failing. Can I have renderForSpecs() render larger than just one pixel?

[likangning93]

Is this ok? Licecap's output seems to produce a lot of color banding.

[pjcozzi]

@likangning93 perhaps use a lower resolution but more bits or fps? I think it needs to be under 4MB for twitter.

If it turns out to be too much work, just create a 2-4 screenshots and we'll attach those to a tweet.

[likangning93]

How about this one? Giphy has a video to gif convertor that seems to do a better job at preserving color. It's about 1.7 MB.

[likangning93]

I think I streamlined the test some, but I'm still not sure if the SkyAtmosphere is drawing, or if that drawing would be visible in any configuration with how I'm currently testing. A lot of the other tests for SkyAtmosphere note that there's problems testing the pixel values from drawing with the SkyAtmosphere command.

    var s = new SkyAtmosphere(Ellipsoid.WGS84);
    var context = scene.context;
    var command = s.update(scene.frameState);
    expect(command).toBeDefined();
    command.execute(context);

    var noCorrectedPixels = context.readPixels();

    s.colorCorrect = true;
    command = s.update(scene.frameState);
    command.execute(context);
    expect(context.readPixels()).toEqual(noCorrectedPixels);
    s.hueShift = 1.0;
    command = s.update(scene.frameState);
    command.execute(context);
    expect(context.readPixels()).toEqual(noCorrectedPixels);
    s.brightnessShift = 1.0;
    command = s.update(scene.frameState);
    command.execute(context);
    expect(context.readPixels()).not.toEqual(noCorrectedPixels);

[lilleyse]

I'm basing this comment off of your previous code snippet:

So I think the main problem with the previous code was that you were setting scene.isReadyForAtmosphere = true; instead of scene._environmentState.isReadyForAtmosphere = true;. Also while it didn't affect the test, you use Math.PI_OVER_TWO which will be undefined because it's using JavaScript's Math library. When including Cesium/Core/Math, the "name" of the module is typically CesiumMath to get around this.

This code snippet worked for me:

scene.skyAtmosphere = new SkyAtmosphere();
scene._environmentState.isReadyForAtmosphere = true;

scene.camera.setView({
    destination : Cartesian3.fromDegrees(-75.5847, 40.0397, 1000.0),
    orientation: {
        heading : -CesiumMath.PI_OVER_TWO,
        pitch : 0.2,
        roll : 0.0
    }
});

var color = scene.renderForSpecs();
expect(color).not.toEqual([0, 0, 0, 255]);

scene.skyAtmosphere.hueShift = 0.5;
var hueColor = scene.renderForSpecs();
expect(hueColor).not.toEqual([0, 0, 0, 255]);
expect(hueColor).not.toEqual(color);

[lilleyse]

Actually I'm wrong that Math.PI_OVER_TWO will be undefined, though it is still good practice to use CesiumMath in case you also want to use the JS math library.

[likangning93]

Awesome! Thanks Sean! Updated.

[pjcozzi]

@likangning93 that animated gif is perfect, thanks! I scheduled a tweet for next week when folks are back from vacation!

[pjcozzi]

Can we aim to merge this today so it can make the 1.23 release tomorrow?

[likangning93]

Updated, fixed a merge conflict in CHANGES.md.

[lilleyse]

Instead of this variable it's easier to just check in update that hue, saturation, or brightness are not 0.0.

[lilleyse]

Mainly this is just from a usability standpoint, the user doesn't need to know to turn on colorCorrect first, it just works when changing hue, saturation, or brightness.

[likangning93]

Updated

[pjcozzi]

Also include a link to where the new Sandcastle example will be hosted, e.g., for 3D models, the link is http://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=3D%20Models.html&label=Showcases

[pjcozzi]

Looking good! Just those comments from me.

[likangning93]

Updated

[pjcozzi]

Looks good, thanks!

@lilleyse can do the honor of merging when ready!

[lilleyse]

Looks good to me too.