Making ShadowMap constructor private #4890

rahwang · 2017-01-20T14:47:21Z

lilleyse · 2017-01-20T15:06:29Z

I'm not sure if this is possible to do, but can the constructor be private but ShadowMap still be visible in the documentation? Shadows have a few settings like darkness, maximumDistance, size, etc which would be nice to have in the documentation still.

pjcozzi · 2017-01-20T15:33:29Z

I'm not sure if this is possible to do, but can the constructor be private but ShadowMap still be visible in the documentation?

@internalConstructor - not sure how well supported it is now - see ModelMesh for an example.

Shadows have a few settings like darkness, maximumDistance, size, etc which would be nice to have in the documentation still.

How do users get to the shadow map object? Through the scene?

We could keep this class public with the private constructor and make any other members private as needed.

lilleyse · 2017-01-20T15:41:10Z

viewer.shadowMap is the standard way, but its also accessible from scene.

rahwang · 2017-01-20T19:29:58Z

How's this, @lilleyse ? Should normalOffset be visible since its mentioned?

lilleyse · 2017-01-20T20:24:28Z

Looking at jsdoc documentation @internalConstructor isn't supported, and I can't find another way of doing this...

Whether the constructor can be hidden or not, I think its still a good idea to keep the parameters since they are helpful for anyone coding cesium.

Maybe we just need a note that it is not recommend to construct a ShadowMap directly since Cesium does not support multiple light sources.

I don't see a problem with instantiating with scene instead of context, and just get the context with scene.context. Passing in scene is starting to get more common now with height references (like Model). This will have breaking changes though here and in Cesium Pro. Technically I think this can all be done without sending in context or scene, but will require bigger changes.

Should normalOffset be visible since its mentioned?

Hm, I suppose there should be a setting for this since it is mentioned. The normal offset is controlled by setting _terrainBias, _primitiveBias, and _pointBias's normalOffset property.

lilleyse · 2017-01-20T20:26:16Z

Oh so Cesium adds internalConstructor as a synonym for class in cesiumTags.js.

lilleyse · 2017-01-20T20:29:03Z

Overall adding that warning comment and using scene instead of context are what I would do for the time being. @pjcozzi what do you think about passing in scene?

pjcozzi · 2017-01-21T14:37:46Z

Overall adding that warning comment and using scene instead of context are what I would do for the time being. @pjcozzi what do you think about passing in scene?

Yes, more precisely

Pass in scene, instead of context
Mark the constructor internalConstructor. Include the "warning comment" just like the example I pointed to in ModelMesh - this should point the reader to Viewer.shadowMap to get the object
@lilleyse please evaluate if any members of ShadowMap should now be marked @private

lilleyse · 2017-01-23T14:38:52Z

@lilleyse please evaluate if any members of ShadowMap should now be marked @Private

All looks good.

pjcozzi · 2017-01-23T21:39:15Z

@rahwang any update here?

rahwang · 2017-01-24T18:49:04Z

Changes made, @pjcozzi . Now using scene instead of context.

lilleyse

Looks good. With a few tweaks I tested that the new normalOffset setter works well.

lilleyse · 2017-01-25T15:02:21Z

Source/Scene/ShadowMap.js

@@ -107,37 +107,28 @@ define([

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


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.

lilleyse · 2017-01-25T15:02:27Z

Source/Scene/ShadowMap.js

-     * @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.


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

lilleyse · 2017-01-25T15:02:44Z

Source/Scene/ShadowMap.js

     *
     * @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}
+     *


Remove empty line.

lilleyse · 2017-01-25T15:05:12Z

Source/Scene/ShadowMap.js

@@ -149,6 +140,8 @@ define([

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


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

lilleyse · 2017-01-25T15:05:49Z

Source/Scene/ShadowMap.js

@@ -149,6 +140,8 @@ define([

        this._enabled = defaultValue(options.enabled, true);
        this._softShadows = defaultValue(options.softShadows, false);
+        this._normalOffset = defaultValue(options.normalOffset, true);
+        this._normalOffsetScale = defaultValue(options.normalOffsetScale, 0.1);


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.

lilleyse · 2017-01-25T15:08:59Z

Source/Scene/ShadowMap.js

+            },
+            set : function(value) {
+                this.dirty = this._normalOffset !== value;
+                this._normalOffset = value;


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

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.

lilleyse · 2017-01-25T20:25:44Z

Looks good!

Making ShadowMap constructor private

f410631

Remove private constructor options from ShadowMap docs

7bd60d3

updating shadowMap constructor to take scene instead of context

c6c9229

Fixing comment for shadowMap normalOffset

8c3acf9

lilleyse reviewed Jan 25, 2017

View reviewed changes

rahwang added 2 commits January 25, 2017 15:16

Fixing shadowMap constructor changes

3c18a73

Adding tests for shadowMap constructor changes

a143d82

lilleyse merged commit 356b295 into CesiumGS:master Jan 25, 2017

rahwang deleted the make-shadowMap-constructor-private branch January 25, 2017 21:03

rahwang mentioned this pull request Jan 26, 2017

updating shadowMap constructor to take context instead of scene #4926

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Making ShadowMap constructor private #4890

Making ShadowMap constructor private #4890

rahwang commented Jan 20, 2017

lilleyse commented Jan 20, 2017

pjcozzi commented Jan 20, 2017

lilleyse commented Jan 20, 2017

rahwang commented Jan 20, 2017

lilleyse commented Jan 20, 2017

lilleyse commented Jan 20, 2017

lilleyse commented Jan 20, 2017 •

edited

Loading

pjcozzi commented Jan 21, 2017

lilleyse commented Jan 23, 2017

pjcozzi commented Jan 23, 2017

rahwang commented Jan 24, 2017

lilleyse left a comment

lilleyse Jan 25, 2017

lilleyse Jan 25, 2017

lilleyse Jan 25, 2017

lilleyse Jan 25, 2017

lilleyse Jan 25, 2017

lilleyse Jan 25, 2017

lilleyse Jan 25, 2017

lilleyse commented Jan 25, 2017

Making ShadowMap constructor private #4890

Making ShadowMap constructor private #4890

Conversation

rahwang commented Jan 20, 2017

lilleyse commented Jan 20, 2017

pjcozzi commented Jan 20, 2017

lilleyse commented Jan 20, 2017

rahwang commented Jan 20, 2017

lilleyse commented Jan 20, 2017

lilleyse commented Jan 20, 2017

lilleyse commented Jan 20, 2017 • edited Loading

pjcozzi commented Jan 21, 2017

lilleyse commented Jan 23, 2017

pjcozzi commented Jan 23, 2017

rahwang commented Jan 24, 2017

lilleyse left a comment

Choose a reason for hiding this comment

lilleyse Jan 25, 2017

Choose a reason for hiding this comment

lilleyse Jan 25, 2017

Choose a reason for hiding this comment

lilleyse Jan 25, 2017

Choose a reason for hiding this comment

lilleyse Jan 25, 2017

Choose a reason for hiding this comment

lilleyse Jan 25, 2017

Choose a reason for hiding this comment

lilleyse Jan 25, 2017

Choose a reason for hiding this comment

lilleyse Jan 25, 2017

Choose a reason for hiding this comment

lilleyse commented Jan 25, 2017

lilleyse commented Jan 20, 2017 •

edited

Loading