3D Tiles - Point Cloud Styling #4336

lilleyse · 2016-09-19T01:46:20Z

For #3241 and CesiumGS/3d-tiles#22 (comment)

Initial steps for point cloud styling based on shaders. I've tested a bunch of different styles and it seems to be working pretty well.

TODO:

Write tests.
Point Cloud Styling sandcastle
Think about styling properties like strings, regex, null, and undefined.
64 bit float, 32/64 bit integers (not strictly styling related)
Should the shading language be able to reference semantics like COLOR, or NORMAL?
Add point size to styling language

There are a few open issues:

What to do about vector and matrix types in the styling language? Vector properties are supported, but the only thing you can do right now is access their components. Matrix types aren't supported at all. Should all operations be component wise? Should arrays be treated as vec3's? There are a bunch of complications all around and it would be easiest to just to access components and not support vector math, but if needed I can get something working.
Since glsl is a lot stricter about types certain styles just won't work, like comparing floats to bools, doing operations on floats that are meant for bools (and the reverse). It's pretty hard to check what types the left and right side of an expression may be, except in simple cases. Right now valid styles may create broken shaders and it's hard to check before compiling the shader.
What is the best way to update the point cloud when the style changes. The current way works but there may be a cleaner solution.
Possibly discussed before, but should the show expression support a conditions list? I can understand why it wouldn't need to, since it's just true/false.
Should ${expression} be allowed in the expression part of a condition: like

var style = {
    color : {
        expression : "Number(${temperature} < 0.1)",
        conditions : {
            "${expression} === 1.0" : "rgb(${expression}*255, 0, 0)",
            "${expression} === 0.0" : "color('blue')"
        }
    }
};

Demo: http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/pnts-styling/Apps/Sandcastle/?src=Hello%20World.html&label=Showcases&gist=9d8bd5ba00d15b424da2e294cfa1a5c4

pjcozzi · 2016-09-20T00:48:46Z

@lilleyse merge in 3d-tiles. There must be minor conflicts from the recent RGB565 merge.

What to do about vector and matrix types in the styling language? Vector properties are supported, but the only thing you can do right now is access their components.

What do you want to do with them? GLSL-like functions, e.g., normalize? These are on the roadmap, but I don't think we need to do them as part of this PR unless you have a compelling use case.

Matrix types aren't supported at all.

Roadmap, unless you have a compelling use case.

Should all operations be component wise?

I'm confused; I thought you said you can only access their components. Are you sure we didn't implement +, etc. with the JavaScript back-end for declarative styling?

Should arrays be treated as vec3's?

No, think of declarative styling as closer to JavaScript than GLSL.

pjcozzi · 2016-09-20T01:02:56Z

Since glsl is a lot stricter about types certain styles just won't work, like comparing floats to bools, doing operations on floats that are meant for bools (and the reverse). It's pretty hard to check what types the left and right side of an expression may be, except in simple cases. Right now valid styles may create broken shaders and it's hard to check before compiling the shader.

The options are:

Leave it as is. Cesium technically won't follow the styling spec here. Most likely no one else would either.
Change the styling spec to be more strict with types.
(2) but only for point clouds.
Completely separate styling spec for point clouds.
Require a per-point CPU-backend for these cases. ha.

1 is the least work, but 2 is probably the right solution since it keeps the spec simple and doesn't fragment itself.

Just a warning that the styling spec updates may be non-trivial; a language spec is way harder than a format spec. Please deeply think through if just "types must match with absolutely no implicit conversion" limits any potentially common use cases.

pjcozzi · 2016-09-20T01:04:19Z

What is the best way to update the point cloud when the style changes. The current way works but there may be a cleaner solution.

I'll look, but I think just recompiling the shader for now is fine (which is what we do for the CPU styling implementation). Later, we could map some variables to uniforms for fast paths, which is how the original CPU styling implementation worked.

pjcozzi · 2016-09-20T01:06:55Z

Possibly discussed before, but should the show expression support a conditions list? I can understand why it wouldn't need to, since it's just true/false.

I think so. For complex conditions, wouldn't this let users break them over several checks? Just because the results is boolean doesn't mean we should limit how that boolean is computed.

pjcozzi · 2016-09-20T01:07:44Z

Should ${expression} be allowed in the expression part of a condition

Yes. Is this tested in our CPU implementation and documented well enough in the spec?

pjcozzi · 2016-09-20T01:12:57Z

Demo: http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/pnts-styling/Apps/Sandcastle/?src=Hello%20World.html&label=Showcases&gist=9d8bd5ba00d15b424da2e294cfa1a5c4

