First commit with distance #5040

glee2244 · 2017-02-27T18:24:41Z

I've attempted to include distance in the styling language for vectors. But I'm still confused as to how Cartesian3.distance() could work when the inputs could be anything other than type vec3. I've also tried to run my tests on my local server but the SpecRunner doesn't seem to display anything.
Issue #4865

@Dylan-Brown

lilleyse · 2017-02-28T01:12:01Z

@glee2244 in the future make a branch off of 3d-tiles and do your work there, this will make it easier to sync up your 3d-tiles branch with the origin.

lilleyse · 2017-02-28T01:28:31Z

I've attempted to include distance in the styling language for vectors. But I'm still confused as to how Cartesian3.distance() could work when the inputs could be anything other than type vec3.

Ok yeah I see the problem now, distance, dot, and cross will need to be handled differently than the other binary functions because they are geometric operations / don't work component-wise like the others.

Probably the best thing to do is be consistent with what's already there and set distance to point to a local function. (Example: https://github.com/AnalyticalGraphicsInc/cesium/blob/3d-tiles/Source/Scene/Expression.js#L107-L116). The function will just compute the distance between two numbers.

Then inside getEvaluateBinaryFunction there will need to be a special case for distance. It will be a separate block that looks similar to the if/else already there. For the number/number check, use the evaluate function that you set above. For the vector cases, use Cartesian2/3/4.distance on the left and right.

lilleyse · 2017-02-28T01:29:27Z

I've also tried to run my tests on my local server but the SpecRunner doesn't seem to display anything.

You may just need to build, try running npm run build. This is important whenever switching between branches that have different files, like going between master and 3d-tiles.

lilleyse · 2017-03-14T14:09:40Z

@glee2244 Do you have enough to go with here?

glee2244 · 2017-03-18T21:26:51Z

@lilleyse Yes, thanks so much for the pointers!

glee2244 · 2017-03-19T22:32:45Z

@lilleyse Should I be throwing a Developer Error if the distance function is called with mismatching inputs? For example, calling the distance between a number and a vec3?

lilleyse · 2017-03-20T01:53:47Z

Yes, throw an error for those cases.

glee2244 · 2017-03-23T20:35:15Z

@lilleyse I've pushed the changes as you've suggested, and it seems like the tests I've written pass, but I still get a failed check - Travis CI Build failed? Any ideas here?

lilleyse · 2017-03-23T20:36:36Z

There are some unrelated test failures in the 3d-tiles branch right now, just ignore for now.

glee2244 · 2017-03-23T22:16:01Z

@lilleyse Alright, duly noted. I'm going to continue with implementing dot and cross.

lilleyse

Looks good so far!

It's likely that #5044 will be merged first, so there may be some merge conflicts in getEvaluateBinaryFunction.

lilleyse · 2017-03-24T21:39:57Z

Source/Scene/Expression.js

