3d tiles unary ops #4628

Dylan-Brown · 2016-11-10T00:34:33Z

…ians

pjcozzi · 2016-11-10T19:08:24Z

Apps/Sandcastle/gallery/3D Tiles Point Cloud Styling.html

-addStyle('Cos', {
-    color : "color() * cos(${temperature} - 0)"
+addStyle('Trigonometric Functions', {
+    color : "color() * radians(cos(${temperature} - 0)) + color() * sin(${temperature} - 0) + color() * tan(${temperature} - 0)"


Why the - 0s?

I figured they would just be easier to tweak, but I'll remove them

pjcozzi · 2016-11-10T19:08:35Z

Apps/Sandcastle/gallery/3D Tiles Point Cloud Styling.html

-    color : "color() * cos(${temperature} - 0)"
+addStyle('Trigonometric Functions', {
+    color : "color() * radians(cos(${temperature} - 0)) + color() * sin(${temperature} - 0) + color() * tan(${temperature} - 0)"
+


Minor, but extra whitespace

I'm not sure where I should put it.

@pjcozzi means just to remove it.

pjcozzi · 2016-11-10T19:09:31Z

Source/Scene/Expression.js

+        acos : Math.acos,
+        asin : Math.asin,
+        atan : Math.atan,
+        radians : function(left) {


Here and below use the Cesium math functions: https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Core/Math.js#L419-L440

When I write
radians : CesiumMath.toRadians(), degrees : CesiumMath.toDegrees()
JSHint informs me that CesiumMath is undefined. I see the functions are defined in the file there. How do I reference them? Maybe this is a javascript thing I don't yet understand

Make sure CesiumMath is included correctly, check out https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Specs/Core/Cartesian4Spec.js for example.

Also it should be radians : CesiumMath.toRadians, degrees : CesiumMath.toDegrees

alright finally done

lilleyse · 2016-11-10T19:12:00Z

Source/Scene/Expression.js

@@ -346,7 +363,7 @@ define([
            //>>includeEnd('debug');
            val = createRuntimeAst(expression, args[0]);
            return new Node(ExpressionNodeType.UNARY, call, val);
-        } else if (call === 'cos') {
+        } else if (defined(unaryFunctions[call])) {


Remove the abs section above.

lilleyse · 2016-11-10T19:12:06Z

Source/Scene/Expression.js

}
+

Remove this empty line so that the else if below is attached, even though it is wrapped with a pragma.

lilleyse · 2016-11-10T19:12:11Z

Source/Scene/Expression.js

+            return evaluate(this._left.evaluate(feature));
+        }
+    }
+


Place this function closer to the other evaluate functions. Right above evaluateLiteral is fine.

lilleyse · 2016-11-10T19:12:15Z

Specs/Scene/ExpressionSpec.js

+        }).toThrowDeveloperError();
+    });
+
+    t('evaluates atan function', function() {


Should be it.

lilleyse · 2016-11-10T19:12:19Z

Specs/Scene/ExpressionSpec.js

+
+    it('evaluates radians function', function() {
+        var expression = new Expression('radians(0)');
+        expect(expression.evaluate(undefined)).toEqual(0);


To improve this test use a value instead of 0. You can build the style string to support PI - like 'radians(' + Math.PI + ')'

You can use radians(PI) now: CesiumGS/3d-tiles#151

You want me to convert radians to radians?

Ah sorry, this comment applies to the degrees function. Also the degrees comment applies here.

lilleyse · 2016-11-10T19:12:23Z

Specs/Scene/ExpressionSpec.js

+
+    it('evaluates degrees function', function() {
+        var expression = new Expression('degrees(0)');
+        expect(expression.evaluate(undefined)).toEqual(0);


Also tweak this test to something like degrees(360), toEqual(Math.PI). If floating precision is an issue, you can use toEqualEpsilon.

Whoops, meant 2 * Math.PI

lilleyse · 2016-11-10T19:14:00Z

Apps/Sandcastle/gallery/3D Tiles Point Cloud Styling.html

+});
+
+addStyle('Arc Trigonometric Functions', {
+    color : "color() * degrees(acos(${temperature} - 0)) + color() * asin(${temperature} - 0) + color() * atan(${temperature} - 0)"
 });


This test produces just a pure white, try tweaking the values a bit.

Also styling based on time is now possible with #4626 so you can incorporate that in some way.

lilleyse · 2016-11-14T21:24:00Z

Source/Scene/Expression.js

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


Missing semicolon

lilleyse · 2016-11-18T14:55:07Z

Apps/Sandcastle/gallery/3D Tiles Point Cloud Styling.html

+});
+
+addStyle('Arc Trigonometric Functions', {
+    color : "color() * degrees(acos(${temperature})) + color() * asin(${temperature}) + color() * atan(${temperature}) - 30"


These two samples still produce pure white. Try to tweak them a bit.

For the first one removing the +30 helps. For the second one both the -30 and the degrees are keeping it white. When the channels of the color are greater than 1.0 it will be white.

lilleyse · 2016-11-18T14:55:10Z

Source/Scene/Expression.js

+        asin : Math.asin,
+        atan : Math.atan,
+        radians : CesiumMath.toRadians(),
+        degrees : CesiumMath.toDegrees()


For this area make sure to address these comments:

#4628 (comment)

I've done as suggested, but I'm having trouble running a local copy of sandcastle so I'm trying to fix that now so I can come up with a better styling case

I redownloaded my repo and even though I'm on my 3d-tiles-unary-ops branch, the 3D tiles style sandcastle file is just gone, with a lot of others, even though they're showing up in my repo on github? I don't understand how that's happening

Is the html file gone or is the file just missing from the list in Sandcastle? If you haven't already make sure to do npm run build .

Something weird happened but removing and readding the directory fixed it. However, when I run npm install it fails:

npm ERR! Linux 3.13.0-79-generic
npm ERR! argv "/usr/bin/nodejs" "/usr/bin/npm" "install"
npm ERR! node v6.9.1
npm ERR! npm v3.10.8
npm ERR! code ELIFECYCLE
npm ERR! [email protected] postinstall: node install.js
npm ERR! Exit status 1
npm ERR!
npm ERR! Failed at the [email protected] postinstall script 'node install.js'.
npm ERR! Make sure you have the latest version of node.js and npm installed.
npm ERR! If you do, this is most likely a problem with the electron package,
npm ERR! not with npm itself.
npm ERR! Tell the author that this fails on your system:
npm ERR! node install.js
npm ERR! You can get information on how to open an issue for this project with:
npm ERR! npm bugs electron
npm ERR! Or if that isn't available, you can get their info via:
npm ERR! npm owner ls electron
npm ERR! There is likely additional logging output above.
npm ERR! Please include the following file with any support request:

I don't think I follow. You mean all the statements like "//>>includeEnd('debug')" ?

What you're doing in the screenshot is fine (though we alphabetize the order in the first first array instead of the second, so ../Core/Math should be after isArray). The code pushed to github has a bunch of unnecessary includes in it.

Ah I see you're newest commit, yeah that's what I meant.

One last thing, the CesiumMath in the second array needs to be at the same index.

Gotcha, changed and committed, waiting on Travis now

lilleyse · 2016-11-18T14:55:16Z

Source/Scene/Expression.js

+    Node.prototype._evaluateBooleanConversion = function(feature) {
+        return Boolean(this._left.evaluate(feature));
+    };
+


Remove this function, it is a duplicate of below. Also remove _evaluateAbsoluteValue, _evaluateCosine, _evaluateSquareRoot.

lilleyse · 2016-11-18T14:56:41Z

@Dylan-Brown this is really close, just these few more comments.

…esiumGS#4603

…going to commit small changes until the integration test passes CesiumGS#4603

lilleyse · 2016-11-18T20:59:10Z

In case you missed this comment, this should fix the failure: #4628 (comment)

Dylan-Brown · 2016-11-18T21:45:55Z

The error with the check is:
"Expected 20626.480624709635 to equal epsilon 6.283185307179586, 1e-10."
the test that fails is:
it('evaluates degrees function', function() { var expression = new Expression('degrees(360)'); expect(expression.evaluate(undefined)).toEqualEpsilon(2*Math.PI, CesiumMath.EPSILON10); });
I think this means there's something wrong with CesiumMath.toDegrees. Investigating now

EDIT: Nope. just relized I was converting 360 radians into degrees.

lilleyse · 2016-11-21T14:32:30Z

Specs/Scene/ExpressionSpec.js

+    });
+
+    it('evaluates degrees function', function() {
+        var expression = new Expression('degrees(' + 2*Math.PI + ')');


All else looks good, this should be my last comment - Now with PI supported in the styling language just change this back to 'degrees(2 * PI)'

lilleyse · 2016-11-21T16:53:17Z

Awesome I'm glad to merge this!

lilleyse · 2016-11-21T16:54:49Z

Next up just add these functions to https://github.com/AnalyticalGraphicsInc/3d-tiles.

pjcozzi · 2016-11-21T20:30:48Z

Nice work here, thanks again @Dylan-Brown!

Dylan-Brown added 8 commits November 6, 2016 14:35

added sin, tan CesiumGS#4603

ec8ff9e

added acos, asin, atan CesiumGS#4603

d4eaf60

small adjustment for 3d demo, separated trig, arc trig functions Cesi…

3c5b870

…umGS#4603

added radians, degrees, not to demo CesiumGS#4603

8bafd01

data driven CesiumGS#4603

68ac1a1

data driven CesiumGS#4603 removed excess functions, added degrees/rad…

c185368

…ians

forgot to delete a few lines CesiumGS#4603

9dd5f1e

styling examples CesiumGS#4603

5689d49

pjcozzi reviewed Nov 10, 2016

View reviewed changes

lilleyse reviewed Nov 10, 2016

View reviewed changes

Specs/Scene/ExpressionSpec.js

}).toThrowDeveloperError();

});

t('evaluates atan function', function() {

Copy link

Contributor

lilleyse Nov 10, 2016

Choose a reason for hiding this comment

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

Should be it.

lilleyse reviewed Nov 10, 2016

View reviewed changes

lilleyse reviewed Nov 14, 2016

View reviewed changes

Dylan-Brown added 4 commits November 15, 2016 15:45

fixed merge CesiumGS#4603

f951df0

updated based on suggestions CesiumGS#4603

31a3e76

removed abs, sqrt section CesiumGS#4603

0f11c24

updated conversion from 0 radians to pi CesiumGS#4603

0477a6f

lilleyse reviewed Nov 18, 2016

View reviewed changes

Dylan-Brown added 3 commits November 18, 2016 14:08

removed duplicate functions, imported CesiumMath, styling update(?) C…

4ee3b34

…esiumGS#4603

CesiumGS#4603 found math issue

9cd51cf

CesiumGS#4603 merge conflict resolved

24cd1be

Dylan-Brown added 2 commits November 18, 2016 15:17

includes? CesiumGS#5603

6c56dac

I really don't understand this define function and can't test so I'm …

d6fa4d7

…going to commit small changes until the integration test passes CesiumGS#4603

Dylan-Brown added 2 commits November 18, 2016 16:22

CesiumGS#4603 hope this works

91c6b69

updated test cases

4463339

Dylan-Brown added 3 commits November 18, 2016 16:52

nevermind

5d99266

alright

f94c4ce

alright pt. 2

341c363

lilleyse reviewed Nov 21, 2016

View reviewed changes

lilleyse mentioned this pull request Nov 21, 2016

3D Tiles - binary functions #4643

Closed

updated to reflect PI in the styling language

3b7f6e5

lilleyse merged commit d7f2aa6 into CesiumGS:3d-tiles Nov 21, 2016

Dylan-Brown deleted the 3d-tiles-unary-ops branch November 21, 2016 22:26

lilleyse mentioned this pull request Nov 30, 2016

3D Tiles binary functions #4688

Merged

3d tiles unary ops #4628

3d tiles unary ops #4628

Conversation

Dylan-Brown commented Nov 10, 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

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

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 Nov 18, 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Nov 18, 2016

lilleyse commented Nov 18, 2016

Dylan-Brown commented Nov 18, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Nov 21, 2016

lilleyse commented Nov 21, 2016

pjcozzi commented Nov 21, 2016

Dylan-Brown Nov 18, 2016 •

edited

Loading

Dylan-Brown commented Nov 18, 2016 •

edited

Loading