This is cool. Please clean it up and make it a separate "3D Tiles Point Cloud Styling" Sandcastle example; I think it would be too much to combine with the 3D Tiles example.

pjcozzi · 2016-09-20T01:13:18Z

Do you have the styling code example for the screenshot at the top of this PR?

pjcozzi · 2016-09-20T01:16:27Z

Source/Scene/Cesium3DTileStyle.js

+     *
+     * @param {String} name Name to give to the generated function.
+     * @param {String} variablePrefix Prefix that is added to any variable names to access vertex attributes.
+     * @param {Object} info Stores information about the generated shader function.


Can we rename info. From this context, I have no idea what it is.

pjcozzi · 2016-09-20T01:18:26Z

Source/Scene/Cesium3DTileStyle.js

+     *
+     * @param {String} name Name to give to the generated function.
+     * @param {String} variablePrefix Prefix that is added to any variable names to access vertex attributes.
+     * @param {Object} info Stores information about the generated shader function.


Same comment.

pjcozzi · 2016-09-20T01:21:08Z

Source/Scene/Cesium3DTileStyleEngine.js

        var length = content.featuresLength;
        var style = styleEngine._style;

        stats.numberOfFeaturesStyled += length;

+        // Apply style to point cloud. Only apply style if the point cloud is not backed by a batch table.
+        if ((content instanceof PointCloud3DTileContent) && (length === 0)) {
+            content.applyStyle(frameState, style);


Perhaps rework this function so that each content type has a function like applyStyleWithShader that returns a boolean. If it returns false then the style was not applied and the loop below can be used. This avoids the special cases and reduces coupling.

pjcozzi · 2016-09-20T01:25:48Z

Source/Scene/Expression.js

+            case ExpressionNodeType.CONDITIONAL:
+                return '(' + test + ' ? ' + left + ' : ' + right + ')';
+            case ExpressionNodeType.MEMBER:
+                // This is intended for accessing the components of vec2, vec3, and vec4 properties. String members aren't supported.


@hobu are strings common per-point metadata?

@lilleyse if so, I think we need to put the characters into textures to support this. If not, it may be reasonable for the spec to disallow strings in styles for point clouds.

pjcozzi · 2016-09-20T01:27:23Z

Source/Scene/Expression.js

+                }
+                break;
+            default:
+                // Not supported: FUNCTION_CALL, ARRAY, REGEX, VARIABLE_IN_STRING, LITERAL_NULL, LITERAL_REGEX, LITERAL_UNDEFINED


Some of these can probably be explicitly disallowed by the spec (I suppose we can't get around this since any sane point cloud styling engine will use a shader) like regex, but please mark this in the task list so we can think through it before merging.

pjcozzi · 2016-09-20T01:29:04Z

Source/Scene/PointCloud3DTileContent.js

+        this._hasNormals = false;
+        this._hasBatchIds = false;
+
+        // TODO : How to expose this? Will this be part of the point cloud styling or a property of the tileset?


Doesn't need to be part of this PR; we can determine this when we also consider point attenuation, blurring, etc., which will have the same questions.

Ah, old TODO.

pjcozzi

I'll do a more detailed review before merging, but this is off to a nice start.

pjcozzi · 2016-09-20T01:32:48Z

Source/Scene/PointCloud3DTileContent.js

+            attributeLocations[attributeName] = attribute.location;
+        }
+
+        var vs = 'attribute vec3 a_position; \n' +


Can we combine all the shader generation code with the similar code above?

Addressed in b7c2dfd, though I'm not sure if you had something else in mind.

pjcozzi · 2016-09-20T18:54:10Z

When generating the shaders we should optimize out all constants at "compile time" mean that expressions like rgba(255, 255, 0, 0.1) that contain all literals should be evaluated and then hardcoded into the shader so they are not needlessly evaluated per-vertex per-frame.

lilleyse · 2016-09-28T17:31:41Z

I made a few changes to the type comparisons:

Operator	Type
!	booleans
+ and - (unary)	numbers
< <= > >=	numbers
+ (binary)	both numbers or both strings
- * / %	numbers
isNan isFinite	numbers
logical and/or	no change - still booleans
=== !==	no change - any type allowed

It seems too restrictive to force the equality comparisons to be the same type, so in this case the shader styling would not conform to the spec.

lilleyse · 2016-09-28T17:47:23Z

