More robust point cloud post processor #5455

AnimatedRNG · 2017-06-12T13:57:15Z

Still don't have the "change previous stages so that they write out ES coordinates to the texture" part working; everything else looks okay though.

pjcozzi

Looks good at quick glance. Just a few small comments. @lilleyse can review and merge when ready.

pjcozzi · 2017-06-13T20:15:47Z

Source/Scene/PointCloudPostProcessor.js

+            width : screenWidth,
+            height : screenHeight,
+            pixelFormat : PixelFormat.RGBA,
+            pixelDatatype : PixelDatatype.FLOAT,


Do we need to check for an extension for this?

WebGL 2 or https://www.khronos.org/registry/webgl/extensions/OES_texture_float/

pjcozzi · 2017-06-13T20:16:17Z

Source/Scene/PointCloudPostProcessor.js

-                sampler : createSampler()
-            });
+        for (i = 0; i < 3; ++i) {
+            if (i < 2) {


Use two separate for loops; it will be cleaner.

pjcozzi · 2017-06-13T20:20:29Z

Source/Scene/PointCloudPostProcessor.js

-            // TODO : for now assume than any pass can write depth, possibly through EXT_frag_depth.
-            // Definitely needed for the initial render into the FBO, possibly also the ping-pong processing.
-            framebuffers[i] = new Framebuffer({
+        /* We want to reuse textures as much as possible, so here's the order


Stick with the // comment style.

This is actually not in the Coding Guide, but I'm pretty sure it is a common practice throughout Cesium except for comments for generating reference doc with JSDoc.

Could you open a separate PR to mention this at the bottom of the Basic Code Construction?

pjcozzi · 2017-06-13T20:22:18Z

Source/Scene/PointCloudPostProcessor.js

+         */
+
+        processor._framebuffers = {
+            "prior": new Framebuffer({


Here and similar properties below, "prior" -> prior

Quotes are not needed here. When they are, use single quotes to be consistent with the rest of the code base.

pjcozzi · 2017-06-13T20:22:58Z

Source/Scene/PointCloudPostProcessor.js

-        var framebuffers = new Array(2);
+        var depthTextures = new Array(3);
+
+        var ecTexture = new Texture({


Somewhere, we should comment (or rename this) about what "EC" stands for.

pjcozzi · 2017-06-13T20:27:57Z

Source/Scene/PointCloudPostProcessor.js

+        var framebuffers = processor._framebuffers;
+        var clearCommands = new Array(4);
+        i = 0;
+        for (var name in framebuffers) {


Throughout, it looks like _framebuffers is used as both an array and object. Be consistent. Perhaps this can be an array with constants in an enum for look ups.

pjcozzi · 2017-06-13T20:29:04Z

Source/Scene/PointCloudPostProcessor.js

@@ -222,15 +315,59 @@ define([

        if (resized) {
            destroyFramebuffers(processor);
-            createFramebuffers(processor, context);
-            updateCommandFramebuffers(processor);
+            //createFramebuffers(processor, context);


We generally don't commit code that is commented out. Usually, remove it.

Otherwise, it can stay in your branch like this, but we would remove it before merging into master.

pjcozzi · 2017-06-13T20:29:22Z

Source/Scene/PointCloudPostProcessor.js

+            'void main() \n' +
+            '{ \n' +
+            '    czm_point_cloud_post_process_main(); \n' +
+            //'    gl_FragData[0] = gl_FragColor;' +


Same comment.

… is left dirty before use

…on't keep resetting

lilleyse

Just a bunch of small things, overall it looks nearly ready.

lilleyse · 2017-09-11T15:08:07Z

Source/Scene/PointCloudPostProcessor.js

+                derivedCommand.castShadows = false;
+                derivedCommand.receiveShadows = false;
+
+                var derivedCommandRenderState = new RenderState(derivedCommand.renderState);


We usually don't construct RenderStates directly, instead we typically do clone(derivedCommand.renderState). The RenderState.fromCache call below will actually create the render state from that.

lilleyse · 2017-09-11T15:08:27Z

Source/Scene/PointCloudPostProcessor.js

+
+                var derivedCommandUniformMap = derivedCommand.uniformMap;
+                var newUniformMap = {
+                    'u_pointAttenuationMaxSize' : attenuationUniformFunction


Remove the quotes.

lilleyse · 2017-09-11T15:14:31Z

Source/Scene/PointCloud3DTileContent.js

@@ -255,7 +256,7 @@ define([
            get : function() {
                return this._batchTable;
            }
-        }
+        },


Remove comma.

lilleyse · 2017-09-11T15:14:57Z

Source/Scene/Cesium3DTileset.js

+         * @param {Boolean} useTriangle Whether or not to use the triangle wave optimization
+         * @param {Boolean} enableAO Whether or not to blend with ambient occlusion
+         * @param {Boolean} depthViewEnabled Whether or not the depth view is enabled
+         * @param {Boolean} AOViewEnabled Whether or not the ambient occlusion view is enabled


Can you rename AOViewEnabled to aoViewEnabled throughout?

lilleyse · 2017-09-11T15:36:56Z

Source/Scene/PointCloud3DTileContent.js

@@ -766,7 +770,7 @@ define([
        }

        // Edit the function header to accept the point position, color, and normal
-        return source.replace('()', '(vec3 position, vec3 position_absolute, vec4 color, vec3 normal)');
+        return source.replace('()', '(vec3 position, vec3 positionWC, vec4 color, vec3 normal)');


I was the one to suggest this before, but I realized this name change would break styles that use the POSITION_ABSOLUTE built-in. At least this occurrence should remain as position_absolute, but the others can stay positionWC.

I'm not exactly sure if the latest commit fixes this, but I think it's fixed

lilleyse · 2017-09-11T15:45:41Z

Source/Scene/PointCloudPostProcessor.js

+        processor._edgeCullingTexture = undefined;
+        processor._sectorLUTTexture = undefined;
+        processor._aoTextures = undefined;
+        processor._dirty = undefined;


This line can be removed.

lilleyse · 2017-09-11T15:58:00Z

Source/Scene/PointCloudPostProcessor.js

+
+        var first = sectors[0];
+        var second = sectors[0];
+        sectors.forEach(function(element) {


Just a plain loop is fine and more fitting with code conventions. Also would sectors be fine as a JS array?

lilleyse · 2017-09-11T16:04:22Z

Source/Scene/PointCloudPostProcessor.js

+            this.sigmoidSharpness = tileset.pointCloudPostProcessorOptions.sigmoidSharpness;
+            this.dropoutFactor = tileset.pointCloudPostProcessorOptions.dropoutFactor;
+            this.delay = tileset.pointCloudPostProcessorOptions.delay;
+            dirty = true;


@AnimatedRNG did you see this comment?

Edit: The original comment is here: https://github.com/AnalyticalGraphicsInc/cesium/pull/5455/files#r132295486

lilleyse · 2017-09-11T16:05:53Z

Source/Scene/PointCloudPostProcessor.js

+            commandList.push(debugViewCommand);
+        }
+
+        commandList.push(clearCommands['prior']);


Style - clearCommands.prior

lilleyse · 2017-09-21T13:19:22Z

Source/Scene/PointCloudPostProcessor.js

+                derivedCommand.castShadows = false;
+                derivedCommand.receiveShadows = false;
+
+                var derivedCommandRenderState = RenderState.fromCache(derivedCommand.renderState);


Do a clone here instead since the RenderState.fromCache call is below. For example, look at https://github.com/AnalyticalGraphicsInc/cesium/blob/master/Source/Scene/Cesium3DTileBatchTable.js#L1329-L1332.

lilleyse · 2017-09-21T13:20:14Z

Source/Scene/PointCloudPostProcessor.js

+        processor._stencilMaskTexture = stencilMaskTexture;
+    }
+
+    function addConstants(sourceFS, constantName, replacement) {


Rename replacement to value.

lilleyse · 2017-09-21T13:21:02Z

Source/Scene/PointCloudPostProcessor.js

+        this._drawCommands = undefined;
+        this._clearCommands = undefined;
+
+        this.densityScaleFactor = '10.0';


This can be a number instead of a string.

It's slightly easier for it to remain a string because javascript keeps representing the number 10.0 as 10 when it is stringified. When the preprocessor define gets copied in, there are casting issues.

lilleyse · 2017-09-21T13:33:15Z

Specs/Scene/PointCloudPostProcessorSpec.js

+        scene.primitives.removeAll();
+    });
+
+    it('enabling the point cloud post processor increases the number of draw calls', function() {


This test fails on Travis since the WebGL Stub does not enable the required extensions for the processor. You may want to make processingSupported in PointCloudPostProcessor a static function like PointCloudPostProcessor.isSupported and check that before running the test.

lilleyse · 2017-09-21T13:40:05Z

Source/Scene/PointCloudPostProcessor.js

+        densityEdgeCullFS = addConstants(
+            densityEdgeCullFS,
+            'epsilon8',
+            1e-8


Thinking about these defines some more, it might be cleaner to keep unchanging defines like this in the shader code, and only call addConstants on those that do change.

lilleyse · 2017-10-18T19:26:16Z

@pjcozzi do you want to review this before it goes into point-cloud-processor?

pjcozzi · 2017-10-18T21:39:40Z

do you want to review this...

Yes

...before it goes into point-cloud-processor?

No

I'll review as soon as I can - and comment here - but please merge in the meantime.

lilleyse · 2017-10-19T17:13:19Z

Thanks for all the work on this @AnimatedRNG!

AnimatedRNG force-pushed the point-cloud-processor-ss branch from ab7d69b to 3424709 Compare June 12, 2017 14:43

pjcozzi requested changes Jun 13, 2017

View reviewed changes

AnimatedRNG added 22 commits June 19, 2017 10:15

Now moves extensions to the top of the shader

3cbbbe0

Not-so-functional existing (non-derived) post-processing framework

8268a56

Adds new derived command code

86fac01

Clearing the framebuffers now happens in the correct order -- nothing…

cef8a2a

… is left dirty before use

Now properly hands off data between stages

8e2d7ca

Now correctly currying depth

7f4827b

Temporarily reducing the number of frustra so that the depth values d…

4adc68c

…on't keep resetting

Moves shaders into their own files

32810ec

Working point occlusion filter

9394cca

Fixes resizing/destroy issues and takes options now

f77cb8b

Now drawing color point cloud

3c9bf84

Adds enabled property

d36c8ff

Adds new Sandcastle demo for point cloud post processing

b5fd034

Sandcastle demo now includes even more options

303ad68

Adds region growing pass

197b54c

Adds more configurable parameters to PointCloudPostProcessor

3db0993

Fixes bug where additional region growing passes didn't grow empty areas

449f325

More sane parameter for the number of region growing passes

ce5bf10

Some code cleanup in the region growing filter

2520c89

Sector histogram modification has better branching now

8331358

Better inverse trig

c22a98e

Some code cleanup and branch optimization

6930938

pjcozzi mentioned this pull request Jun 27, 2017

Replace atan in polyline dash vertex shader with fast approximation #5545

Closed

AnimatedRNG added 5 commits June 27, 2017 15:52

Updates capability flags for WebGL 2

03f2266

Newline after version in ShaderSource

d6b1869

Updates pointcloud processor to WebGL 2.0

5862b6b

Adds support for the EXT_color_buffer_float extension

37cef07

Moves GLSLModernizer to its own file

e9855c0

AnimatedRNG added 9 commits September 4, 2017 19:45

Removes pi defines from shader file

e65689a

Removes empty line

e08fa64

Removes the morphTo3D call

fbb8ba1

Removes files from spec

297c1ca

Removes TODOs

81243b3

Destroy method now destroys the processor

5e6ad06

Adds method documentation for the PointCloudPostProcessor

255216e

Removes old comment

a5b474e

Formatting changes

1503713

lilleyse reviewed Sep 11, 2017

View reviewed changes

lilleyse mentioned this pull request Sep 11, 2017

Simple point cloud attenuation #5824

Closed

AnimatedRNG added 9 commits September 13, 2017 15:09

All the defines are now added at runtime

05a3cda

Removes unnecessary line

d46ebb4

Style fixes

0cc0aaa

Fixes sectors array iteration

9da4c9e

Render state now created from cache

a95f9ca

Renames AOView to aoView

3dc4d2f

Renames one occurrence of position_absolute to positionWC

8c3439a

Fixes change that broke density view

0923e73

Adds a new edge culling metric -- distanceConstraint

0deb547

lilleyse reviewed Sep 21, 2017

View reviewed changes

AnimatedRNG added 3 commits September 28, 2017 14:20

Clone fixes

b3df085

Tests should hopefully no longer break CI

0fbdb39

Tests longer assume that pointCloudPostProcessor has been created

4b1a918

AnimatedRNG force-pushed the point-cloud-processor-ss branch from 437a285 to 4b1a918 Compare October 18, 2017 17:26

lilleyse merged commit 93145ba into CesiumGS:point-cloud-processor Oct 19, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More robust point cloud post processor #5455

More robust point cloud post processor #5455

AnimatedRNG commented Jun 12, 2017

pjcozzi left a comment

pjcozzi Jun 13, 2017

pjcozzi Jun 13, 2017

pjcozzi Jun 13, 2017

pjcozzi Jun 13, 2017

pjcozzi Jun 13, 2017

pjcozzi Jun 13, 2017

pjcozzi Jun 13, 2017

pjcozzi Jun 13, 2017

lilleyse left a comment

lilleyse Sep 11, 2017

lilleyse Sep 11, 2017

lilleyse Sep 11, 2017

lilleyse Sep 11, 2017

lilleyse Sep 11, 2017

AnimatedRNG Sep 13, 2017

lilleyse Sep 11, 2017

lilleyse Sep 11, 2017

lilleyse Sep 11, 2017 •

edited

Loading

lilleyse Sep 11, 2017

lilleyse Sep 21, 2017

lilleyse Sep 21, 2017

lilleyse Sep 21, 2017

AnimatedRNG Sep 28, 2017

lilleyse Sep 21, 2017

lilleyse Sep 21, 2017

lilleyse commented Oct 18, 2017

pjcozzi commented Oct 18, 2017

lilleyse commented Oct 19, 2017

More robust point cloud post processor #5455

More robust point cloud post processor #5455

Conversation

AnimatedRNG commented Jun 12, 2017

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

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 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

lilleyse Sep 11, 2017 • 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

Choose a reason for hiding this comment

lilleyse commented Oct 18, 2017

pjcozzi commented Oct 18, 2017

lilleyse commented Oct 19, 2017

lilleyse Sep 11, 2017 •

edited

Loading