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

Making ShadowMap constructor private #4890

Merged
merged 6 commits into from
Jan 25, 2017
Merged
Show file tree
Hide file tree
Changes from 4 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
74 changes: 51 additions & 23 deletions Source/Scene/ShadowMap.js
Original file line number Diff line number Diff line change
Expand Up @@ -107,37 +107,28 @@ define([

/**
* Creates a shadow map from the provided light camera.
* <p>
* Use {@link Viewer#shadowMap}
Copy link
Contributor

Choose a reason for hiding this comment

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

It's fine to remove the indentation for this line.

Expand on the description a bit. Maybe 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
*
* @param {Object} options An object containing the following properties:
* @param {Context} options.context The context in which to create the shadow map.
* @param {Camera} options.lightCamera A camera representing the light source.
* @param {Boolean} [options.enabled=true] Whether the shadow map is enabled.
* @param {Boolean} [options.isPointLight=false] Whether the light source is a point light. Point light shadows do not use cascades.
* @param {Boolean} [options.pointLightRadius=100.0] Radius of the point light.
* @param {Boolean} [options.cascadesEnabled=true] Use multiple shadow maps to cover different partitions of the view frustum.
* @param {Number} [options.numberOfCascades=4] The number of cascades to use for the shadow map. Supported values are one and four.
* @param {Number} [options.maximumDistance=5000.0] The maximum distance used for generating cascaded shadows. Lower values improve shadow quality.
* @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.
* @internalConstructor
*
* @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}
*
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove empty line.

*/
function ShadowMap(options) {
options = defaultValue(options, defaultValue.EMPTY_OBJECT);
var context = options.context;
var scene = options.scene;

Copy link
Contributor

Choose a reason for hiding this comment

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

It's still nice to keep these parameter descriptions even if the shadow map isn't meant to be constructed directly.

//>>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 +140,8 @@ 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._normalOffsetScale = defaultValue(options.normalOffsetScale, 0.1);
Copy link
Contributor

Choose a reason for hiding this comment

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

I would not expose this right now as an option or a getter/setter. The default values used before are tuned for the different bias modes.

this.dirty = true;

/**
Expand Down Expand Up @@ -183,6 +176,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,8 +187,8 @@ define([
polygonOffset : polygonOffsetSupported,
polygonOffsetFactor : 1.1,
polygonOffsetUnits : 4.0,
normalOffset : true,
normalOffsetScale : 0.5,
normalOffset : this._normalOffset,
normalOffsetScale : this._normalOffsetScale,
normalShading : true,
normalShadingSmooth : 0.3,
depthBias : 0.0001
Expand All @@ -204,8 +198,8 @@ define([
polygonOffset : polygonOffsetSupported,
polygonOffsetFactor : 1.1,
polygonOffsetUnits : 4.0,
normalOffset : true,
normalOffsetScale : 0.1,
normalOffset : this._normalOffset,
normalOffsetScale : this._normalOffsetScale,
normalShading : true,
normalShadingSmooth : 0.05,
depthBias : 0.00002
Expand All @@ -215,8 +209,8 @@ define([
polygonOffset : false,
polygonOffsetFactor : 1.1,
polygonOffsetUnits : 4.0,
normalOffset : false,
normalOffsetScale : 0.0,
normalOffset : this._normalOffset,
normalOffsetScale : this._normalOffsetScale,
normalShading : true,
normalShadingSmooth : 0.1,
depthBias : 0.0005
Expand Down Expand Up @@ -384,6 +378,40 @@ 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.

}
},

/**
* Amount to offset shadows by along the normal. Addresses shadow acne problems.
*
* @memberof ShadowMap.prototype
* @type {Number}
* @default 0.1
*/
normalOffsetScale : {
get : function() {
return this._normalOffsetScale;
},
set : function(value) {
this.dirty = this._normalOffsetScale !== value;
this._normalOffsetScale = value;
}
},

/**
* Determines if soft shadows are enabled. Uses pcf filtering which requires more texture reads and may hurt performance.
*
Expand Down
22 changes: 11 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 @@ -413,15 +413,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 +511,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 +564,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 +848,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