Added support for multiple expressions with passing tests #5232
Conversation
|
This is for Ticket #5153. For now, the support is for ${key}, where the keys are case sensitive. I haven't really thought about reserved expression names, especially since I don't know which ones are reserved. Since it looks like all reserved expressions are in full uppercase, one idea would be to standardize all keys in this spec to be .toLowerCase(). I would love your input on this issue! |
|
RE: New comments under original issue I did indeed implement this as string replacement. Therefore, I guess this isn't really a performance improvement, but I think this feature still has value as a development tool / convenience tool, as it'll be much more convenient for developers to write conditions with multiple expressions . |
Source/Scene/ConditionsExpression.js
Outdated
| * conditions : [ | ||
| * ['${expression} === "1"', 'color("#FF0000")'], | ||
| * ['${expression} === "2"', 'color("#00FF00")'], | ||
| * ["(${id} !== 1) && (${Area} > 0)", "color('#0000FF')"], | ||
| * ['true', 'color("#FFFFFF")'] | ||
| * ] |
There was a problem hiding this comment.
For clarity, remove the new line and use ${id} in the first condition and ${Area} in the second.
Source/Scene/ConditionsExpression.js
Outdated
| @@ -32,9 +30,14 @@ define([ | |||
| * @example | |||
| * var expression = new Cesium.Expression({ | |||
| * expression : 'regExp("^1(\\d)").exec(${id})', | |||
| * expressions : { | |||
| * id : "RegEx('^1(\\d)$').exec(${id})", | |||
There was a problem hiding this comment.
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)
Source/Scene/ConditionsExpression.js
Outdated
| // 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; |
There was a problem hiding this comment.
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.
Source/Scene/ConditionsExpression.js
Outdated
| // 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; | ||
| this._expressions = conditionsExpression.expressions || {}; |
There was a problem hiding this comment.
We try to avoid this type of syntax, and also the allocation. Instead use defaultValue(conditionsExpression.expressions, defaultValue.EMPTY_OBJECT)
Source/Scene/ConditionsExpression.js
Outdated
| cond = cond.replace(expressionPlaceholder, 'undefined'); | ||
| condExpression = condExpression.replace(expressionPlaceholder, 'undefined'); | ||
|
|
||
| //Loop over all expressions for replacement instead of only replacing one |
There was a problem hiding this comment.
Shorten the description, just Loop over all expressions for replacement is fine.
There was a problem hiding this comment.
Also style note, change to // Loop
Source/Scene/ConditionsExpression.js
Outdated
|
|
||
| //Loop over all expressions for replacement instead of only replacing one | ||
| for (var key in expressions) { | ||
| if (expressions.hasOwnProperty((key))) { |
There was a problem hiding this comment.
Remove unneeded parentheses.
Source/Scene/ConditionsExpression.js
Outdated
| for (var key in expressions) { | ||
| if (expressions.hasOwnProperty((key))) { | ||
| var expressionPlaceholder = new RegExp('\\$\\{' + key + '\\}', 'g'); | ||
| if (defined(expressions[key])) { |
There was a problem hiding this comment.
Factor expressions[key] out into a variable.
This is probably the right approach. If a batch table property or built-in property (like POSITION) and expression happen to use the same name, precedence goes towards the expression. |
|
Looks great overall! As just one extra thing can you edit one of the examples in |
|
@Jane-Of-Art @lilleyse what is the plan here? Do we want to get this into |
|
@lilleyse do you plan to finish this up? |
|
This was pretty close already, so I'm fine with finishing it up. |
lilleyse
force-pushed
the
multiExpressions
branch
from
8c84e10
to
9b21b83
Compare
…me name. Build regex for expression keys rather than generic variable regex.
lilleyse
force-pushed
the
multiExpressions
branch
from
9b21b83
to
8930949
Compare
|
@pjcozzi this is ready now. |
|
Spec issue: CesiumGS/3d-tiles#226 This is a breaking change for any styles that use the |
|
@pmconne heads up for this small breaking changing in the styling language: #5232 (comment) Thanks again @Jane-Of-Art @lilleyse! |
@lilleyse @Dylan-Brown
Keys inside the expressions object and the key "expression" are now all used in parsing conditions.
Had a bit of trouble with having to learn and fix the new RegEx parsing, but it all works now!