I made some changes to how scratch colors are used in the evaluate functions in Expression.js in 758ebb2 with slight modifications in other commits. There were some problems where multiple colors would write to the same scratch variables. The new solution manages an array of scratch variables to take care of that.

5cb1088 fixes CesiumGS/3d-tiles#2 (comment)

The vector math situation is a little better now. I allow arrays to be converted to vec2, vec3, or vec4 to support some vector math.

Will update soon with tests and a Point Cloud Styling sandcastle.

pjcozzi · 2016-09-29T11:57:23Z

I made a few changes to the type comparisons...

Looks good, except:

It seems too restrictive to force the equality comparisons to be the same type, so in this case the shader styling would not conform to the spec.

I think it is important the the styling language be fully implementable both with a tradition compiler/interpreter as well as a transpiler to GLSL.

So we need to:

Figure out how to implement this in GLSL
Disallow it completely.
Disallow it for point clouds. Besides fragmentation, this is also troublesome since other tile formats may prefer a GLSL backend for very dynamic use cases.

Let's discussion in person if needed.

pjcozzi · 2016-10-10T15:28:43Z

This test fails in Chrome and Firefox for me:

pjcozzi · 2016-10-10T15:36:06Z

Source/Scene/PointCloud3DTileContent.js

        var styleableProperties;
        if (!defined(batchIds) && defined(batchTableBinary)) {
            styleableProperties = Cesium3DTileBatchTable.getBinaryProperties(pointsLength, batchTableJson, batchTableBinary);
+
+            // WebGL does not support UNSIGNED_INT, INT, or DOUBLE vertex attributes. Convert these to FLOAT.


Does float start to skip whole numbers in the max UNSIGNED_INT/INT range? If so, perhaps we should console.log warn. Otherwise, we should just warn for doubles.

For the spec, I think we'll need to allow conversion to float so that styling is implementable with WebGL 1.0 shaders.

Yeah it does start to skip... I'll add a warning

pjcozzi · 2016-10-10T16:04:08Z

Just those comments.

lilleyse · 2016-10-10T19:09:14Z

Are all the still open questions in the opening comment of this PR in the roadmap issue?

Yes they are in the 3D Tiles Roadmap under the Point Cloud Shader Styling section.

Other than the open questions, do we also need a spec PR to tighten anything up yet?

Possibly depending on how strict we want to be with type conversions. What did you think about this comment: #4336 (comment)? Also there will be some point specific notes like using POSITION, COLOR, and NORMAL semantics.

This test fails in Chrome and Firefox for me:

Fixed now. My Chrome doesn't support OIT so I had to tweak the value a bit to make it pass.

pjcozzi · 2016-10-10T19:12:54Z

If we decide to be loose about type comparisons in the JS implementation we may also want to change === to == to force casting. Right now a style with 5 < "6" would return true but 5 === "5" would be false.

Agreed. I would change it to be loose like JavaScript until we decide otherwise.

What else is this PR waiting on? Just pointSize styling?

lilleyse · 2016-10-10T19:13:46Z

What else is this PR waiting on? Just pointSize styling?

Yes just that.

lilleyse · 2016-10-10T22:02:59Z

Updated.

pjcozzi · 2016-10-15T15:20:50Z

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

@@ -129,6 +129,11 @@
    color : "rgb(${POSITION}[0] * 255, ${POSITION}[1] * 255, ${POSITION}[2] * 255)"
 });

