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

Jump to bottom

Feature reactive Cesium3DTileStyle.style object #7573

Merged
merged 10 commits into from
Mar 30, 2019
Merged

Feature reactive Cesium3DTileStyle.style object #7573

Show file tree
Hide file tree
Changes from 6 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
baf817a
Cesium3DTileStyle.style is reactive
bkuster Feb 13, 2019
e6f454e
handle empty style object
bkuster Feb 15, 2019
3537582
setting of expressions on the style object
bkuster Feb 15, 2019
3771fac
spec update
bkuster Feb 15, 2019
b3f8915
CHANGES.md
bkuster Feb 15, 2019
4fc0b72
Merge branch 'master' into feature-reactiveStyleObject
Mar 18, 2019
6b576b1
.style update refactor
bkuster Mar 21, 2019
32e5732
Merge remote-tracking branch 'original/master' into feature-reactiveS…
bkuster Mar 21, 2019
65a39f3
getJsonFromExpression return custom expressions
bkuster Mar 22, 2019
5f05495
added missing require to spec
bkuster Mar 22, 2019
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
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,7 @@ Change Log
##### Additions :tada:
* `Resource.fetchImage` now has a `flipY` option to vertically flip an image during fetch & decode. It is only valid when `ImageBitmapOptions` is supported by the browser. [#7579](https:/AnalyticalGraphicsInc/cesium/pull/7579)
* Added `backFaceCulling` and `normalShading` options to `PointCloudShading`. Both options are only applicable for point clouds containing normals. [#7399](https:/AnalyticalGraphicsInc/cesium/pull/7399)
* `Cesium3DTileStyle.style` reacts to updates and represents the current state of the style. [#7567](https:/AnalyticalGraphicsInc/cesium/issues/7567)

##### Fixes :wrench:
* Fixed the value for `BlendFunction.ONE_MINUS_CONSTANT_COLOR`. [#7624](https:/AnalyticalGraphicsInc/cesium/pull/7624)
Expand Down
67 changes: 40 additions & 27 deletions Source/Scene/Cesium3DTileStyle.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -159,16 +159,29 @@ define([
that._ready = true;
}

function getExpression(tileStyle, value) {
function getExpression(tileStyle, value, key) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it would be better to separate getting the expression from setting the json. One idea:

function getJsonFromExpression(expression) {
    if (defined(value.expression)) {
        return value.expression;
    } else if (defined(value.conditionsExpression)) {
        return clone(value.conditionsExpression, true);
    }
    return expression;
}
set : function(value) {
    this._show = getExpression(this, value);
    this._style.show = getJsonFromExpression(this._show);
}

A couple things to handle are:

  • this._style would need to be initialized to an empty object
  • What happens if the expression is a custom expression? Should that be added to the json as is? Or should getJsonFromExpression return undefined in that case.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks for the feedback @lilleyse. True, abusing getExpression in this fashion is not very intuitive, I like your proposal and will implement something along those lines.

  1. this._style initialization in the constructor makes sense to me, but this would change a non-experimental API, as the default value for .style would not longer be undefined but Object, or would you still return undefined in the getter, if the Object is empty?
  2. Haven't thought of custom expressions tbh. Both approaches (undefined or the expression instance) have their uses. You cant serialize the expression, but you could use it to clone the style at runtime. I would lean toward an implementation that is closest to the current behavior, I'll have to checkout what that is first, but would assume it to be undefined?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Changed my mind on 2.. Behavior as is now adds the custom expression passed in the options to style, this should remain unchanged. Added some specs to outline the behavior.

var defines = defaultValue(tileStyle._style, defaultValue.EMPTY_OBJECT).defines;
if (!defined(tileStyle._style)) {
tileStyle._style = {};
}
if (!defined(value)) {
delete tileStyle._style[key];
return undefined;
} else if (typeof value === 'boolean' || typeof value === 'number') {
tileStyle._style[key] = value;
return new Expression(String(value));
} else if (typeof value === 'string') {
tileStyle._style[key] = value;
return new Expression(value, defines);
} else if (defined(value.conditions)) {
tileStyle._style[key] = clone(value, true);
return new ConditionsExpression(value, defines);
} else if (defined(value.expression)) {
tileStyle._style[key] = value.expression;
return value;
} else if (defined(value.conditionsExpression)) {
tileStyle._style[key] = value.conditionsExpression;
return value;
}
return value;
}
Expand Down Expand Up @@ -294,7 +307,7 @@ define([
return this._show;
},
set : function(value) {
this._show = getExpression(this, value);
this._show = getExpression(this, value, 'show');
this._showShaderFunctionReady = false;
}
},
Expand Down Expand Up @@ -356,7 +369,7 @@ define([
return this._color;
},
set : function(value) {
this._color = getExpression(this, value);
this._color = getExpression(this, value, 'color');
this._colorShaderFunctionReady = false;
}
},
Expand Down Expand Up @@ -423,7 +436,7 @@ define([
return this._pointSize;
},
set : function(value) {
this._pointSize = getExpression(this, value);
this._pointSize = getExpression(this, value, 'pointSize');
this._pointSizeShaderFunctionReady = false;
}
},
Expand Down Expand Up @@ -472,7 +485,7 @@ define([
return this._pointOutlineColor;
},
set : function(value) {
this._pointOutlineColor = getExpression(this, value);
this._pointOutlineColor = getExpression(this, value, 'pointOutlineColor');
}
},

Expand Down Expand Up @@ -520,7 +533,7 @@ define([
return this._pointOutlineWidth;
},
set : function(value) {
this._pointOutlineWidth = getExpression(this, value);
this._pointOutlineWidth = getExpression(this, value, 'pointOutlineWidth');
}
},

Expand Down Expand Up @@ -568,7 +581,7 @@ define([
return this._labelColor;
},
set : function(value) {
this._labelColor = getExpression(this, value);
this._labelColor = getExpression(this, value, 'labelColor');
}
},

Expand Down Expand Up @@ -616,7 +629,7 @@ define([
return this._labelOutlineColor;
},
set : function(value) {
this._labelOutlineColor = getExpression(this, value);
this._labelOutlineColor = getExpression(this, value, 'labelOutlineColor');
}
},

Expand Down Expand Up @@ -664,7 +677,7 @@ define([
return this._labelOutlineWidth;
},
set : function(value) {
this._labelOutlineWidth = getExpression(this, value);
this._labelOutlineWidth = getExpression(this, value, 'labelOutlineWidth');
}
},

Expand Down Expand Up @@ -712,7 +725,7 @@ define([
return this._font;
},
set : function(value) {
this._font = getExpression(this, value);
this._font = getExpression(this, value, 'font');
}
},

Expand Down Expand Up @@ -760,7 +773,7 @@ define([
return this._labelStyle;
},
set : function(value) {
this._labelStyle = getExpression(this, value);
this._labelStyle = getExpression(this, value, 'labelStyle');
}
},

Expand Down Expand Up @@ -808,7 +821,7 @@ define([
return this._labelText;
},
set : function(value) {
this._labelText = getExpression(this, value);
this._labelText = getExpression(this, value, 'labelText');
}
},

Expand Down Expand Up @@ -856,7 +869,7 @@ define([
return this._backgroundColor;
},
set : function(value) {
this._backgroundColor = getExpression(this, value);
this._backgroundColor = getExpression(this, value, 'backgroundColor');
}
},

Expand Down Expand Up @@ -895,7 +908,7 @@ define([
return this._backgroundPadding;
},
set : function(value) {
this._backgroundPadding = getExpression(this, value);
this._backgroundPadding = getExpression(this, value, 'backgroundPadding');
}
},

Expand Down Expand Up @@ -943,7 +956,7 @@ define([
return this._backgroundEnabled;
},
set : function(value) {
this._backgroundEnabled = getExpression(this, value);
this._backgroundEnabled = getExpression(this, value, 'backgroundEnabled');
}
},

Expand Down Expand Up @@ -982,7 +995,7 @@ define([
return this._scaleByDistance;
},
set : function(value) {
this._scaleByDistance = getExpression(this, value);
this._scaleByDistance = getExpression(this, value, 'scaleByDistance');
}
},

Expand Down Expand Up @@ -1021,7 +1034,7 @@ define([
return this._translucencyByDistance;
},
set : function(value) {
this._translucencyByDistance = getExpression(this, value);
this._translucencyByDistance = getExpression(this, value, 'translucencyByDistance');
}
},

Expand Down Expand Up @@ -1060,7 +1073,7 @@ define([
return this._distanceDisplayCondition;
},
set : function(value) {
this._distanceDisplayCondition = getExpression(this, value);
this._distanceDisplayCondition = getExpression(this, value, 'distanceDisplayCondition');
}
},

Expand Down Expand Up @@ -1108,7 +1121,7 @@ define([
return this._heightOffset;
},
set : function(value) {
this._heightOffset = getExpression(this, value);
this._heightOffset = getExpression(this, value, 'heightOffset');
}
},

Expand Down Expand Up @@ -1156,7 +1169,7 @@ define([
return this._anchorLineEnabled;
},
set : function(value) {
this._anchorLineEnabled = getExpression(this, value);
this._anchorLineEnabled = getExpression(this, value, 'anchorLineEnabled');
}
},

Expand Down Expand Up @@ -1204,7 +1217,7 @@ define([
return this._anchorLineColor;
},
set : function(value) {
this._anchorLineColor = getExpression(this, value);
this._anchorLineColor = getExpression(this, value, 'anchorLineColor');
}
},

Expand Down Expand Up @@ -1252,7 +1265,7 @@ define([
return this._image;
},
set : function(value) {
this._image = getExpression(this, value);
this._image = getExpression(this, value, 'image');
}
},

Expand Down Expand Up @@ -1291,7 +1304,7 @@ define([
return this._disableDepthTestDistance;
},
set : function(value) {
this._disableDepthTestDistance = getExpression(this, value);
this._disableDepthTestDistance = getExpression(this, value, 'disableDepthTestDistance');
}
},

Expand Down Expand Up @@ -1339,7 +1352,7 @@ define([
return this._horizontalOrigin;
},
set : function(value) {
this._horizontalOrigin = getExpression(this, value);
this._horizontalOrigin = getExpression(this, value, 'horizontalOrigin');
}
},

Expand Down Expand Up @@ -1387,7 +1400,7 @@ define([
return this._verticalOrigin;
},
set : function(value) {
this._verticalOrigin = getExpression(this, value);
this._verticalOrigin = getExpression(this, value, 'verticalOrigin');
}
},

Expand Down Expand Up @@ -1435,7 +1448,7 @@ define([
return this._labelHorizontalOrigin;
},
set : function(value) {
this._labelHorizontalOrigin = getExpression(this, value);
this._labelHorizontalOrigin = getExpression(this, value, 'labelHorizontalOrigin');
}
},

Expand Down Expand Up @@ -1483,7 +1496,7 @@ define([
return this._labelVerticalOrigin;
},
set : function(value) {
this._labelVerticalOrigin = getExpression(this, value);
this._labelVerticalOrigin = getExpression(this, value, 'labelVerticalOrigin');
}
},

Expand Down
Loading