+    isArray,
+    CesiumMath,
+    jsep,
+    ExpressionNodeType) {


Undo this indentation changes in here and ExpressionSpec.js.

However the rest of the formatting fixes seem fine.

lilleyse · 2017-03-24T21:51:09Z

Specs/Scene/ExpressionSpec.js

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

+    it ('evaluates the distance function', function() {


it ( -> it( here and below.

lilleyse · 2017-03-24T21:53:40Z

Specs/Scene/ExpressionSpec.js

+            return new Expression('distance(1, 3, 0)');
+        }).toThrowDeveloperError();
+    });
+


Also adds tests for mismatching types.

glee2244 · 2017-03-27T03:09:20Z

Pushing my implementations to dot and cross, but some of my tests are failing - will resolve them within the next couple days.

glee2244 · 2017-04-03T19:54:17Z

@lilleyse Is cross meant to only take two parameters? I was trying to call Cartesian3.cross with two parameters but just realized that the function I wanted to use takes 3 (the third being the object onto which to store the result). https://cesiumjs.org/Cesium/Build/Documentation/Cartesian3.html?classFilter=Carte

lilleyse · 2017-04-03T20:14:48Z

For that you can send in ScratchStorage.getCartesian3()

glee2244 · 2017-04-09T17:37:27Z

@lilleyse HI Sean, I've fixed my tests and some indentation/spacing that you pointed out before, and I've also refactored my implementations slightly. Let me know if there's anything else I should fix.

lilleyse

The updates look good!

lilleyse · 2017-04-13T21:10:17Z

Source/Scene/Expression.js

@@ -123,6 +126,44 @@ define([

    function log2(number) {
        return CesiumMath.logBase(number, 2.0);
+


Remove empty line.

lilleyse · 2017-04-13T21:10:58Z

Source/Scene/Expression.js

+        } else {
+            throw new DeveloperError('Function dot requires matching types of inputs. Arguments are ' + x + ' and ' + y + '.');
+        }
+


Remove empty line.

lilleyse · 2017-04-13T21:11:44Z

Source/Scene/Expression.js

+        if (typeof x === 'number' && typeof y === 'number') {
+            return Math.abs(x - y);
+        } else if (x instanceof Cartesian2 && y instanceof Cartesian2) {
+            return Cartesian2.distance(x ,y);


Formatting fix: (x ,y) -> (x, y)

lilleyse · 2017-04-13T21:16:33Z

Source/Scene/Expression.js

+        } else if (x instanceof Cartesian4 && y instanceof Cartesian4) {
+            return Cartesian4.distance(x, y);
+        } else {
+            throw new DeveloperError('Function distance requires matching types of inputs. Arguments are ' + x + ' and ' + y + '.');


Wrap each of these three DeveloperError with

//>>includeStart('debug', pragmas.debug); throw new DeveloperError('.....') //>>includeEnd('debug');

so that it is not included in release builds.

lilleyse · 2017-04-13T21:25:26Z

Source/Scene/Expression.js

            case ExpressionNodeType.LITERAL_UNDEFINED:
                //>>includeStart('debug', pragmas.debug);
                throw new DeveloperError('Error generating style shader: undefined is not supported.');
-                //>>includeEnd('debug');
+            //>>includeEnd('debug');


Remove the indentation changes here.

lilleyse · 2017-04-13T21:27:13Z

Specs/Scene/ExpressionSpec.js

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


Fix formatting for }) .toThrowDeveloperError();

lilleyse · 2017-04-13T21:28:02Z

Specs/Scene/ExpressionSpec.js

+
+    it('throws if distance function takes mismatching types of arguments', function() {
+        expect(function() {
+            return new Expression('distance(1, vec2(3.0, 2.0)').evaluate(frameState,undefined);


Style: frameState,undefined to frameState, undefined for this and others.

lilleyse · 2017-04-13T21:29:25Z

Specs/Scene/ExpressionSpec.js

+        }).toThrowDeveloperError();
+    });
+
+    it('throws if cross function takes mismatching types of arguments', function() {


Rename to throws if cross function does not take vec3 arguments

lilleyse · 2017-04-13T21:29:35Z

Specs/Scene/ExpressionSpec.js

@@ -2167,6 +2266,7 @@ defineSuite([
        }).toThrowDeveloperError();
    });

+


Remove empty line.

lilleyse · 2017-04-13T21:31:21Z

Specs/Scene/ExpressionSpec.js

@@ -3016,7 +3116,7 @@ defineSuite([
        expect(shaderExpression).toEqual(expected);
    });

-     it('gets shader expression for sin', function() {
+    it('gets shader expression for sin', function() {
        var expression = new Expression('sin(0.0)');
        var shaderExpression = expression.getShaderExpression('', {});
        var expected = 'sin(0.0)';


Can you also add tests for distance, dot, and cross for the shader expressions? Similar to

it('gets shader expression for min', function() { var expression = new Expression('min(3.0,5.0)'); var shaderExpression = expression.getShaderExpression('', {}); var expected = 'min(3.0, 5.0)'; expect(shaderExpression).toEqual(expected); });

glee2244 · 2017-04-17T03:49:36Z

Have added shader expression tests for distance, dot and cross and fixed spacing/indentation as requested, except for reverting indentation for includes here - for some odd reason the indentation keeps going back to incorrect spacing even after I update and save it. Will try to resolve asap.

lilleyse · 2017-04-18T13:33:38Z

Source/Scene/Expression.js

+        if (x instanceof Cartesian3 && y instanceof Cartesian3) {
+            return Cartesian3.cross(x, y, ScratchStorage.getCartesian3());
+        } else {
+            throw new DeveloperError('Invalid argument type for cross() function, must be vec3');


Wrap this with //>>includeStart('debug', pragmas.debug); and //>>includeEnd('debug'); too

lilleyse · 2017-04-18T13:39:50Z

Source/Scene/Expression.js

@@ -1623,7 +1672,7 @@ define([
            case ExpressionNodeType.FUNCTION_CALL:
                //>>includeStart('debug', pragmas.debug);
                throw new DeveloperError('Error generating style shader: "' + value + '" is not supported.');
-                //>>includeEnd('debug');
+            //>>includeEnd('debug');


Undo all these indentations as well.

lilleyse

Very close...

lilleyse · 2017-04-24T12:29:15Z

Source/Scene/Expression.js

+        if (typeof x === 'number' && typeof y === 'number') {
+            return x * y;
+        } else if (x instanceof Cartesian2 && y instanceof Cartesian2) {
+            return Cartesian2.dot(x ,y);


Change to (x, y)

lilleyse · 2017-04-24T12:29:19Z

Source/Scene/PointCloud3DTileContent.js

@@ -872,6 +875,7 @@ define([
        var hasNormals = content._hasNormals;
        var hasBatchIds = content._hasBatchIds;
        var backFaceCulling = content._backFaceCulling;
+        //var normalShading = content._normalShading;


Remove this commented normalShading code.

lilleyse · 2017-04-25T19:02:46Z

Can you remove the normalShading code entirely? That should only belong in the other PR.

lilleyse · 2017-04-28T13:18:43Z

Thanks @glee2244!

First commit with distance

e25191e

Second try with distance

e6c9bcd

lilleyse reviewed Mar 24, 2017

View reviewed changes

Implementing dot and cross, fixing tests

0ba46d2

lilleyse mentioned this pull request Mar 27, 2017

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

Merged

glee2244 added 3 commits April 9, 2017 12:41

Fixed cross tests

7fa0645

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

577f341

Fixed indentation and spacing

ccaaad8

lilleyse reviewed Apr 13, 2017

View reviewed changes

Fixed indentation and added shader expression tests

02f0db3

Attempt to revert indentation

bc64040

lilleyse reviewed Apr 18, 2017

View reviewed changes

glee2244 added 2 commits April 20, 2017 15:06

Adding debug wrapping and fixing indentation

7d18df7

Initializing variables for normal shading

2044390

lilleyse reviewed Apr 24, 2017

View reviewed changes

Fixing spacing

66a149a

Removing normalShading - doesn't belong in vector styling support

38dc3e3

lilleyse merged commit 6c6562d into CesiumGS:3d-tiles Apr 28, 2017

lilleyse mentioned this pull request Apr 28, 2017

Draft of options to turn off normal shading for point clouds #5228

Merged

		@@ -123,6 +126,44 @@ define([

		function log2(number) {
		return CesiumMath.logBase(number, 2.0);

		@@ -2167,6 +2266,7 @@ defineSuite([
		}).toThrowDeveloperError();
		});

First commit with distance #5040

First commit with distance #5040

Conversation

glee2244 commented Feb 27, 2017 • edited Loading

lilleyse commented Feb 28, 2017

lilleyse commented Feb 28, 2017

lilleyse commented Feb 28, 2017

lilleyse commented Mar 14, 2017

glee2244 commented Mar 18, 2017

glee2244 commented Mar 19, 2017

lilleyse commented Mar 20, 2017

glee2244 commented Mar 23, 2017

lilleyse commented Mar 23, 2017

glee2244 commented Mar 23, 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

glee2244 commented Mar 27, 2017

glee2244 commented Apr 3, 2017

lilleyse commented Apr 3, 2017

glee2244 commented Apr 9, 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

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

glee2244 commented Apr 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

lilleyse commented Apr 25, 2017

lilleyse commented Apr 28, 2017

glee2244 commented Feb 27, 2017 •

edited

Loading