3D Tiles - Ternary Functions #4709

Dylan-Brown · 2016-12-01T21:01:21Z

PR for issue #4694
Code hasn't fully addressed issue; still need to do a few things

Evaluate if data driven approach is feasible for clamp and lerp / mix, given my comments in the issue
Insert an example into 3D Tiles Point Cloud Styling.html
Write tests in ExpressionSpec.js
Update 3d-tiles repo's Styling README.md

Dylan-Brown · 2016-12-05T04:56:48Z

@lilleyse I think there's an issue here with the typing; It's not recognizing the 'new Expression(...)', because on the nextline, it's saying something about not being able to get the shader expression for an undefined expression. I'm reading through Expression.js and seeing if there's anything that could hint at what's wrong, but so far I haven't found anything

lilleyse · 2016-12-05T13:49:05Z

The ternaryFunctions check isn't handled in parseCall, which may be the cause of that issue.

lilleyse · 2016-12-06T19:33:51Z

Source/Scene/Expression.js

+                createRuntimeAst(expression, args[1]),
+                createRuntimeAst(expression, args[2])
+            ];
+            return new Node(ExpressionNodeType.TERNARY, call, val);


Instead of sending the values this should assign the left, right, and test, similar to here: https://github.com/AnalyticalGraphicsInc/cesium/pull/4688/files#diff-113136b97bec6733c699e609394258e1R379

And place this block after the unary functions. You may want to wait for #4688 to be merged first to avoid conflicts.

lilleyse · 2016-12-06T19:34:52Z

Source/Scene/Expression.js

