fixed shadowing on entity

[tomermoshe]

Fixes #1825

[cesium-concierge]

@tomermoshe thanks for the pull request!

I noticed that CHANGES.md has not been updated. If this change updates the Cesium API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and bump this pull request so we know it was updated. For more info, see the Pull Request Guidelines.

I am a bot who helps you make Cesium awesome! Thanks again.

[tomermoshe]

Fixing issue #1825
Updated CHANGES.md

[hpinkos]

Thanks for the pull request @tomermoshe!
Can you please send us a Contributor License Agreement so we can review and merge this?
Also merge master when you get a chance, there is a conflict with CHANGES.md

[tomermoshe]

done and done :)

[hpinkos]

@tomermoshe this sample code from #1825 produces the same lighting problems that exist in master:

var viewer = new Cesium.Viewer('cesiumContainer', {
    sceneMode: Cesium.SceneMode.SCENE2D
});
var scene = viewer.scene;

var instances = [];

for (var lon = -175.0; lon < 175.0; lon += 5.0) {
  for (var lat = -85.0; lat < 85.0; lat += 5.0) {
    instances.push(new Cesium.GeometryInstance({
      geometry : new Cesium.RectangleGeometry({
        rectangle : Cesium.Rectangle.fromDegrees(lon, lat, lon + 5.0, lat + 5.0),
        vertexFormat : Cesium.PerInstanceColorAppearance.VERTEX_FORMAT
      }),
      attributes : {
        color : Cesium.ColorGeometryInstanceAttribute.fromColor(Cesium.Color.WHITE)
      }
    }));
  }
}

scene.primitives.add(new Cesium.Primitive({
  geometryInstances : instances,
  appearance : new Cesium.PerInstanceColorAppearance()
}));

[tomermoshe]

It's because the appearance is created without {flat:true} . Right now by default PerInstanceColorAppearance is using PerInstanceColorAppearanceFS which takes shadowing into account. If PerInstanceColorAppearance is created with {flat: true} the shader that will be used is PerInstanceColorAppearanceFlatFS (not 100% sure about the name of the shader as I'm not near a computer) which will draw the geometry without shadows.

[hpinkos]

Thanks for the explanation @tomermoshe, I forgot about that. We should make sure that the Entity API is passing the correct options to the appearance as well.

@bagnell do you want to review this?

[mramato]

While flat: true works around the issue, I don't think it's the actual fix for the issue. @bagnell may know more.

[pjcozzi]

@tomermoshe thanks again for contributing, we received the CLA.

[tomermoshe]

Hey guys :) How is the review going?
@mramato As of my understanding, Logically, If PerInstanceColorAppearance is created explicitly with no shadows, the coloring of the geometry should be flat regardless of the issue. Is that correct?

[hpinkos]

@tomermoshe sorry for the delay. A lot of the team is at the FOSS4G conference this week. We'll have more time to review your PR when we get back. Thanks!

[tomermoshe]

any progress on the PR review?

[lilleyse]

I don't think this is the right fix either.

@mramato As of my understanding, Logically, If PerInstanceColorAppearance is created explicitly with no shadows, the coloring of the geometry should be flat regardless of the issue. Is that correct?

Shadows only affect whether the geometry receives shadows from itself or other objects in the scene, but does not affect the shading of the geometry. So you can have a geometry with flat shading that still receives shadows.

Basically shadows and flat/regular shading are separate concepts.

[hpinkos]

Thanks for taking the time to look into this @tomermoshe! However, it sounds like this isn't the way we want to go about fixing this. We're always happy to review pull requests if you want to submit a different fix =)

@lilleyse if you have any suggestions for a different approach, please make a comment in #1825