Extent setters in Cesium3DTileStyle #5412

SunBlack · 2017-06-02T13:50:07Z

See discussion here

…sium3DTileStyle_setters

SunBlack · 2017-06-12T09:05:30Z

No idea why tests are failing here, during tests in #5308 seems to pass currently. On my local machine tests currently stucks on 3d-tiles and my branch.

pjcozzi · 2017-06-12T12:57:57Z

I saw the forum discussion, and the approach is OK with me. Thanks for the update, @SunBlack!

lilleyse · 2017-07-07T14:20:33Z

Source/Scene/Cesium3DTileStyle.js

@@ -209,7 +209,7 @@ define([
        },

        /**
-         * Gets or sets the {@link StyleExpression} object used to evaluate the style's <code>show</code> property.
+         * Gets or sets the {@link StyleExpression} object used to evaluate the style's <code>show</code> property. Alternative an object defining a show style can be used.


Add examples of each of these cases to the doc.

Also tweak the new description a bit: Alternatively a boolean, string, or object defining a show style can be used.

lilleyse · 2017-07-07T14:22:57Z

Source/Scene/Cesium3DTileStyle.js

+                } else {
+                    this._show = undefined;
+                    return;
+                }


Maybe it's not necessary to split up these two last cases. This would also allow someone to define their own evaluator like the example in the documentation.

if (typeof value === 'boolean') { this._show = new Expression(String(value)); } else if (typeof value === 'string') { this._show = new Expression(value); } else if (defined(value.conditions)) { this._show = new ConditionsExpression(value); } else { this._show = value; }

lilleyse · 2017-07-07T14:28:44Z

Source/Scene/Cesium3DTileStyle.js

@@ -246,13 +246,24 @@ define([
                return this._show;
            },
            set : function(value) {
+                if (typeof value === 'boolean') {
+                    this._show = new Expression(String(value));


Now that defines are part of the styling language also pass those through to the expressions.

I think this should be a separate PR, because we don't have a getter/setter for defines now. So I don't know if you really want old defines are still evaluated. And in case you allow getter and setter for defines you have to think other things, like if you need to recreate all expressions.

I don't think it hurts to pass the defines in even if they are no longer relevant to the new style, at the very least it does cut down on the code duplication. But if you would prefer that to be a different PR that is also ok with me.

lilleyse · 2017-07-07T14:33:39Z

Source/Scene/Cesium3DTileStyle.js

                this._showShaderFunctionReady = false;
-                this._show = value;
            }
        },


The setup function can now be simplified be calling these setters within.

Depends on discussion above ;)

I having some trouble with a test, if I use setter in setup. In case I use new setter to initialize all variables following test fail: return undefined shader functions when the style is empty. But I think existing tests are wrong in this case. If e.g. no color is defined DEFAULT_JSON_COLOR_EXPRESSION will be used. So I don't understand why the test is expecting an undefined result instead of a shader generated from value DEFAULT_JSON_COLOR_EXPRESSION.

For shader styling undefined is returned because otherwise DEFAULT_JSON_COLOR_EXPRESSION would cause all points to be white.

To fix this

that._colorShaderFunctionReady = !defined(styleJson.color); that._showShaderFunctionReady = !defined(styleJson.show); that._pointSizeShaderFunctionReady = !defined(styleJson.pointSize);

need to go below the setters in setup.

It works, but I don't like code design here, because it is not really clear why one time getColorShaderFunction returns undefined and other times a valid shader, if both times value of color is equal to DEFAULT_JSON_COLOR_EXPRESSION.

So I would prefer: In case no color expression is defined color returns undefined instead of DEFAULT_JSON_COLOR_EXPRESSION. In Cesium3DTileBatchTable.prototype.applyStyle change code from:

var color = style.color.evaluateColor(frameState, feature, scratchColor); var show = style.show.evaluate(frameState, feature);

to

var color = defined(style.color) ? style.color.evaluateColor(frameState, feature, scratchColor) : 'white'; var show = defined(style.show) ? style.show.evaluate(frameState, feature) : true;

That approach seems good to me too.

Behavior changed. I should be finished with the PR (hopefully all tests are passing on travis ;) )

…um3DTileStyle_setters

…_setters' into Extent_Cesium3DTileStyle_setters

…etter

SunBlack · 2017-07-17T10:47:40Z

No idea why the other tests are failing.

lilleyse · 2017-07-17T15:24:54Z

@SunBlack is this ready? The new changes look good to me.

SunBlack · 2017-07-17T15:29:52Z

Yes, i'm finished :)

lilleyse · 2017-07-17T15:30:04Z

Actually one small fix is needed in Cesium3DTilesInspectorViewModel:

this._style.color.evaluateColor(frameState, currentFeature, scratchColor); will need to check if color is defined first.

SunBlack · 2017-07-17T15:34:17Z

Maybe we should add a note to Readme about this changes. Any suggestion for a good short description?

lilleyse · 2017-07-17T16:32:59Z

A somewhat long description, but it works:

Added ability to set a style's color, show, or pointSize with a string or object literal. show may also take a boolean and pointSize may take a number.

SunBlack · 2017-07-17T18:38:12Z

Should we mention that Cesium3DTileStyle don't is initialized anymore with default values, because this is a breaking change?

…um3DTileStyle_setters

lilleyse · 2017-07-18T14:51:04Z

Yes also mention that in the breaking changes.

@pjcozzi would it be fine for this change to not go through deprecation? The change is that color, show, and pointSize properties of Cesium3DTileStyle can now be undefined rather than being set to default values.

pjcozzi · 2017-07-18T14:55:25Z

Maybe, I'll try to look at this soon.

SunBlack · 2017-07-20T11:28:12Z

In general I think it is okay to have here a breaking change, because 3d tiles are new to stable branch and I'm not sure if anyone is really using getter here. If not: We could add a deprecated warning to getter for release in 2 weeks and merge this MR direct after release. But I'm not sure if it is a good idea, because getter will be called at each tile and I'm not sure how I can filter it so message occurs only if it is really a deprecating case.

pjcozzi · 2017-07-21T12:34:28Z

Yes, this does not need to go through deprecation.

Please add a bit more doc to the set/get that explains that the get function doesn't always return exactly what is provided to the set function since it is converted to the native expression.

@lilleyse please merge when ready; I will not have the bandwidth to look at this again.

lilleyse · 2017-07-25T02:51:42Z

Thanks @SunBlack!

SunBlack · 2017-07-25T11:27:47Z

Tweaked doc is not fully correct, because if you set a custom object getter will return this custom object instead of an Expression, but i think it is okay ;)

Heiko Thiel added 3 commits June 2, 2017 15:46

Extent setters in Cesium3DTileStyle

0455a13

Merge remote-tracking branch 'remotes/cesium/3d-tiles' into Extent_Ce…

8a5dba1

…sium3DTileStyle_setters

Adjusted documentation

8b551d2

pjcozzi added the category - 3d tiles label Jun 12, 2017

mramato changed the base branch from 3d-tiles to master June 19, 2017 17:11

Merge branch 'master' into Extent_Cesium3DTileStyle_setters

486f9f6

lilleyse reviewed Jul 7, 2017

View reviewed changes

Heiko Thiel added 9 commits July 11, 2017 13:19

Merge remote-tracking branch 'remotes/cesium/master' into Extent_Cesi…

292123c

…um3DTileStyle_setters

Adjust documentation

4d54231

Merge remote-tracking branch 'remotes/origin/Extent_Cesium3DTileStyle…

7d14b87

…_setters' into Extent_Cesium3DTileStyle_setters

Added support of interpreting existing defines in Cesium3DTileStyle s…

15f1cbc

…etter

Fix tests

b8a5025

Changed behavior: Cesium3DTileStyle does not set default values anymore

8d57348

Fix whitespaces

c2eea43

Fix syntax error

9bc3764

Fix remaining Cesium3DTileStyleSpec tests

a40077c

lilleyse and others added 5 commits July 17, 2017 09:42

Fix eslint errors

c150bbf

Remove obsolete code

585cb0f

Fix after last commit

bfb7c51

Fix syntax mistaken

e47c0c6

Replace tab by whitespaces

3cdf90c

Fix Cesium3DTilesInspector

10d5c7a

Heiko Thiel added 3 commits July 17, 2017 23:18

Merge remote-tracking branch 'remotes/cesium/master' into Extent_Cesi…

d71aeb5

…um3DTileStyle_setters

Update CHANGES.md

7a44bfb

Merge remote-tracking branch 'remotes/cesium/master' into Extent_Cesi…

6c45625

…um3DTileStyle_setters

Heiko Thiel and others added 4 commits July 24, 2017 14:36

Merge branch 'master' into Extent_Cesium3DTileStyle_setters

6d88a37

Added deprecation note

70f562f

Tweak doc wording

fff7cdd

Merge branch 'master' into Extent_Cesium3DTileStyle_setters

2f2ac1c

lilleyse merged commit fbb1dd7 into CesiumGS:master Jul 25, 2017

SunBlack deleted the Extent_Cesium3DTileStyle_setters branch July 25, 2017 11:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extent setters in Cesium3DTileStyle #5412

Extent setters in Cesium3DTileStyle #5412

SunBlack commented Jun 2, 2017

SunBlack commented Jun 12, 2017

pjcozzi commented Jun 12, 2017

lilleyse Jul 7, 2017

lilleyse Jul 7, 2017

lilleyse Jul 7, 2017

lilleyse Jul 7, 2017

SunBlack Jul 11, 2017

lilleyse Jul 11, 2017

lilleyse Jul 7, 2017

SunBlack Jul 11, 2017

SunBlack Jul 13, 2017

lilleyse Jul 13, 2017

SunBlack Jul 14, 2017

lilleyse Jul 14, 2017

SunBlack Jul 14, 2017

SunBlack commented Jul 17, 2017

lilleyse commented Jul 17, 2017

SunBlack commented Jul 17, 2017

lilleyse commented Jul 17, 2017

SunBlack commented Jul 17, 2017

lilleyse commented Jul 17, 2017

SunBlack commented Jul 17, 2017

lilleyse commented Jul 18, 2017

pjcozzi commented Jul 18, 2017

SunBlack commented Jul 20, 2017

pjcozzi commented Jul 21, 2017

lilleyse commented Jul 25, 2017

SunBlack commented Jul 25, 2017

Extent setters in Cesium3DTileStyle #5412

Extent setters in Cesium3DTileStyle #5412

Conversation

SunBlack commented Jun 2, 2017

SunBlack commented Jun 12, 2017

pjcozzi commented Jun 12, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SunBlack commented Jul 17, 2017

lilleyse commented Jul 17, 2017

SunBlack commented Jul 17, 2017

lilleyse commented Jul 17, 2017

SunBlack commented Jul 17, 2017

lilleyse commented Jul 17, 2017

SunBlack commented Jul 17, 2017

lilleyse commented Jul 18, 2017

pjcozzi commented Jul 18, 2017

SunBlack commented Jul 20, 2017

pjcozzi commented Jul 21, 2017

lilleyse commented Jul 25, 2017

SunBlack commented Jul 25, 2017