Open pjcozzi opened 9 years ago
I will add this to the roadmap.
Also, some useful info is here: http://stackoverflow.com/questions/4719320/new-number-vs-number
@ggetz here's some notes for when you are ready to integrate the new expressions into the style JSON schema.
show
options go away, and show
is always a string with an expression that evaluates to Boolean.color
can be a string with an expression that evaluates to Color or an object for a color map or ramp (replacing the highlighted code here).Here's a map:
"color" : {
"key" : "RegEx('^1(\\d)$').exec(${id})",
"map" : {
"1" : "Color('#FF0000')",
"2" : "Color('#00FF00')"
},
"defaultValue" : "Color('#FFFFFF')"
}
Note that the value in each pair in the map (e.g., "Color('#FF0000')"
) is an expression evaluating to a Color so it could include variables, etc.
Likewise, I'm pretty sure we will want the key in each pair to also be an expression (kinda like you had before), and we can optimize the case where all keys in the map are literals by building the hash lookup at compile time.
For "color ramps" or "intervals", I think it is best to start with a generic set of conditionals that we execute like a series of if...else
statements, e.g.,
"color" : {
"key" : "${Height} / ${Area}",
"conditional" : {
"(${KEY} >= 1.0) && (${KEY} < 10.0)" : "Color('#FF00FF')",
"(${KEY} >= 10.0) && (${KEY} < 30.0)" : "Color('#FF0000')",
"(${KEY} >= 30.0) && (${KEY} < 50.0)" : "Color('#FFFF00')",
"(${KEY} >= 50.0) && (${KEY} < 70.0)" : "Color('#00FF00')",
"(${KEY} >= 70.0) && (${KEY} < 100.0)" : "Color('#00FFFF')",
"(${KEY} >= 100.0)" : "Color('#0000FF')",
}
}
key
could be optional (or even not part of the schema, but it adds a nice readability and easy optimization for complete expressions). Each conditional property has an expression that evaluates to Boolean and an expression that evaluates to Color.
This approach allows a lot of flexibility and avoids a bunch of schema for closed/open intervals; however, it isn't well suited to a binary search for evaluation (it is basically a linear search). Optimizing it would require a bunch of analysis; later, if we need, we could just introduce an interval
schema.
Note that these color maps and ramps could be implemented completely in an expression with the ternary operator, but it would be ugly, painful to generate, and harder to optimize at runtime.
For "color ramps" or "intervals", I think it is best to start with a generic set of conditionals that we execute like a series of if...else statements
Aren't a map and ramp pretty much the same thing? For example, you can express the "map" in "ramp" syntax pretty easily if we add "defaultValue":
"color" : {
"key" : "RegEx('^1(\\d)$').exec(${id})",
"conditional" : {
"${key} === 1" : "Color('#FF0000')",
"${key} === 2" : "Color('#00FF00')"
},
"defaultValue" : "Color('#FFFFFF')"
}
I actually had that at one point, but made them separate so it would be easy to optimize maps with all literals, e.g., build the hash (JavaScript object) at compile time. However, I think it is still pretty easy to do the same compile-time analysis with conditional
(just look for === LITERAL
for all properties). Give it a shot!
@ggetz I tightened up the spec in 20d820105127cc3282ca523764c5ae0275547898. Can you please review?
A few questions:
RegExp
type have a toString()
function like Color
? It should.RegExp
(even if we just disable them for now). For example, colors say this:Colors support the following binary operators by performing component-wise operations: ===, !==, +, -, *, /, and %. For example color() === color() is true since the red, green, blue, and alpha components are equal.
- It's fine to say the following, but we also need to document the arguments like we do for
Color
,isNaN
, etc.The
regExp
function behaves like the JavaScriptRegExp
constructor and takes the same arguments.
- Can you add a statement to the variables section like 'Variables can be used anywhere an expression can be used except...' or can they be used everywhere (in which case, we should make that explicit)?
@ggetz to organize things, I moved our task lists from https://github.com/AnalyticalGraphicsInc/3d-tiles/issues/2#issuecomment-180355858 and https://github.com/AnalyticalGraphicsInc/3d-tiles/issues/2#issuecomment-183918785 to the top comment: https://github.com/AnalyticalGraphicsInc/3d-tiles/issues/2#issue-88423282
Does our RegExp type have a toString() function like Color? It should.
Not an explicit function, I will add one.
We need to explain how operators work with RegExp (even if we just disable them for now). For example, colors say this:
I would say operators do not work with RegExp at all for now.
I'll add the other items to the spec as well.
Also the changes in 20d8201
look good.
@ggetz I stepped through the code in Expression.js
and made some minor tweaks in https://github.com/AnalyticalGraphicsInc/cesium/commit/10c8c9f914a718cf1660bd7dd1133f6529752966
Here's a few questions/TODOs:
try
blocks and only do the part of the code that might throw instead the try
block._evaluateLiteralString
need to call replaceBackslashes
? Can we do this at compile time instead?instanceof
checks in _evaluatePlus
at compile time, e.g., so we will end up with more _evaluate
functions but they will be optimized for their particular type instead of doing type checking at runtime. If it helps make things clean in the compile step, we could even do a first pass over the AST to compute type info, then a second pass to generate the runtime AST. Not sure that the extra pass is necessary though.new Color
(like in _evaluatePlus
) will be bad for performance. Can we make color.evaluate
take a result parameter (see styleContent
in Cesium3DTileStyleEngine.js
) and use scratch variables?@ggetz test coverage for Expression.js
is pretty good at 98%. Can you please add tests for the DeveloperError
s that are not covered to bring coverage up to 100%?
Can we do the instanceof checks in _evaluatePlus at compile time, e.g., so we will end up with more _evaluate functions but they will be optimized for their particular type instead of doing type checking at runtime.
I'm guessing this would apply to all of the arithmetic _evaluate
functions?
Also we could do some of the instanceof
checks at compile time, but we would need to leave one evaluate function with the instanceof
checks in at runtime because of expressions where the types are not clear until evaluation, for example ${myColor} + 3 * color('red')
I'm guessing this would apply to all of the arithmetic _evaluate functions?
Yes.
Also we could do some of the instanceof checks at compile time, but we would need to leave one...
OK.
Should we wait for the optimized evaluate functions? I believe they are currently on the roadmap under "Later".
Should we wait for the optimized evaluate functions?
Will it be significantly time consuming or significantly deteriorate the code quality? If not, I would do it now. If so, then we can evaluate later.
If it's targeted to the operators involving Colors, it shouldn't be too time consuming.
@ggetz did you add key
to https://github.com/AnalyticalGraphicsInc/cesium/pull/3655 as suggested in https://github.com/AnalyticalGraphicsInc/3d-tiles/issues/2#issuecomment-187728554?
For example:
"color" : {
"key" : "${Height} / ${Area}",
"conditional" : {
"(${KEY} >= 1.0) && (${KEY} < 10.0)" : "color('#FF00FF')",
"(${KEY} >= 10.0) && (${KEY} < 30.0)" : "color('#FF0000')",
"(${KEY} >= 30.0) && (${KEY} < 50.0)" : "color('#FFFF00')",
"(${KEY} >= 50.0) && (${KEY} < 70.0)" : "color('#00FF00')",
"(${KEY} >= 70.0) && (${KEY} < 100.0)" : "color('#00FFFF')",
"(${KEY} >= 100.0)" : "color('#0000FF')"
}
}
If not, can you please add it and name it expression
since key
came from the map terminology?
Also, what do you think of renaming conditional
in the schema to conditions
, e.g.,
"color" : {
"expression" : "RegEx('^1(\\d)$').exec(${id})",
"conditional" : {
"{$EXPRESSION} === 1" : "color('#FF0000')",
"{$EXPRESSION} === 2" : "color('#00FF00')",
"true" : "color('#FFFFFF')"
}
}
becomes
"color" : {
"expression" : "RegEx('^1(\\d)$').exec(${id})",
"conditions" : {
"{$EXPRESSION} === 1" : "color('#FF0000')",
"{$EXPRESSION} === 2" : "color('#00FF00')",
"true" : "color('#FFFFFF')"
}
}
@ggetz did you add key to AnalyticalGraphicsInc/cesium#3655 as suggested in #2 (comment)?
Also, later, we could consider expanding key
/expression
into an object that could hold multiple expressions for complex conditions, e.g.,
"color" : {
"expressions" : {
"id" : "RegEx('^1(\\d)$').exec(${id})",
"area" : "${length} * ${height}"
},
"conditions" : {
"({$ID} !== 1) && (${AREA} > 0)" : "color('#FF0000')"
}
}
This is overkill for now, but let's consider it if experience shows it will be useful.
"Expression" and "conditions" sound good. For expression, I think ${expression}
is more straight forward than {$EXPRESSION}
because it looks like the other variables.
I do like the idea of multiple expressions as well.
I think ${expression} is more straight forward than {$EXPRESSION} because it looks like the other variables.
OK.
Also, this will override a property named expression
so, in that case, they need to use feature.expression
.
I do like the idea of multiple expressions as well.
OK, let's add it when we need it.
I do like the idea of multiple expressions as well.
Also, I already updated the roadmap for this.
Also, I already updated the roadmap for this.
OK thanks.
First cut of the spec is in #80.
First cut of the spec is in #80.
OK
Suggestions for new name:
tl;dr The current specification for conditions
requires JSON object property order to be preserved, which is rarely true.
JavaScript's for ... in
statement iterates over object properties in an arbitrary order. The popular Java library org.json defines a JSON object as "an unordered collection of name/value pairs". Iteration order over Golang maps (a common internal representation for JSON objects) is randomized.
Concrete example: Styling/README.md contains the example
{
"show" : "${Area} > 0",
"color" : {
"conditions" : {
"${Height} < 60" : "color('#13293D')",
"${Height} < 120" : "color('#1B98E0')",
"true" : "color('#E8F1F2', 0.5)"
}
}
}
A feature with property Height
== 0 matches all three conditions, and therefore its computed color will depend entirely on the evaluation order of the conditions.
Generating conditions
objects automatically will also be problematic, as some languages and libraries do not allow the order of properties in the serialized JSON output to be controlled.
Suggestion: either require that all conditions
are mutually exclusive (tricky to enforce) or replace the conditions
object with an ordered Array
of [condition, value]
pairs (uglier, but well defined).
Good eye, @twpayne! I'm surprised we haven't ran into this issue in practice already.
replace the conditions object with an ordered Array of [condition, value] pairs (uglier, but well defined).
+1 for this.
@lilleyse is it reasonable for you to make this change (code and spec) as part of your current point cloud declarative styling work?
@lilleyse is it reasonable for you to make this change (code and spec) as part of your current point cloud declarative styling work?
Yes it should be easy to change. For the spec I'll open a separate PR for this fix. For the code I'll make the change in my styling PR.
Yes it should be easy to change. For the spec I'll open a separate PR for this fix. For the code I'll make the change in my styling PR.
In https://github.com/AnalyticalGraphicsInc/cesium/pull/4336/commits/5cb1088d0dbc8b01d2eb46b289e1ab77150c01b1, part of https://github.com/AnalyticalGraphicsInc/cesium/pull/4336.
Labeling draft 1.0
just for the utility functions and finishing the vector types.
@ggetz is there anything here critical for 1.0?
Is there anyway to color based on whether the lat & lng are within a polygon?
@ronanmcnulty It sounds like classification might fit your use case better.
Thanks for the input, @pierotofy. We definitely want to support custom functions for expressions, and, I agree, will most likely go with the concise syntax. Thanks for the code snippets!
Were custom functions for expressions ever implemented @pjcozzi ? I'm attempting to color a point cloud by elevation but there's no height attribute for point clouds.
@ThornM9 no but there may be another way to achieve your use case, the best place to ask is the Cesium forum: https://groups.google.com/forum/?hl=en#!forum/cesium-dev
I noticed that atan2(y,x) is more inaccurate than atan(y/x). (While atan(y/x) only works for x > 0 of course). In my particular use case I used the atan2 function to compute the longitude of points in the tileset.
The resulting longitude from atan2(y, x) was off by about 0.00001 (ca. 10 meters), while it looked perfect with atan(y/x).
Unfortunately I don't know how to tell you the exact resulting numbers since they are only in the GPU I guess, I can only give this estimate from what it looks like.
My cesium version is 1.63, however this was reproducable in a sandcastle.
See https://github.com/CesiumGS/3d-tiles/issues/179 for optimizing large number of conditional expressions.
See https://github.com/CesiumGS/3d-tiles/issues/239 for batching vector images into a texture atlas that's part of the payload
See https://github.com/CesiumGS/3d-tiles/issues/252 for mutables (like GL uniforms)
See https://github.com/CesiumGS/3d-tiles/issues/266 for styling alpha separately
See https://github.com/CesiumGS/3d-tiles/issues/268 for accessing feature ids in the styling language
See https://github.com/CesiumGS/3d-tiles/issues/292 for accessing instanced attributes in the styling language
More considerations for 3D Tiles Next:
${classId.propertyId}
? Or only when there are collisions?${buildingType} === BuildingType.HOSPITAL
Short-Term
Spec
@ggetz
@pjcozzi
Cesium Implementation
@ggetz
BooleanExpression
,NumberExpression
, and friendsCesium3DTileStyle
(and implicitly forCesium3DTileStyleEngine
)styleEngine
last to constructors (and make it optional?)ColorRampExpression.computeEvenlySpacedIntervals
)@pjcozzi
Later
Spec
JulianDate
datatype to support styling with date/time metadataColor
so why not here?), see https://github.com/AnalyticalGraphicsInc/3d-tiles/issues/2#issuecomment-184332713${foo[${memberName}]}
distance
czm_time
,czm_inShadow
, etc.interval
to optimizeconditional
when used for intervalsProperty
system, e.g., evaluate expressions asProperty
or vice-versa.Cesium Implementation
Cesium3DTileStyle
and friends for users writing their own styles.replaceVariables
inExpression.js
?Built-in functions