Skip to content

Making ShadowMap constructor private #4890

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

Merged
merged 6 commits into from
Jan 25, 2017
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion Apps/Sandcastle/gallery/development/Multiple Shadows.html
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,7 @@
camera.lookAtTransform(Cesium.Matrix4.IDENTITY);

var shadowMap = new Cesium.ShadowMap({
context : scene.context,
scene : scene,
lightCamera : pointLightCamera,
isPointLight : true,
softShadows : false
Expand Down
10 changes: 5 additions & 5 deletions Apps/Sandcastle/gallery/development/Shadows.html
Original file line number Diff line number Diff line change
Expand Up @@ -410,30 +410,30 @@

if (lightSource === 'Fixed') {
shadowOptions = {
context : context,
scene : scene,
lightCamera : fixedLightCamera,
cascadesEnabled : false
};
} else if (lightSource === 'Point') {
shadowOptions = {
context : context,
scene : scene,
lightCamera : pointLightCamera,
isPointLight : true
};
} else if (lightSource === 'Spot') {
shadowOptions = {
context : context,
scene : scene,
lightCamera : spotLightCamera,
cascadesEnabled : false
};
} else if (cascades === 4) {
shadowOptions = {
context : context,
scene : scene,
lightCamera : lightCamera
};
} else if (cascades === 1) {
shadowOptions = {
context : context,
scene : scene,
lightCamera : lightCamera,
numberOfCascades : 1
};
Expand Down
1 change: 1 addition & 0 deletions CHANGES.md
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ Change Log
* Removed separate `heading`, `pitch`, `roll` parameters from `Transform.headingPitchRollToFixedFrame` and `Transform.headingPitchRollQuaternion`. Pass a `headingPitchRoll` object instead. [#4843](https://github.com/AnalyticalGraphicsInc/cesium/pull/4843)
* The property `binornmal` has been renamed to `bitangent` for `Geometry` and `VertexFormat`. [#4856](https://github.com/AnalyticalGraphicsInc/cesium/pull/4856)
* A handful of `CesiumInspectorViewModel` properties were removed or changed from variables to functions. See [#4857](https://github.com/AnalyticalGraphicsInc/cesium/pull/4857)
* The `ShadowMap` constructor has been made private. See [#4010](https://github.com/AnalyticalGraphicsInc/cesium/issues/4010)
* Added support for custom geocoder services and autocomplete [#4723](https://github.com/AnalyticalGraphicsInc/cesium/pull/4723).
* Added [Custom Geocoder Sandcastle example](http://localhost:8080/Apps/Sandcastle/index.html?src=Custom%20Geocoder.html)
* Added `GeocoderService`, an interface for geocoders.
Expand Down
2 changes: 1 addition & 1 deletion Source/Scene/Scene.js
Original file line number Diff line number Diff line change
Expand Up @@ -563,7 +563,7 @@ define([
* @type {ShadowMap}
*/
this.shadowMap = new ShadowMap({
context : context,
scene : this,
lightCamera : this._sunCamera,
enabled : defaultValue(options.shadows, false)
});
Expand Down
40 changes: 33 additions & 7 deletions Source/Scene/ShadowMap.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,12 +107,15 @@ define([

/**
* Creates a shadow map from the provided light camera.
* <p>
* Use {@link Viewer#shadowMap} to get the scene's shadow map originating from the sun. In general do not construct directly.
* </p>
*
* The normalOffset bias pushes the shadows forward slightly, and may be disabled
* for applications that require ultra precise shadows.
*
* @alias ShadowMap
* @constructor
* @internalConstructor
*
* @param {Object} options An object containing the following properties:
* @param {Context} options.context The context in which to create the shadow map.
Expand All @@ -126,18 +129,19 @@ define([
* @param {Number} [options.size=2048] The width and height, in pixels, of each shadow map.
* @param {Boolean} [options.softShadows=false] Whether percentage-closer-filtering is enabled for producing softer shadows.
* @param {Number} [options.darkness=0.3] The shadow darkness.
* @param {Boolean} [options.normalOffset=true] Whether a normal bias is applied to shadows.
*
* @exception {DeveloperError} Only one or four cascades are supported.
*
* @demo {@link http://cesiumjs.org/Cesium/Apps/Sandcastle/index.html?src=Shadows.html|Cesium Sandcastle Shadows Demo}
*/
function ShadowMap(options) {
options = defaultValue(options, defaultValue.EMPTY_OBJECT);
var context = options.context;
var scene = options.scene;

//>>includeStart('debug', pragmas.debug);
if (!defined(context)) {
throw new DeveloperError('context is required.');
if (!defined(scene)) {
throw new DeveloperError('scene is required.');
}
if (!defined(options.lightCamera)) {
throw new DeveloperError('lightCamera is required.');
Expand All @@ -149,6 +153,7 @@ define([

this._enabled = defaultValue(options.enabled, true);
this._softShadows = defaultValue(options.softShadows, false);
this._normalOffset = defaultValue(options.normalOffset, true);
Copy link
Contributor

Choose a reason for hiding this comment

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

When the parameters are added back to the doc, include normalOffset.

this.dirty = true;

/**
Expand Down Expand Up @@ -183,6 +188,7 @@ define([
// In IE11 and Edge polygon offset is not functional.
// TODO : Also disabled for instances of Firefox and Chrome running ANGLE that do not support depth textures.
// Re-enable once https://github.com/AnalyticalGraphicsInc/cesium/issues/4560 is resolved.
var context = scene._context;
var polygonOffsetSupported = true;
if (FeatureDetection.isInternetExplorer() || FeatureDetection.isEdge() || ((FeatureDetection.isChrome() || FeatureDetection.isFirefox()) && FeatureDetection.isWindows() && !context.depthTexture)) {
polygonOffsetSupported = false;
Expand All @@ -193,7 +199,7 @@ define([
polygonOffset : polygonOffsetSupported,
polygonOffsetFactor : 1.1,
polygonOffsetUnits : 4.0,
normalOffset : true,
normalOffset : this._normalOffset,
normalOffsetScale : 0.5,
normalShading : true,
normalShadingSmooth : 0.3,
Expand All @@ -204,7 +210,7 @@ define([
polygonOffset : polygonOffsetSupported,
polygonOffsetFactor : 1.1,
polygonOffsetUnits : 4.0,
normalOffset : true,
normalOffset : this._normalOffset,
normalOffsetScale : 0.1,
normalShading : true,
normalShadingSmooth : 0.05,
Expand All @@ -215,7 +221,7 @@ define([
polygonOffset : false,
polygonOffsetFactor : 1.1,
polygonOffsetUnits : 4.0,
normalOffset : false,
normalOffset : this._normalOffset,
normalOffsetScale : 0.0,
normalShading : true,
normalShadingSmooth : 0.1,
Expand Down Expand Up @@ -384,6 +390,26 @@ define([
}
},

/**
* Determines if a normal bias will be applied to shadows.
*
* @memberof ShadowMap.prototype
* @type {Boolean}
* @default true
*/
normalOffset : {
get : function() {
return this._normalOffset;
},
set : function(value) {
this.dirty = this._normalOffset !== value;
this._normalOffset = value;
Copy link
Contributor

Choose a reason for hiding this comment

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

This setter should also set this._terrainBias.normalOffset, this._primitiveBias.normalOffset, and this._pointBias.normalOffset,

Copy link
Contributor

Choose a reason for hiding this comment

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

Also add a test for this. It should be fine to set normalOffset to false and expect the normalOffset to be false for all the bias modes. I don't think this requires any rendering as part of the test.

this._terrainBias.normalOffset = value;
this._primitiveBias.normalOffset = value;
this._pointBias.normalOffset = value;
}
},

/**
* Determines if soft shadows are enabled. Uses pcf filtering which requires more texture reads and may hurt performance.
*
Expand Down
33 changes: 22 additions & 11 deletions Specs/Scene/ShadowMapSpec.js
Original file line number Diff line number Diff line change
Expand Up @@ -275,7 +275,7 @@ defineSuite([
lightCamera.lookAt(center, new Cartesian3(0.0, 0.0, 1.0));

scene.shadowMap = new ShadowMap({
context : scene.context,
scene : scene,
lightCamera : lightCamera
});
}
Expand All @@ -289,7 +289,7 @@ defineSuite([
lightCamera.lookAt(center, new Cartesian3(0.0, 0.0, 1.0));

scene.shadowMap = new ShadowMap({
context : scene.context,
scene : scene,
lightCamera : lightCamera,
numberOfCascades : 1
});
Expand All @@ -313,7 +313,7 @@ defineSuite([
lightCamera.lookAt(center, new Cartesian3(0.0, 0.0, 20.0));

scene.shadowMap = new ShadowMap({
context : scene.context,
scene : scene,
lightCamera : lightCamera,
cascadesEnabled : false
});
Expand All @@ -331,7 +331,7 @@ defineSuite([
lightCamera.lookAt(center, new Cartesian3(0.0, 0.0, 20.0));

scene.shadowMap = new ShadowMap({
context : scene.context,
scene : scene,
lightCamera : lightCamera,
cascadesEnabled : false
});
Expand All @@ -345,7 +345,7 @@ defineSuite([
lightCamera.position = center;

scene.shadowMap = new ShadowMap({
context : scene.context,
scene : scene,
lightCamera : lightCamera,
isPointLight : true
});
Expand Down Expand Up @@ -390,7 +390,7 @@ defineSuite([

it('sets default shadow map properties', function() {
scene.shadowMap = new ShadowMap({
context : scene.context,
scene : scene,
lightCamera : new Camera(scene)
});

Expand All @@ -400,6 +400,7 @@ defineSuite([
expect(scene.shadowMap._isSpotLight).toBe(false);
expect(scene.shadowMap._cascadesEnabled).toBe(true);
expect(scene.shadowMap._numberOfCascades).toBe(4);
expect(scene.shadowMap._normalOffset).toBe(true);
});

it('throws without options.context', function() {
Expand All @@ -413,15 +414,15 @@ defineSuite([
it('throws without options.lightCamera', function() {
expect(function() {
scene.shadowMap = new ShadowMap({
context : scene.context
scene : scene
});
}).toThrowDeveloperError();
});

it('throws when options.numberOfCascades is not one or four', function() {
expect(function() {
scene.shadowMap = new ShadowMap({
context : scene.context,
scene : scene,
lightCamera : new Camera(scene),
numberOfCascades : 3
});
Expand Down Expand Up @@ -511,7 +512,7 @@ defineSuite([
lightCamera.lookAt(center, new Cartesian3(1.0, 0.0, 0.1));

scene.shadowMap = new ShadowMap({
context : scene.context,
scene : scene,
lightCamera : lightCamera
});

Expand Down Expand Up @@ -564,7 +565,7 @@ defineSuite([
lightCamera.lookAt(center, new Cartesian3(0.0, 0.0, 1.0));

scene.shadowMap = new ShadowMap({
context : scene.context,
scene : scene,
lightCamera : lightCamera
});

Expand Down Expand Up @@ -848,7 +849,7 @@ defineSuite([
lightCamera.lookAt(center, new Cartesian3(0.0, 0.0, 1.0));

scene.shadowMap = new ShadowMap({
context : scene.context,
scene : scene,
lightCamera : lightCamera
});

Expand Down Expand Up @@ -920,6 +921,16 @@ defineSuite([
expect(shadowFar).toBeGreaterThan(shadowFarFit);
});

it('set normalOffset', function() {
createCascadedShadowMap();
scene.shadowMap.normalOffset = false;

expect(scene.shadowMap._normalOffset, false);
expect(scene.shadowMap._terrainBias, false);
expect(scene.shadowMap._primitiveBias, false);
expect(scene.shadowMap._pointBias, false);
});

it('set maximumDistance', function() {
box.show = true;
floor.show = true;
Expand Down