Draft of support for vectors in binary and ternary expressions #5044

Jane-Of-Art · 2017-02-27T21:01:35Z

Draft of the changes I'm proposing for supporting vectors in all binary expressions (min, max, pow, atan2). Unfortunately I couldn't get my local machine to run and check everything yet so I will have to see if all new specs pass!

This is a part of ticket #4865

@lilleyse @Dylan-Brown

Jane-Of-Art · 2017-02-27T21:29:30Z

I'm not sure if it's my throw new DeveloperError notation that's wrong, but I don't see why all of my Error-throwing tests are failing :(

The good news is, properly-formated expression calls seem to all be passing!

  Scene/Expression
    ✗ throws if atan2 function takes invalid types / instances of arguments
	Expected function to throw DeveloperError.
	    at Specs/Scene/ExpressionSpec.js:2110:12
	    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)

	Expected function to throw DeveloperError.
	    at Specs/Scene/ExpressionSpec.js:2114:12
	    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)

	Expected function to throw DeveloperError.
	    at Specs/Scene/ExpressionSpec.js:2118:12
	    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)

    ✗ throws if pow function takes invalid types / instances of arguments
	Expected function to throw DeveloperError.
	    at Specs/Scene/ExpressionSpec.js:2152:12
	    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)

	Expected function to throw DeveloperError.
	    at Specs/Scene/ExpressionSpec.js:2156:12
	    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)

	Expected function to throw DeveloperError.
	    at Specs/Scene/ExpressionSpec.js:2160:12
	    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)

    ✗ throws if min function takes invalid types / instances of arguments
	Expected function to throw DeveloperError.
	    at Specs/Scene/ExpressionSpec.js:2196:12
	    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)

	Expected function to throw DeveloperError.
	    at Specs/Scene/ExpressionSpec.js:2200:12
	    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)

    ✗ throws if min function takes invalid types / instances of arguments
	Expected function to throw DeveloperError.
	    at Specs/Scene/ExpressionSpec.js:2236:12
	    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)

	Expected function to throw DeveloperError.
	    at Specs/Scene/ExpressionSpec.js:2240:12
	    at Object.<anonymous> (Specs/customizeJasmine.js:80:30)

Jane-Of-Art · 2017-02-27T21:54:03Z

@lilleyse

Dylan just took a look and couldn't figure out why these cases aren't throwing errors properly. Could you take a look at my syntax quickly?

Thanks!

lilleyse · 2017-02-27T21:56:37Z

Sure, I'll take a look at this later today.

lilleyse

This is an awesome start!

lilleyse · 2017-02-28T00:45:58Z

Specs/Scene/ExpressionSpec.js

+        expect(expression.evaluate(frameState, undefined).x).toEqualEpsilon(0.0, CesiumMath.EPSILON10);
+        expect(expression.evaluate(frameState, undefined).y).toEqualEpsilon(0.5 * Math.PI, CesiumMath.EPSILON10);
+        expect(expression.evaluate(frameState, undefined).z).toEqualEpsilon(0.25 * Math.PI, CesiumMath.EPSILON10);
+        expect(expression.evaluate(frameState, undefined).w).toEqualEpsilon(0.5 * Math.PI, CesiumMath.EPSILON10);


For each of these, just evaluate the expression once and do a toEqualEpsilon on a Cartesian, like evaluates pow function below.

lilleyse · 2017-02-28T00:50:37Z

Specs/Scene/ExpressionSpec.js

+
+        expression = new Expression('pow(vec4(5,4,3,2),vec4(0,2,3,5))');
+        expect(expression.evaluate(frameState, undefined)).toEqual(new Cartesian4(1.0, 16.0, 27.0, 32.0));
+


Remove empty line.

lilleyse · 2017-02-28T00:51:07Z

Specs/Scene/ExpressionSpec.js

@@ -2089,12 +2104,36 @@ defineSuite([
        }).toThrowDeveloperError();
    });

+    it('throws if atan2 function takes invalid types / instances of arguments', function() {


Just throws if atan2 function takes mismatching types is fine.

Same comment for the other tests.

lilleyse · 2017-02-28T00:54:42Z

Specs/Scene/ExpressionSpec.js

+        }).toThrowDeveloperError();
+    });
+
+


Remove empty line.

lilleyse · 2017-02-28T01:03:00Z

Source/Scene/Expression.js

+        atan2 : { evaluate: Math.atan2, requireTypeMatch: true },
+        pow : { evaluate: Math.pow, requireTypeMatch: true },
+        min : { evaluate: Math.min, requireTypeMatch: false },
+        max : { evaluate: Math.max, requireTypeMatch: false }