+    function getEvaluateTernaryFunction(call) {
+        var evaluate = ternaryFunctions[call];
+        return function(feature) {
+            return evaluate(this._left.evaluate(feature));


This needs to also evaluate right and test, like here: https://github.com/AnalyticalGraphicsInc/cesium/pull/4688/files#diff-113136b97bec6733c699e609394258e1R662

lilleyse · 2016-12-06T19:36:52Z

Source/Scene/Expression.js

-                } else if (value === 'cos') {
-                    return 'cos(' + left + ')';
-                } else if (value === 'sqrt') {
-                    return 'sqrt(' + left + ')';


Good catch fixing these.

lilleyse · 2016-12-06T19:37:48Z

Source/Scene/ExpressionNodeType.js

@@ -25,7 +25,8 @@ define([
        LITERAL_COLOR : 13,
        LITERAL_REGEX : 14,
        LITERAL_UNDEFINED : 15,
-        LITERAL_GLOBAL : 16
+        LITERAL_GLOBAL : 16,
+        TERNARY : 17


Place this above BINARY and shift all the numbers.

lilleyse · 2016-12-06T19:38:59Z

Source/Core/Math.js

@@ -699,7 +699,7 @@ define([
    };

    /**
-     * Constraint a value to lie between two values.
+     * Constraints a value to lie between two values.


Constrains?

lilleyse · 2016-12-06T19:40:20Z

Source/Scene/Expression.js

-        degrees : CesiumMath.toDegrees
+        sin : Math.sin,
+        sqrt : Math.sqrt,
+        tan : Math.tan


This may need a bit organization just to group cos/sin/tan together and near the acos/asin/atan.

lilleyse · 2016-12-06T19:40:32Z

Source/Scene/Expression.js

+
+    var ternaryFunctions = {
+        clamp : CesiumMath.clamp,
+        mix: CesiumMath.lerp


Change to mix : CesiumMath.lerp.

lilleyse · 2016-12-06T19:41:02Z

Source/Scene/Expression.js

+            case ExpressionNodeType.TERNARY:
+                if (defined(ternaryFunctions[value])) {
+                    return value + '(' + left + ', ' + right + ', ' + test + ')';
+                }


Place this near the unary functions.

@lilleyse So, now the shader expression test cases are passing, but the test cases for 'evaluates clamp function' and 'evaluates mix function' don't make sense; for mix, "Expected NaN to equal 1." is the error message, and "DeveloperError: max is required." for clamp. These don't make sense to me, since three arguments are passed for clamp and mix doesn't seem to be getting any arguments, given that the result is NaN.

@lilleyse isExactClass, isClass, getExactClassName are functions that are also now failing in my code. The only other place I see this referernced is in PR #4625
Actually, nevermind, build is passing. Should be done!

lilleyse · 2016-12-12T14:32:37Z

Source/Scene/ExpressionNodeType.js

-        LITERAL_UNDEFINED : 15,
-        LITERAL_GLOBAL : 16
+        TERNARY : 2,
+        BINARY : 3,


Place ternary after binary.

lilleyse · 2016-12-12T14:32:42Z

Source/Scene/Expression.js

+    function getEvaluateTernaryFunction(call) {
+        var evaluate = ternaryFunctions[call];
+        return function(feature) {
+            return evaluate(this._left.evaluate(feature), this._right.evaluate(feature));


Should be:
return evaluate(this._left.evaluate(feature), this._right.evaluate(feature), this._test.evaluate(feature));

This should fix the errors here: https://github.com/AnalyticalGraphicsInc/cesium/pull/4709/files#r91228640

lilleyse · 2016-12-12T14:33:42Z

Source/Scene/Expression.js

@@ -647,6 +662,8 @@ define([
            if (node._value === 'TILES3D_TILESET_TIME') {
                node.evaluate = evaluateTime;
            }
+        } else if (node._type === ExpressionNodeType.TERNARY) {
+            node.evaluate = getEvaluateTernaryFunction(node._value);


Place after binary. Also can you put the unary before binary in this function - not caused by you but it's good for consistency.

This reverts commit b431c86, reversing changes made to 163bedf.

lilleyse · 2016-12-16T16:59:28Z

It lists 132 files as changed. Did you merge master into this at any point?

Dylan-Brown · 2016-12-16T17:02:19Z

@lilleyse I had merged from 3d-tiles, I believe, because I had a merge conflict... I'll try and revert the other changes

Dylan-Brown · 2016-12-16T17:28:12Z

@lilleyse I opened a new PR, #4761, that only includes one commit from me and is otherwise even with 3d-tiles. I must have merged from master in this one; apologies. I am closing this PR and I believe you will find the new one to contain all the code I've written for this one, but without the other files changed.

Dylan-Brown added 8 commits December 1, 2016 14:56

Merge remote-tracking branch 'upstream/3d-tiles' into ternary-functions

8c5dff0

updated Expression, Expression Node Type to include ternary functions.

7ec9aa7

removed ternary ops from unary ops array

567d840

grammar error in Math.js, Expression.js supporting ternary functions

3faddd6

tests for clamp, mix

bffea5d

tests for clamp, mix removed framestate

6ee74ec

updated expected in shader expression for clamp, mix

1ba1225

Merge remote-tracking branch 'upstream/3d-tiles' into ternary-functions

0f7f5c3

parseCall handling

e618d81

lilleyse reviewed Dec 6, 2016

View reviewed changes

Dylan-Brown added 3 commits December 6, 2016 23:24

corrections to some cosmetic comments

273edaf

merge upstream

8aae757

functions evaluating

02343dd

lilleyse reviewed Dec 12, 2016

View reviewed changes

lilleyse mentioned this pull request Dec 15, 2016

3D Tiles - Support vectors in the styling language #4759

Merged

2 tasks

Dylan-Brown added 4 commits December 15, 2016 16:54

Fixed evaluation function

163bedf

merge conflicts resolved in Expression.js

b431c86

Revert "merge conflicts resolved in Expression.js"

4927aa5

This reverts commit b431c86, reversing changes made to 163bedf.

Style examples for mix, lerp

1a510cb

Dylan-Brown mentioned this pull request Dec 16, 2016

3D Tiles - Ternary Functions (fixed) #4761

Merged

Dylan-Brown closed this Dec 16, 2016

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3D Tiles - Ternary Functions #4709

3D Tiles - Ternary Functions #4709

Dylan-Brown commented Dec 1, 2016 •

edited

Loading

Dylan-Brown commented Dec 5, 2016

lilleyse commented Dec 5, 2016

lilleyse Dec 6, 2016

lilleyse Dec 6, 2016

lilleyse Dec 6, 2016

lilleyse Dec 6, 2016

lilleyse Dec 6, 2016

lilleyse Dec 6, 2016

lilleyse Dec 6, 2016

lilleyse Dec 6, 2016

lilleyse Dec 6, 2016

Dylan-Brown Dec 7, 2016 •

edited

Loading

Dylan-Brown Dec 15, 2016 •

edited

Loading

lilleyse Dec 12, 2016

lilleyse Dec 12, 2016

lilleyse Dec 12, 2016

lilleyse Dec 12, 2016

lilleyse commented Dec 16, 2016

Dylan-Brown commented Dec 16, 2016

Dylan-Brown commented Dec 16, 2016

3D Tiles - Ternary Functions #4709

3D Tiles - Ternary Functions #4709

Conversation

Dylan-Brown commented Dec 1, 2016 • edited Loading

Dylan-Brown commented Dec 5, 2016

lilleyse commented Dec 5, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Dylan-Brown Dec 7, 2016 • edited Loading

Choose a reason for hiding this comment

Dylan-Brown Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Dec 16, 2016

Dylan-Brown commented Dec 16, 2016

Dylan-Brown commented Dec 16, 2016

Dylan-Brown commented Dec 1, 2016 •

edited

Loading

Dylan-Brown Dec 7, 2016 •

edited

Loading

Dylan-Brown Dec 15, 2016 •

edited

Loading