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

lilleyse · 2016-12-15T15:31:22Z

This adds support for vec2, vec3, and vec4 types in the styling language.

Under the hood all vector types are treated as Cartesian4, with missing components set to 0.0. Vector components may be accessed with .x, .y, .z, .w and [0], [1], [2], [3]. Colors components can also be accessed like this too in addition to .red, .green, .blue, and .alpha.

The following operators are supported for vectors:
Unary +, unary -, +, - *, /, %, ===, ==, !==, !=

The following operators are not supported:
<, <, >=, <=, &&, ||, !~, =~

To-do:

Support component-wise versions for many of the built-in functions. I'll start this once 3D Tiles - Ternary Functions #4709 and 3D Tiles - sign, floor, ceil, round, etc for styling language #4604 are complete.
Spec PR that will help finish off Add point cloud styling section 3d-tiles#138

pjcozzi · 2016-12-15T17:53:06Z

Source/Scene/Expression.js

@@ -928,6 +1029,10 @@ define([
        var right = this._right.evaluate(frameState, feature);
        if ((right instanceof Color) && (left instanceof Color)) {
            return Color.mod(left, right, ScratchStorage.getColor());
+        } else if ((right instanceof Cartesian4) && (left instanceof Cartesian4)) {
+            // TODO : make mod a built-in Cartesian function?


Up to you, but I'm not sure it is worth it until/less we have another use case since we would also have to add it to Cartesian2, Cartesian3, etc.

pjcozzi · 2016-12-15T17:54:42Z

Under the hood all vector types are treated as Cartesian4

The memory usage here is fine, but I am not thrilled with the potential compute overhead when evaluating expressions on the CPU. Perhaps don't worry about it now, but add a PERFORMANCE_IDEA comment.

pjcozzi · 2016-12-15T17:58:19Z

Since these look like GLSL types, can the spec please have a short section to explicitly say how the types are different than GLSL? For example, they do not support swizzling (and you'll have to decide if vec4(1.0), for example, is allowed; it is OK with me).

Also, does the GLSL implementation allow styles that are not valid, e.g., swizzling, since it is handed off to the GLSL compiler? If so, we should explicitly catch these cases.

pjcozzi · 2016-12-15T18:03:10Z

For the spec, what happens if the index is out of bounds when accessing the vector with array notation?

pjcozzi · 2016-12-15T18:04:23Z

Specs/Scene/ExpressionSpec.js

+        expect(expression.evaluate(frameState, undefined)).toEqual(0.5);
+    });
+
+    it('evaluates color properties (["red"], ["green"], ["blue"], ["alpha"])', function() {


What is the use case for supporting this? color[${CHANNEL}]?

This is for color('yellow')['red'] equivalent to color('yellow').red.

Here and below, I see what it does, I'm just asking why since GLSL does not allow this since it does not have strings. How would the GLSL backend handle color[${CHANNEL}]?

If there are compelling use cases, we can keep this.

I'm just treating ["red"] as a special case, but yeah other strings like color[${CHANNEL}] will not work. I am fine with disallowing ["red"] in GLSL for consistency.

I'm not sure what you mean by disallowing. I simply mean that arrays should be indexed with integers, not strings.

Accessing color components like this is supported in the spec:

Bracket notation ([]) is also used to access properties, e.g., color['red'], or arrays, e.g., temperatures[1].

Ah, right. OK, disallow and document for now.

pjcozzi · 2016-12-15T18:04:39Z

Specs/Scene/ExpressionSpec.js

+        expect(expression.evaluate(frameState, undefined)).toEqual(0.5);
+    });
+
+    it('evaluates color properties (["x"], ["y"], ["z"], ["w"])', function() {


Same comment.

For vec4(1,1,1,1)['x'] equivalent to vec4(1,1,1,1).x

pjcozzi · 2016-12-15T18:06:03Z

Specs/Scene/ExpressionSpec.js

+    });
+
+    it('evaluates expression with multiple nested vectors', function() {
+        var expression = new Expression('vec4(vec2(1, 2)[vec3(6, 1, 5).y], 2, vec4().w, 5)');


Also for the spec, is construction like vec3(vec2(1.0, 2.0), 3.0) allows? Up to you, just explicitly state it.

This is not allowed, all components must evaluate to numbers (which the example above does). I will make this clear in the spec.

pjcozzi · 2016-12-15T18:07:31Z

Specs/Scene/ExpressionSpec.js

+        expression = new Expression('-vec3(1, 2, 3)');
+        expect(expression.evaluate(frameState, undefined)).toEqual(new Cartesian4(-1, -2, -3, 0));
+
+        expression = new Expression('vec2(1, 2) + vec4(3, 4, 5, 6)');


Here and elsewhere, does GLSL allow this? I think this should be more strictly typed.

GLSL does not allow it by default - but all vector types are turned into vec4 so this is valid. There are a lot of complications in recognizing vec2, vec3, vec4 type mismatches otherwise. I will add a note in the spec that vec2, vec3, and vec4 are just different constructors for the vector type - which is always vec4.

I think you are letting this specific implementation too strongly influence the spec. Would a language really define that all vector types map to vec4?

There are a lot of complications in recognizing vec2, vec3, vec4 type mismatches...

What are they?

For the JS side it isn't too bad, just that operations between different Cartesian types will result in NaN. In GLSL the shader will not compile due to type mismatches (which cannot be verified ahead since properties are not known ... like color : ${id} > 10 ? vec3(1.0) : vec2(2.0)).

Having everything be the same type makes it easier to do operations between vector properties and literal vectors without needing to support special constructors like vec4(${rgbcolor}, 1.0).

What about dropping vec2 and vec3 and always using vec4?

What about dropping vec2 and vec3 and always using vec4?

We could see how much milage we get from this.

Remind me, why are we adding vector types at all right now? To flush out the math library with things like dot and cross products? If that is the case, don't we really need a vec3.

I am also still not convinced that we can't reasonably detect this even if some error messages have to be "Could not compile generated GLSL shader, this is often due to a vector type mismatch."

The vector types are needed to support vector properties in the batch table binary, which can be vec2, vec3, vec4, or accessing point positions with ${POSITION} as well as math operations. Having vec4 as the base type also makes it easier to interact with colors (should colors and vectors mix?).

In line with other vector libraries I can support vec2, vec3, and vec4 as distinct types - however it will add more complexity.

pjcozzi · 2016-12-15T18:08:16Z

Looks good, just those comments.

lilleyse · 2016-12-15T18:28:28Z

(and you'll have to decide if vec4(1.0), for example, is allowed; it is OK with me).

This is allowed, but it will result in vec4(1,0,0,0) instead of vec4(1,1,1,1). I'll make it clear in the spec that unset components default to 0.

Also, does the GLSL implementation allow styles that are not valid, e.g., swizzling, since it is handed off to the GLSL compiler? If so, we should explicitly catch these cases.

The GLSL implementation catches these cases and will throw a developer error.

For the spec, what happens if the index is out of bounds when accessing the vector with array notation?

Hm... good point - the current behavior is to return undefined in the JS backend and not compile in the GLSL backend.

pjcozzi · 2016-12-15T19:02:53Z

(and you'll have to decide if vec4(1.0), for example, is allowed; it is OK with me).

This is allowed, but it will result in vec4(1,0,0,0) instead of vec4(1,1,1,1). I'll make it clear in the spec that unset components default to 0.

As I mentioned above, I don't think this is a good idea at the spec level.

For the spec, what happens if the index is out of bounds when accessing the vector with array notation?

Hm... good point - the current behavior is to return undefined in the JS backend and not compile in the GLSL backend.

Can we be consistent? Can variables be passed at runtime as array indices? If not, it is probably easy enough to catch this at compile time with GLSL and map undefined to 0.0 or whatever we decided for GLSL.

lilleyse · 2016-12-16T19:17:45Z

As I mentioned above, I don't think this is a good idea at the spec level.

Do you mean the spec should have it become vec4(1,1,1,1) or that this decision is too specific for the spec?

Can we be consistent? Can variables be passed at runtime as array indices? If not, it is probably easy enough to catch this at compile time with GLSL and map undefined to 0.0 or whatever we decided for GLSL.

Yes variables can be passed at runtime so checking at compile-time is not possible.

pjcozzi · 2016-12-18T14:32:47Z

As I mentioned above, I don't think this is a good idea at the spec level.

Do you mean the spec should have it become vec4(1,1,1,1) or that this decision is too specific for the spec?

Yes, I would do vec4(1,1,1,1) like GLSL. The implicit mapping of all vector types to vec4 is just not at all common in the, I dunno, dozens of vector libraries I have seen. We should mirror GLSL as close as possible.

Can we be consistent? Can variables be passed at runtime as array indices? If not, it is probably easy enough to catch this at compile time with GLSL and map undefined to 0.0 or whatever we decided for GLSL.

Yes variables can be passed at runtime so checking at compile-time is not possible.

If the JavaScript and GLSL implementations need to be different, please record the details in CesiumGS/3d-tiles#140. We have to be very diligent and precise; otherwise, the styling ecosystem will not be robust.

lilleyse · 2016-12-19T16:10:09Z

Yes, I would do vec4(1,1,1,1) like GLSL. The implicit mapping of all vector types to vec4 is just not at all common in the, I dunno, dozens of vector libraries I have seen. We should mirror GLSL as close as possible.

Sounds good to me. I'll follow GLSL more closely.

Yes variables can be passed at runtime so checking at compile-time is not possible.
If the JavaScript and GLSL implementations need to be different, please record the details in CesiumGS/3d-tiles#140. We have to be very diligent and precise; otherwise, the styling ecosystem will not be robust.

Added a note to the list.

lilleyse · 2017-01-06T16:14:11Z

Update to support vec2, vec3, and vec4 as distinct types. The constructors follow the same rules as glsl, so some valid constructors are:

vec4(1)
vec3(1, 2, 3)
vec3(vec4(1, 2, 3, 4))
vec4(vec3(1, 2, 3), 4)
vec4(vec2(1, 2), vec2(3, 4))
etc...

pjcozzi · 2017-01-06T21:15:27Z

Do you want to move the tasklist in the first comment here to another roadmap? Merging this without finishing them is OK with me.

pjcozzi · 2017-01-06T21:19:36Z

Source/Scene/Expression.js

+        // For example vec3(1, 2, 3) or vec3(vec2(1, 2), 3) are both valid.
+        //
+        // If the number of components does not equal the vector's size, then a DeveloperError is thrown - with two exceptions:
+        // 1. A vector may be constructed from a larger vector and drop the extra components.


Is (1) also true in GLSL?

If not, is it hard/slow to implement the same behavior here?

Merging, we'll open a separate PR if this needs a code change.

Yeah (1) is true in GLSL.

lilleyse added 2 commits December 14, 2016 22:31

Support vectors in the styling language

12cbbfe

Typo

a5d9c37

pjcozzi reviewed Dec 15, 2016

View reviewed changes

Merge branch '3d-tiles' into vector

7726151

lilleyse force-pushed the vector branch from 4ace2fe to 1268f69 Compare January 6, 2017 16:09

Support vec2, vec3, and vec4 as distinct types

d76dade

lilleyse force-pushed the vector branch from 1268f69 to d76dade Compare January 6, 2017 16:22

pjcozzi reviewed Jan 6, 2017

View reviewed changes

pjcozzi merged commit fd76c00 into 3d-tiles Jan 6, 2017

pjcozzi deleted the vector branch January 6, 2017 21:24

This was referenced Jan 9, 2017

More unary fns for 3d-tiles #4775

Merged

Vector types: vec2, vec3, vec4 CesiumGS/3d-tiles#166

Merged

3D Tiles Styling - add vector support to the built-in functions #4865

Closed

This was referenced Mar 27, 2017

Options to turn off normal shading for point clouds #5152

Closed

Support multiple expressions in a 3D Tiles Style #5153

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

lilleyse commented Dec 15, 2016 •

edited

Loading

pjcozzi Dec 15, 2016

pjcozzi commented Dec 15, 2016

pjcozzi commented Dec 15, 2016

pjcozzi commented Dec 15, 2016

pjcozzi Dec 15, 2016

lilleyse Dec 15, 2016

pjcozzi Dec 15, 2016

lilleyse Dec 16, 2016

pjcozzi Dec 18, 2016

lilleyse Dec 19, 2016

pjcozzi Dec 19, 2016

pjcozzi Dec 15, 2016

lilleyse Dec 15, 2016

pjcozzi Dec 15, 2016

lilleyse Dec 15, 2016

pjcozzi Dec 15, 2016

lilleyse Dec 15, 2016

pjcozzi Dec 15, 2016

lilleyse Dec 16, 2016

pjcozzi Dec 18, 2016

lilleyse Dec 19, 2016 •

edited

Loading

pjcozzi commented Dec 15, 2016

lilleyse commented Dec 15, 2016

pjcozzi commented Dec 15, 2016

lilleyse commented Dec 16, 2016

pjcozzi commented Dec 18, 2016

lilleyse commented Dec 19, 2016

lilleyse commented Jan 6, 2017

pjcozzi commented Jan 6, 2017

pjcozzi Jan 6, 2017

pjcozzi Jan 6, 2017

lilleyse Jan 6, 2017

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

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

Conversation

lilleyse commented Dec 15, 2016 • edited Loading

Choose a reason for hiding this comment

pjcozzi commented Dec 15, 2016

pjcozzi commented Dec 15, 2016

pjcozzi commented Dec 15, 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

lilleyse Dec 19, 2016 • edited Loading

Choose a reason for hiding this comment

pjcozzi commented Dec 15, 2016

lilleyse commented Dec 15, 2016

pjcozzi commented Dec 15, 2016

lilleyse commented Dec 16, 2016

pjcozzi commented Dec 18, 2016

lilleyse commented Dec 19, 2016

lilleyse commented Jan 6, 2017

pjcozzi commented Jan 6, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Dec 15, 2016 •

edited

Loading

lilleyse Dec 19, 2016 •

edited

Loading