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

Added support for multiple expressions with passing tests #5232

Merged
merged 10 commits into from
May 23, 2017
Merged

Added support for multiple expressions with passing tests #5232

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
ae9745c
Added support for multiple expressions with passing tests
Jane-Of-Art Apr 24, 2017
1b681c3
Fixed comment to show that keys are case sensitive
Jane-Of-Art Apr 24, 2017
647fdfc
Removed console.log
Jane-Of-Art Apr 24, 2017
c1a11d0
removed change that's no longer necessary
Jane-Of-Art Apr 24, 2017
2be73f6
PR updates
lilleyse May 22, 2017
995929a
Move expressions to a different area
lilleyse May 22, 2017
b18a351
Merge branch '3d-tiles' into multiExpressions
lilleyse May 23, 2017
b08c03c
Add example to Point Cloud sandcastle
lilleyse May 23, 2017
8930949
Fix infinite loop where an expression references a property of the sa…
lilleyse May 23, 2017
be3933e
Small fixes
lilleyse May 23, 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
36 changes: 26 additions & 10 deletions Source/Scene/ConditionsExpression.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,8 +15,6 @@ define([
Expression) {
'use strict';

var expressionPlaceholder = /\$\{expression}/g;

/**
* Evaluates a conditions expression defined using the
* {@link https://github.com/AnalyticalGraphicsInc/3d-tiles/tree/master/Styling|3D Tiles Styling language}.
Expand All @@ -32,9 +30,14 @@ define([
* @example
* var expression = new Cesium.Expression({
* expression : 'regExp("^1(\\d)").exec(${id})',
* expressions : {
* id : "RegEx('^1(\\d)$').exec(${id})",
Copy link
Contributor

Choose a reason for hiding this comment

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

This is technically valid if ${id} is a built-in tile property, but as an example it is confusing to see id reference ${id}. Maybe use ${name} instead.

(yeah... this was my example in #5153)

* Area : "${length} * ${height}"
* },
* conditions : [
* ['${expression} === "1"', 'color("#FF0000")'],
* ['${expression} === "2"', 'color("#00FF00")'],
* ["(${id} !== 1) && (${Area} > 0)", "color('#0000FF')"],
* ['true', 'color("#FFFFFF")']
* ]
Copy link
Contributor

Choose a reason for hiding this comment

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

For clarity, remove the new line and use ${id} in the first condition and ${Area} in the second.

* });
Expand All @@ -45,8 +48,13 @@ define([
function ConditionsExpression(conditionsExpression) {
this._conditionsExpression = clone(conditionsExpression, true);
this._conditions = conditionsExpression.conditions;
this._expression = conditionsExpression.expression;

// Insert expression to expressions if it exists
// Then evaluate using expressions throughout class
// this._expression has to stay in prototype for specs to keep passing, but it can be removed in the future.
this._expression = conditionsExpression.expression;
Copy link
Contributor

Choose a reason for hiding this comment

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

As part of this PR can you remove any uses of _expression and also any styles that reference a single expression. We should only support the multiple-expression syntax.

this._expressions = conditionsExpression.expressions || {};
Copy link
Contributor

Choose a reason for hiding this comment

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

We try to avoid this type of syntax, and also the allocation. Instead use defaultValue(conditionsExpression.expressions, defaultValue.EMPTY_OBJECT)

this._expressions.expression = conditionsExpression.expression;
this._runtimeConditions = undefined;

setRuntime(this);
Expand Down Expand Up @@ -79,19 +87,27 @@ define([
var runtimeConditions = [];
var conditions = expression._conditions;
if (defined(conditions)) {
var exp = expression._expression;
var expressions = expression._expressions;
var length = conditions.length;
for (var i = 0; i < length; ++i) {
var statement = conditions[i];
var cond = String(statement[0]);
var condExpression = String(statement[1]);
if (defined(exp)) {
cond = cond.replace(expressionPlaceholder, exp);
condExpression = condExpression.replace(expressionPlaceholder, exp);
} else {
cond = cond.replace(expressionPlaceholder, 'undefined');
condExpression = condExpression.replace(expressionPlaceholder, 'undefined');

//Loop over all expressions for replacement instead of only replacing one
Copy link
Contributor

Choose a reason for hiding this comment

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

Shorten the description, just Loop over all expressions for replacement is fine.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also style note, change to // Loop

for (var key in expressions) {
if (expressions.hasOwnProperty((key))) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Remove unneeded parentheses.

var expressionPlaceholder = new RegExp('\\$\\{' + key + '\\}', 'g');
if (defined(expressions[key])) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Factor expressions[key] out into a variable.

cond = cond.replace(expressionPlaceholder, expressions[key]);
condExpression = condExpression.replace(expressionPlaceholder, expressions[key]);
} else {
cond = cond.replace(expressionPlaceholder, 'undefined');
condExpression = condExpression.replace(expressionPlaceholder, 'undefined');
}
}
}

runtimeConditions.push(new Statement(
new Expression(cond),
new Expression(condExpression)
Expand Down
51 changes: 51 additions & 0 deletions Specs/Scene/ConditionsExpressionSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,31 @@ defineSuite([
]
};

var jsonExpWithMultipleExpressions = {
expressions : {
halfHeight: '${Height}/2',
quarterHeight: '${Height}/4'
},
conditions : [
['${halfHeight} > 50 && ${halfHeight} < 100', 'color("blue")'],
['${quarterHeight} > 50 && ${quarterHeight} < 52', 'color("red")'],
['true', 'color("lime")']
]
};

var jsonExpWithMixedExpressions = {
expression: '${Height}',
expressions : {
halfHeight: '${Height}/2',
quarterHeight: '${Height}/4'
},
conditions : [
['${expression} > 100 && ${halfHeight} < 100', 'color("blue")'],
['${quarterHeight} > 50 && ${halfHeight} < 26', 'color("red")'],
['true', 'color("lime")']
]
};


it('constructs', function() {
var expression = new ConditionsExpression(jsonExp);
Expand Down Expand Up @@ -124,6 +149,32 @@ defineSuite([
expect(expression.evaluateColor(frameState, undefined)).toEqual(Color.BLUE);
});

it('constructs and evaluates conditional expression with multiple expressions', function() {
var expression = new ConditionsExpression(jsonExpWithMultipleExpressions);
expect(expression._expressions).toEqual({halfHeight: '${Height}/2', quarterHeight: '${Height}/4', expression: undefined});
expect(expression._conditions).toEqual([
['${halfHeight} > 50 && ${halfHeight} < 100', 'color("blue")'],
['${quarterHeight} > 50 && ${quarterHeight} < 52', 'color("red")'],
['true', 'color("lime")']
]);
expect(expression.evaluateColor(frameState, new MockFeature(101))).toEqual(Color.BLUE);
expect(expression.evaluateColor(frameState, new MockFeature(52))).toEqual(Color.LIME);
expect(expression.evaluateColor(frameState, new MockFeature(3))).toEqual(Color.LIME);
});

it('constructs and evaluates conditional expression with mixed expressions', function() {
var expression = new ConditionsExpression(jsonExpWithMixedExpressions);
expect(expression._expressions).toEqual({halfHeight: '${Height}/2', quarterHeight: '${Height}/4', expression: '${Height}'});
expect(expression._conditions).toEqual([
['${expression} > 100 && ${halfHeight} < 100', 'color("blue")'],
['${quarterHeight} > 50 && ${halfHeight} < 26', 'color("red")'],
['true', 'color("lime")']
]);
expect(expression.evaluateColor(frameState, new MockFeature(101))).toEqual(Color.BLUE);
expect(expression.evaluateColor(frameState, new MockFeature(52))).toEqual(Color.LIME);
expect(expression.evaluateColor(frameState, new MockFeature(3))).toEqual(Color.LIME);
});

it('gets shader function', function() {
var expression = new ConditionsExpression(jsonExpWithExpression);
var shaderFunction = expression.getShaderFunction('getColor', '', {}, 'vec4');
Expand Down