+addStyle('Style point size', {
+    color : "color('red')",
+    size : "${temperature} * 10"


Why call this size? Why not call it precisely what it is: pointSize, and in the reference doc explain that it defines the point size in pixels when styling point cloud tiles.

My thinking was a generic name would be good for other sizeable things thing billboards. If fine with either way.

pjcozzi · 2016-10-15T15:23:18Z

Before we merge this, can you please also open a PR to add pointSize to the styling spec/schema?

In the spec, note that systems may have min/max point size limits (http://webglreport.com/) and that a valid renderer can just clamp to those system limits (and recommend that they warn when doing so).

Also add the Cesium code and unit test for this.

pjcozzi · 2016-10-15T16:08:25Z

Given that we don't yet know what billboard styling will look like, I would stick with the specific pointSize.

lilleyse · 2016-10-18T15:12:01Z

In the spec, note that systems may have min/max point size limits (http://webglreport.com/) and that a valid renderer can just clamp to those system limits (and recommend that they warn when doing so).

It's not always possible to know when the point size exceeds the maximum value when its based on a batch table property.

Given that 0.4% of users have a point size limited to 1px is this note needed?

lilleyse · 2016-10-18T15:31:51Z

Updated. Rename size to pointSize and now supporting === and !== again.

pjcozzi · 2016-10-18T15:32:55Z

In the spec, note that systems may have min/max point size limits (http://webglreport.com/) and that a valid renderer can just clamp to those system limits (and recommend that they warn when doing so).

It's not always possible to know when the point size exceeds the maximum value when its based on a batch table property.

Given that 0.4% of users have a point size limited to 1px is this note needed?

OK, the spec doesn't need to recommend warning, but I think it still needs to mention clamping; otherwise, it would be impossible to implement with GL point primitives and would require scaling triangles in a vertex shader.

pjcozzi · 2016-10-19T13:32:53Z

Nicely done, @lilleyse!

pnts shader styling

41fc014

pjcozzi reviewed Sep 20, 2016

View reviewed changes

lilleyse added 3 commits September 20, 2016 10:18

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

46d00e3

Organization

8e98836

Rearrange shader construction

b7c2dfd

lilleyse added 6 commits September 27, 2016 14:44

Reorganize parameters and support ARRAY type

cc1c4e6

Change conditions from an object to an array

5cb1088

Change how result params are used

758ebb2

Stricter type comparisons

9d2838e

Reset scratch index for Expression.evaluate

096b15b

Fix tests

976f32f

lilleyse force-pushed the pnts-styling branch from daacc56 to 976f32f Compare September 28, 2016 16:12

lilleyse added 2 commits September 28, 2016 12:15

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

953f9be

Calculate literal colors right away

1fd4b27

Limit array length

dbd681d

lilleyse added 2 commits September 28, 2016 15:04

Added Point Cloud Styling sandcastle

75beb23

Add back TODO

72ee217

pjcozzi mentioned this pull request Sep 29, 2016

Declarative styling CesiumGS/3d-tiles#2

Open

85 tasks

pjcozzi reviewed Oct 10, 2016

View reviewed changes

Tweak test for OIT

6d8a879

Log warning for float conversion

84d8ff7

lilleyse mentioned this pull request Oct 10, 2016

Change equality comparisons to == and != CesiumGS/3d-tiles#135

Merged

lilleyse added 2 commits October 10, 2016 17:17

Style point size

a217024

Change === to == and !== to !=

55cf834

lilleyse force-pushed the pnts-styling branch from 39c63f7 to 55cf834 Compare October 10, 2016 21:59

pjcozzi reviewed Oct 15, 2016

View reviewed changes

Rename size to pointSize

8823472

Support both === and !==

c04ff78

lilleyse force-pushed the pnts-styling branch from 0f5016a to c04ff78 Compare October 18, 2016 15:30

lilleyse added 3 commits October 18, 2016 13:05

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

f0f86ad

Fix styling edge case

83efc1b

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

45d96d8

pjcozzi merged commit 8780fc4 into 3d-tiles Oct 19, 2016

pjcozzi deleted the pnts-styling branch October 19, 2016 13:33

This was referenced Oct 21, 2016

Add point cloud styling section CesiumGS/3d-tiles#138

Merged

Shader styling considerations CesiumGS/3d-tiles#140

Closed

3D Tiles - Point Cloud Styling #4336

3D Tiles - Point Cloud Styling #4336

Conversation

lilleyse commented Sep 19, 2016 • edited

pjcozzi commented Sep 20, 2016

pjcozzi commented Sep 20, 2016

pjcozzi commented Sep 20, 2016

pjcozzi commented Sep 20, 2016

pjcozzi commented Sep 20, 2016

pjcozzi commented Sep 20, 2016

pjcozzi commented Sep 20, 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

pjcozzi Sep 20, 2016 • edited

Choose a reason for hiding this comment

pjcozzi left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Sep 20, 2016

lilleyse commented Sep 28, 2016

lilleyse commented Sep 28, 2016

pjcozzi commented Sep 29, 2016

pjcozzi commented Oct 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Oct 10, 2016

lilleyse commented Oct 10, 2016

pjcozzi commented Oct 10, 2016

lilleyse commented Oct 10, 2016

lilleyse commented Oct 10, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pjcozzi commented Oct 15, 2016

pjcozzi commented Oct 15, 2016

lilleyse commented Oct 18, 2016

lilleyse commented Oct 18, 2016

pjcozzi commented Oct 18, 2016

pjcozzi commented Oct 19, 2016

lilleyse commented Sep 19, 2016 •

edited

pjcozzi Sep 20, 2016 •

edited