While this is more automated, I think its slightly overbuilt considering there are only 4 function. It should be fine to check the call name in getEvaluateBinaryFunction and apply the special cases.

lilleyse · 2017-02-28T01:04:49Z

I'm not sure if it's my throw new DeveloperError notation that's wrong, but I don't see why all of my Error-throwing tests are failing :(

For those cases the error is thrown when the expression is evaluated, not when it's created. Take a look at https://github.com/AnalyticalGraphicsInc/cesium/blob/3d-tiles/Specs/Scene/ExpressionSpec.js#L815-L820 and others.

lilleyse · 2017-02-28T01:05:41Z

Also what errors are your receiving when running locally? Are you able to access the page at http://localhost:8080?

Edit: this sounds like a build issue - if you run npm run build does it work?

lilleyse · 2017-03-14T14:08:30Z

@Jane-Of-Art any updates here?

Jane-Of-Art · 2017-03-15T05:53:41Z

@lilleyse Apologies, we were on spring break last week! All rested now and will update this ticket later this week :)

Jane-Of-Art · 2017-03-20T19:07:57Z

@lilleyse

Vector support for binary and ternary should all be here now! Your comments have been addressed and all tests should work.

The tests now run locally as well, so thanks for the npm run build suggestion :)

Jane-Of-Art · 2017-03-20T19:28:27Z

@lilleyse

As far as I can tell, the build errors are all rendering related, and the ExpressionSpec has passed. Can you tell if it's an error in the files that I edited?

Thanks!
Jane

lilleyse · 2017-03-20T19:49:18Z

One of those errors should be fixed in #5100, but yeah they seem unrelated to this code.

I'll review soon.

lilleyse

Looks good. I just left some small style suggestions.

lilleyse · 2017-03-21T13:53:16Z

Specs/Scene/ExpressionSpec.js

@@ -2143,6 +2292,19 @@ defineSuite([
        }).toThrowDeveloperError();
    });

+    it('throws if min function takes mismatching types', function() {


min -> max

Changed min -> max in the correct location below. I believe it should be min here

lilleyse · 2017-03-21T13:53:27Z

Specs/Scene/ExpressionSpec.js

+        }).toThrowDeveloperError();
+    });
+
+


Remove empty line.

lilleyse · 2017-03-21T13:53:49Z

Source/Scene/Expression.js

        };
    }

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


Remove empty line.

Jane-Of-Art · 2017-03-27T18:19:44Z

@lilleyse @Dylan-Brown

Comments fixed, functionality for length and normalize added. I can delete the length functionality this week if you prefer the other implementation, but Dylan wanted me to implement it this week and I was pretty much done so I figured I'd include it for now.

Also, I noticed that there were shaderExpression tests at the bottom of the expressionSpec. Is that something we should worry about writing?

Thanks!
Jane

Jane-Of-Art · 2017-03-27T18:34:26Z

Specs/Scene/ExpressionSpec.js

