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.

Sign up for GitHub

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

Extent setters in Cesium3DTileStyle #5412

Merged
merged 26 commits into from
Jul 25, 2017
Merged

Extent setters in Cesium3DTileStyle #5412

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
26 commits
Select commit Hold shift + click to select a range
0455a13
Extent setters in Cesium3DTileStyle
Jun 2, 2017
8a5dba1
Merge remote-tracking branch 'remotes/cesium/3d-tiles' into Extent_Ce…
Jun 12, 2017
8b551d2
Adjusted documentation
Jun 12, 2017
486f9f6
Merge branch 'master' into Extent_Cesium3DTileStyle_setters
lilleyse Jul 7, 2017
292123c
Merge remote-tracking branch 'remotes/cesium/master' into Extent_Cesi…
Jul 11, 2017
4d54231
Adjust documentation
Jul 11, 2017
7d14b87
Merge remote-tracking branch 'remotes/origin/Extent_Cesium3DTileStyle…
Jul 11, 2017
15f1cbc
Added support of interpreting existing defines in Cesium3DTileStyle s…
Jul 14, 2017
b8a5025
Fix tests
Jul 14, 2017
8d57348
Changed behavior: Cesium3DTileStyle does not set default values anymore
Jul 14, 2017
c2eea43
Fix whitespaces
Jul 14, 2017
9bc3764
Fix syntax error
Jul 15, 2017
a40077c
Fix remaining Cesium3DTileStyleSpec tests
Jul 17, 2017
c150bbf
Fix eslint errors
lilleyse Jul 17, 2017
585cb0f
Remove obsolete code
Jul 17, 2017
bfb7c51
Fix after last commit
Jul 17, 2017
e47c0c6
Fix syntax mistaken
Jul 17, 2017
3cdf90c
Replace tab by whitespaces
Jul 17, 2017
10d5c7a
Fix Cesium3DTilesInspector
Jul 17, 2017
d71aeb5
Merge remote-tracking branch 'remotes/cesium/master' into Extent_Cesi…
Jul 17, 2017
7a44bfb
Update CHANGES.md
Jul 17, 2017
6c45625
Merge remote-tracking branch 'remotes/cesium/master' into Extent_Cesi…
Jul 18, 2017
6d88a37
Merge branch 'master' into Extent_Cesium3DTileStyle_setters
Jul 24, 2017
70f562f
Added deprecation note
Jul 24, 2017
fff7cdd
Tweak doc wording
lilleyse Jul 25, 2017
2f2ac1c
Merge branch 'master' into Extent_Cesium3DTileStyle_setters
lilleyse Jul 25, 2017
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: 2 additions & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@ Change Log
* The function `Quaternion.fromHeadingPitchRoll(heading, pitch, roll, result)` was removed. Use `Quaternion.fromHeadingPitchRoll(hpr, result)` instead where `hpr` is a `HeadingPitchRoll`.
* The function `Transforms.headingPitchRollToFixedFrame(origin, headingPitchRoll, ellipsoid, result)` was removed. Use `Transforms.headingPitchRollToFixedFrame(origin, headingPitchRoll, ellipsoid, fixedFrameTransform, result)` instead where `fixedFrameTransform` is a a 4x4 transformation matrix (see `Transforms.localFrameToFixedFrameGenerator`).
* The function `Transforms.headingPitchRollQuaternion(origin, headingPitchRoll, ellipsoid, result)` was removed. Use `Transforms.headingPitchRollQuaternion(origin, headingPitchRoll, ellipsoid, fixedFrameTransform, result)` instead where `fixedFrameTransform` is a a 4x4 transformation matrix (see `Transforms.localFrameToFixedFrameGenerator`).
* The `color`, `show`, and `pointSize` properties of `Cesium3DTileStyle` are no longer initialized with default values.
* Deprecated
* `Scene/CullingVolume` is deprecated and will be removed in 1.38. Use `Core/CullingVolume`.
* `Scene/OrthographicFrustum` is deprecated and will be removed in 1.38. Use `Core/OrthographicFrustum`.
Expand All @@ -17,6 +18,7 @@ Change Log
* Fixed issue where composite 3D Tiles that contained instanced 3D Tiles with an external model reference would fail to download the model.
* Added behavior to `Cesium3DTilesInspector` that selects the first tileset hovered over if no tilest is specified. [#5139](https://github.com/AnalyticalGraphicsInc/cesium/issues/5139)
* Added ability to provide a `width` and `height` to `scene.pick`. [#5602](https://github.com/AnalyticalGraphicsInc/cesium/pull/5602)
* 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. [#5412](https://github.com/AnalyticalGraphicsInc/cesium/pull/5412)
* Added setter for `KmlDataSource.name` to specify a name for the datasource [#5660](https://github.com/AnalyticalGraphicsInc/cesium/pull/5660).
* Added setter for `GeoJsonDataSource.name` to specify a name for the datasource [#5653](https://github.com/AnalyticalGraphicsInc/cesium/issues/5653)
* Fixed issue where scene would blink when labels were added. [#5537](https://github.com/AnalyticalGraphicsInc/cesium/issues/5537)
Expand Down
13 changes: 8 additions & 5 deletions Source/Scene/Cesium3DTileBatchTable.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,6 +68,9 @@ define([
StencilOperation) {
'use strict';

var DEFAULT_COLOR_VALUE = Color.WHITE;
var DEFAULT_SHOW_VALUE = true;

/**
* @private
*/
Expand Down Expand Up @@ -414,7 +417,7 @@ define([
Check.typeOf.object('color', color);
//>>includeEnd('debug');

if (Color.equals(color, Color.WHITE) && !defined(this._batchValues)) {
if (Color.equals(color, DEFAULT_COLOR_VALUE) && !defined(this._batchValues)) {
// Avoid allocating since the default is white
return;
}
Expand Down Expand Up @@ -475,7 +478,7 @@ define([
//>>includeEnd('debug');

if (!defined(this._batchValues)) {
return Color.clone(Color.WHITE, result);
return Color.clone(DEFAULT_COLOR_VALUE, result);
}

var batchValues = this._batchValues;
Expand All @@ -495,7 +498,7 @@ define([

Cesium3DTileBatchTable.prototype.applyStyle = function(frameState, style) {
if (!defined(style)) {
this.setAllColor(Color.WHITE);
this.setAllColor(DEFAULT_COLOR_VALUE);
this.setAllShow(true);
return;
}
Expand All @@ -504,8 +507,8 @@ define([
var length = this.featuresLength;
for (var i = 0; i < length; ++i) {
var feature = content.getFeature(i);
var color = style.color.evaluateColor(frameState, feature, scratchColor);
var show = style.show.evaluate(frameState, feature);
var color = defined(style.color) ? style.color.evaluateColor(frameState, feature, scratchColor) : DEFAULT_COLOR_VALUE;
var show = defined(style.show) ? style.show.evaluate(frameState, feature) : DEFAULT_SHOW_VALUE;
this.setColor(i, color);
this.setShow(i, show);
}
Expand Down
157 changes: 104 additions & 53 deletions Source/Scene/Cesium3DTileStyle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,6 @@ define([
Expression) {
'use strict';

var DEFAULT_JSON_COLOR_EXPRESSION = 'color("#ffffff")';
var DEFAULT_JSON_BOOLEAN_EXPRESSION = true;
var DEFAULT_JSON_NUMBER_EXPRESSION = 1.0;

/**
* A style that is applied to a {@link Cesium3DTileset}.
* <p>
Expand Down Expand Up @@ -95,49 +91,13 @@ define([

styleJson = defaultValue(styleJson, defaultValue.EMPTY_OBJECT);

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

var colorExpression = defaultValue(styleJson.color, DEFAULT_JSON_COLOR_EXPRESSION);
var showExpression = defaultValue(styleJson.show, DEFAULT_JSON_BOOLEAN_EXPRESSION);
var pointSizeExpression = defaultValue(styleJson.pointSize, DEFAULT_JSON_NUMBER_EXPRESSION);

var defines = styleJson.defines;

var color;
if (typeof colorExpression === 'string') {
color = new Expression(colorExpression, defines);
} else if (defined(colorExpression.conditions)) {
color = new ConditionsExpression(colorExpression, defines);
}

that._color = color;

var show;
if (typeof showExpression === 'boolean') {
show = new Expression(String(showExpression), defines);
} else if (typeof showExpression === 'string') {
show = new Expression(showExpression, defines);
} else if (defined(showExpression.conditions)) {
show = new ConditionsExpression(showExpression, defines);
}

that._show = show;

var pointSize;
if (typeof pointSizeExpression === 'number') {
pointSize = new Expression(String(pointSizeExpression), defines);
} else if (typeof pointSizeExpression === 'string') {
pointSize = new Expression(pointSizeExpression, defines);
} else if (defined(pointSizeExpression.conditions)) {
pointSize = new ConditionsExpression(pointSizeExpression, defines);
}

that._pointSize = pointSize;
that.color = styleJson.color;
that.show = styleJson.show;
that.pointSize = styleJson.pointSize;

var meta = {};
if (defined(styleJson.meta)) {
var defines = styleJson.defines;
var metaJson = defaultValue(styleJson.meta, defaultValue.EMPTY_OBJECT);
for (var property in metaJson) {
if (metaJson.hasOwnProperty(property)) {
Expand Down Expand Up @@ -209,7 +169,8 @@ 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. Alternatively a boolean, string, or object defining a show style can be used.
* The getter will return the internal {@link Expression} or {@link ConditionsExpression}, which may differ from the value provided to the setter.
* <p>
* The expression must return or convert to a <code>Boolean</code>.
* </p>
Expand All @@ -234,6 +195,28 @@ define([
* return true;
* }
* };
*
* @example
* var style = new Cesium.Cesium3DTileStyle();
* // Override show expression with a boolean
* style.show = true;
* };
*
* @example
* var style = new Cesium.Cesium3DTileStyle();
* // Override show expression with a string
* style.show = '${Height} > 0';
* };
*
* @example
* var style = new Cesium.Cesium3DTileStyle();
* // Override show expression with a condition
* style.show = {
* conditions: [
* ['${height} > 2', 'false'],
* ['true', 'true']
* ];
* };
*/
show : {
get : function() {
Expand All @@ -246,13 +229,25 @@ define([
return this._show;
},
set : function(value) {
var defines = defaultValue(this._style, defaultValue.EMPTY_OBJECT).defines;
if (!defined(value)) {
this._show = undefined;
} else if (typeof value === 'boolean') {
this._show = new Expression(String(value));
} else if (typeof value === 'string') {
this._show = new Expression(value, defines);
} else if (defined(value.conditions)) {
this._show = new ConditionsExpression(value, defines);
} else {
this._show = value;
}
this._showShaderFunctionReady = false;
this._show = value;
}
},
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Depends on discussion above ;)

Copy link
Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Author

Choose a reason for hiding this comment

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

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;

Copy link
Contributor

Choose a reason for hiding this comment

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

That approach seems good to me too.

Copy link
Author

Choose a reason for hiding this comment

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

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


/**
* Gets or sets the {@link StyleExpression} object used to evaluate the style's <code>color</code> property.
* Gets or sets the {@link StyleExpression} object used to evaluate the style's <code>color</code> property. Alternatively a string or object defining a color style can be used.
* The getter will return the internal {@link Expression} or {@link ConditionsExpression}, which may differ from the value provided to the setter.
* <p>
* The expression must return a <code>Color</code>.
* </p>
Expand All @@ -277,6 +272,21 @@ define([
* return Cesium.Color.clone(Cesium.Color.WHITE, result);
* }
* };
*
* @example
* var style = new Cesium.Cesium3DTileStyle();
* // Override color expression with a string
* style.color = 'color("blue")';
*
* @example
* var style = new Cesium.Cesium3DTileStyle();
* // Override color expression with a condition
* style.color = {
* conditions : [
* ['${height} > 2', 'color("cyan")'],
* ['true', 'color("blue")']
* ]
* };
*/
color : {
get : function() {
Expand All @@ -289,13 +299,23 @@ define([
return this._color;
},
set : function(value) {
var defines = defaultValue(this._style, defaultValue.EMPTY_OBJECT).defines;
if (!defined(value)) {
this._color = undefined;
} else if (typeof value === 'string') {
this._color = new Expression(value, defines);
} else if (defined(value.conditions)) {
this._color = new ConditionsExpression(value, defines);
} else {
this._color = value;
}
this._colorShaderFunctionReady = false;
this._color = value;
}
},

/**
* Gets or sets the {@link StyleExpression} object used to evaluate the style's <code>pointSize</code> property.
* Gets or sets the {@link StyleExpression} object used to evaluate the style's <code>pointSize</code> property. Alternatively a number, string, or object defining a pointSize style can be used.
* The getter will return the internal {@link Expression} or {@link ConditionsExpression}, which may differ from the value provided to the setter.
* <p>
* The expression must return or convert to a <code>Number</code>.
* </p>
Expand All @@ -320,6 +340,26 @@ define([
* return 1.0;
* }
* };
*
* @example
* var style = new Cesium.Cesium3DTileStyle();
* // Override pointSize expression with a number
* style.pointSize = 1.0;
*
* @example
* var style = new Cesium.Cesium3DTileStyle();
* // Override pointSize expression with a string
* style.pointSize = '${height} / 10';
*
* @example
* var style = new Cesium.Cesium3DTileStyle();
* // Override pointSize expression with a condition
* style.pointSize = {
* conditions : [
* ['${height} > 2', '1.0'],
* ['true', '2.0']
* ]
* };
*/
pointSize : {
get : function() {
Expand All @@ -332,8 +372,19 @@ define([
return this._pointSize;
},
set : function(value) {
var defines = defaultValue(this._style, defaultValue.EMPTY_OBJECT).defines;
if (!defined(value)) {
this._pointSize = undefined;
} else if (typeof value === 'number') {
this._pointSize = new Expression(String(value));
} else if (typeof value === 'string') {
this._pointSize = new Expression(value, defines);
} else if (defined(value.conditions)) {
this._pointSize = new ConditionsExpression(value, defines);
} else {
this._pointSize = value;
}
this._pointSizeShaderFunctionReady = false;
this._pointSize = value;
}
},

Expand Down Expand Up @@ -389,7 +440,7 @@ define([
}

this._colorShaderFunctionReady = true;
this._colorShaderFunction = this.color.getShaderFunction(functionName, attributePrefix, shaderState, 'vec4');
this._colorShaderFunction = defined(this.color) ? this.color.getShaderFunction(functionName, attributePrefix, shaderState, 'vec4') : undefined;
return this._colorShaderFunction;
};

Expand All @@ -411,7 +462,7 @@ define([
}

this._showShaderFunctionReady = true;
this._showShaderFunction = this.show.getShaderFunction(functionName, attributePrefix, shaderState, 'bool');
this._showShaderFunction = defined(this.show) ? this.show.getShaderFunction(functionName, attributePrefix, shaderState, 'bool') : undefined;
return this._showShaderFunction;
};

Expand All @@ -433,7 +484,7 @@ define([
}

this._pointSizeShaderFunctionReady = true;
this._pointSizeShaderFunction = this.pointSize.getShaderFunction(functionName, attributePrefix, shaderState, 'float');
this._pointSizeShaderFunction = defined(this.pointSize) ? this.pointSize.getShaderFunction(functionName, attributePrefix, shaderState, 'float') : undefined;
return this._pointSizeShaderFunction;
};

Expand Down
2 changes: 1 addition & 1 deletion Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1014,7 +1014,7 @@ define([
// Restore original color to feature that is no longer selected
var frameState = this._scene.frameState;
if (!this.colorize && defined(this._style)) {
currentFeature.color = this._style.color.evaluateColor(frameState, currentFeature, scratchColor);
currentFeature.color = defined(this._style.color) ? this._style.color.evaluateColor(frameState, currentFeature, scratchColor) : Color.WHITE;
} else {
currentFeature.color = oldColor;
}
Expand Down
Loading