@@ -2011,11 +2011,61 @@ defineSuite([

    it('throws if fract function takes an invalid number of arguments', function() {
        expect(function() {
-            return new Expression('log2()');
+            return new Expression('fract()');
        }).toThrowDeveloperError();


Fixed what I believe was a copy & paste error here

Ah yes, thanks for fixing that!

Ah... looks like this was already fixed in 3d-tiles actually.

lilleyse · 2017-03-27T20:01:28Z

Source/Scene/Expression.js

+            return 1.0;
+        } else if (arg instanceof Cartesian2) {
+            length = Cartesian2.magnitude(arg);
+            return Cartesian2.fromElements(arg.x / length, arg.y / length, ScratchStorage.getCartesian2());


Use Cartesian2.normalize instead.

lilleyse · 2017-03-27T20:02:24Z

Specs/Scene/ExpressionSpec.js

@@ -2011,11 +2011,61 @@ defineSuite([

    it('throws if fract function takes an invalid number of arguments', function() {
        expect(function() {
-            return new Expression('log2()');
+            return new Expression('fract()');
        }).toThrowDeveloperError();


Ah yes, thanks for fixing that!

lilleyse · 2017-03-27T20:04:13Z

Specs/Scene/ExpressionSpec.js

@@ -2011,11 +2011,61 @@ defineSuite([

    it('throws if fract function takes an invalid number of arguments', function() {
        expect(function() {
-            return new Expression('log2()');
+            return new Expression('fract()');
        }).toThrowDeveloperError();


Ah... looks like this was already fixed in 3d-tiles actually.

lilleyse · 2017-03-27T20:18:22Z

I can delete the length functionality

It's fine to take yours since you already finished it, I didn't know the other PR would come so soon. I like how length is separated out, and it makes me think we could do that for a lot of the special case functions, not just here but in #5040 .

Also, I noticed that there were shaderExpression tests at the bottom of the expressionSpec. Is that something we should worry about writing?

Yeah I forgot about that, please add tests there as well.

@glee2244 these comments also apply to #5040

Jane-Of-Art · 2017-04-03T18:45:23Z

@lilleyse

Everything should be good now! Should I resolve the conflicts? These mostly arise from the fact that length is in there twice.

lilleyse · 2017-04-03T18:48:41Z

Yes resolve the conflicts, just keep your versions.

lilleyse · 2017-04-13T21:40:54Z

@Jane-Of-Art Still ready to go, just fix the merge conflicts.

Jane-Of-Art · 2017-04-24T18:42:00Z

@lilleyse Rebased code!

lilleyse · 2017-04-25T14:57:10Z

I think something went wrong, the PR now has a lot of extraneous files. Instead of a rebase could you try a merge? Or cherry-pick your commits on top of origin/3d-tiles and force push.

lilleyse · 2017-04-28T13:20:32Z

Also sorry to add more work here, but #5040 is now merged and introduced some conflicts.

pjcozzi · 2017-05-10T00:30:48Z

@Jane-Of-Art @lilleyse what is the plan here? Do we want to get this into 3d-tiles soon so we get that branch in shape for master?

…, added shader tests

lilleyse · 2017-05-12T20:39:55Z

@Jane-Of-Art hopefully you don't mind but I fixed up the PR and fixed the merge conflicts. As before the code looks good, so this is ready to merge.

lilleyse reviewed Feb 28, 2017

View reviewed changes

Jane-Of-Art changed the title ~~Draft of support for vectors in binary expressions~~ Draft of support for vectors in binary and ternary expressions Mar 20, 2017

lilleyse reviewed Mar 21, 2017

View reviewed changes

Jane-Of-Art commented Mar 27, 2017

View reviewed changes

lilleyse reviewed Mar 27, 2017

View reviewed changes

lilleyse mentioned this pull request May 10, 2017

3D Tiles #5308

Merged

23 tasks

Jane-Of-Art added 5 commits May 12, 2017 16:15

Draft of support for vectors in binary expressions

67cc4c6

Fixed type mismatch in specs and cartesian3 bug in function

a960877

Fixed developer error tests; added vector support for ternary functions

e8ad61a

Fixed minor styling issues I noticed

7af5769

Bug Fix ternary

1b4dafc

Jane-Of-Art and others added 4 commits May 12, 2017 16:17

Changed Developer Error Message

3490b93

Resolved merge conflicts, keeping my implementation of length

b009b1f

changed implementation of normalize to ue default cartesian functions…

04ea999

…, added shader tests

Merge branch '3d-tiles' into binary-temp

a5c70e7

lilleyse force-pushed the binary branch from 4909bea to a5c70e7 Compare May 12, 2017 20:37

lilleyse merged commit abf3864 into CesiumGS:3d-tiles May 12, 2017


		expression = new Expression('pow(vec4(5,4,3,2),vec4(0,2,3,5))');
		expect(expression.evaluate(frameState, undefined)).toEqual(new Cartesian4(1.0, 16.0, 27.0, 32.0));

Draft of support for vectors in binary and ternary expressions #5044

Draft of support for vectors in binary and ternary expressions #5044

Conversation

Jane-Of-Art commented Feb 27, 2017

Jane-Of-Art commented Feb 27, 2017

Jane-Of-Art commented Feb 27, 2017

lilleyse commented Feb 27, 2017

lilleyse left a 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

lilleyse commented Feb 28, 2017

lilleyse commented Feb 28, 2017 • edited Loading

lilleyse commented Mar 14, 2017

Jane-Of-Art commented Mar 15, 2017

Jane-Of-Art commented Mar 20, 2017

Jane-Of-Art commented Mar 20, 2017

lilleyse commented Mar 20, 2017

lilleyse left a 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

Jane-Of-Art commented Mar 27, 2017

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 Mar 27, 2017

Jane-Of-Art commented Apr 3, 2017

lilleyse commented Apr 3, 2017

lilleyse commented Apr 13, 2017

Jane-Of-Art commented Apr 24, 2017

lilleyse commented Apr 25, 2017 • edited Loading

lilleyse commented Apr 28, 2017

pjcozzi commented May 10, 2017

lilleyse commented May 12, 2017

lilleyse commented Feb 28, 2017 •

edited

Loading

lilleyse commented Apr 25, 2017 •

edited

Loading