Add multifrustum near/far planes to DebugCameraPrimitive #4932

austinEng · 2017-01-30T19:35:47Z

This adds multifrustum near/far planes to DebugCameraPrimitive and adds a toggle in CesiumInspector to draw these planes. This is useful for debugging what primitives are contained in each frustum.

lilleyse

I like the new functionality!

lilleyse · 2017-01-30T21:08:16Z

CHANGES.md

 * Added the ability to run the unit tests with a [WebGL Stub](https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Documentation/Contributors/TestingGuide#run-with-webgl-stub), which makes all WebGL calls a noop and ignores test expectations that rely on reading back from WebGL.  Use the web link from the main index.html or run with `npm run test-webgl-stub`.
+>>>>>>> agi/master



The merge conflict in here needs to be resolved.

lilleyse · 2017-01-30T21:09:16Z

Source/Scene/DebugCameraPrimitive.js

@@ -93,17 +93,13 @@ define([
    }

    var frustumCornersNDC = new Array(8);


Only 4 now.

lilleyse · 2017-01-30T21:09:44Z

Source/Scene/DebugCameraPrimitive.js


    var scratchMatrix = new Matrix4();
-    var scratchFrustumCorners = new Array(8);
+    var scratchFrustumCorners = new Array(4);
    for (var i = 0; i < 8; ++i) {


Should be 4

lilleyse · 2017-01-30T21:19:51Z

Source/Scene/DebugCameraPrimitive.js

-                positions[i * 3] = corner.x;
-                positions[i * 3 + 1] = corner.y;
-                positions[i * 3 + 2] = corner.z;
+            var numFrustums = Math.max(1, Math.ceil(Math.log(frameState.far / frameState.near) / Math.log(frameState.farToNearRatio)));


I think FrameState should store the exact splits rather than recalculating here.

lilleyse · 2017-01-30T21:20:16Z

Source/Scene/DebugCameraPrimitive.js

            // Create the outline primitive
-            var outlineIndices = new Uint16Array([0,1,1,2,2,3,3,0,0,4,4,7,7,3,7,6,6,2,2,1,1,5,5,4,5,6]);
+            var outlineIndices = new Uint16Array(8 * (2 * numFrustums + 1));
+            // build the far planes


Capitalize the first word in comments.

lilleyse · 2017-01-30T21:25:23Z

Source/Scene/Scene.js

@@ -945,6 +950,40 @@ define([
        },

        /**
+         * This property is for debugging only; it is not for production use.
+         * <p>
+         * When <code>true</code>, draws primitives to show the boundaries of the camera frustum


Tweak wording a bit - draws outlines to show the boundaries of the camera frustums

lilleyse · 2017-01-30T21:25:44Z

Source/Scene/Scene.js

+            get : function() {
+                return this._debugShowFrustumPlanes;
+            },
+


Remove empty line.

lilleyse · 2017-01-30T21:28:19Z

Source/Scene/Scene.js

+                        updateOnChange: false
+                    });
+                    this._debugFrustumPlanes.update(this.frameState);
+                    this.primitives.add(this._debugFrustumPlanes);


Do not add directly to primitives. Instead Scene should be responsible for pushing this._debugFrustumPlanes's commands.

While doing that make sure to destroy it when scene is destroyed.

lilleyse · 2017-01-30T21:28:46Z

Source/Scene/Scene.js

+                return this._debugShowFrustumPlanes;
+            },
+
+            set : function(val) {


val -> value

austinEng · 2017-01-31T13:46:03Z

@lilleyse Updated.

lilleyse · 2017-01-31T15:06:02Z

Looks good to me, does anyone else want to review quickly?

pjcozzi · 2017-02-01T00:27:45Z

Are we sure it is correct? It seems like it doesn't draw the farthest frustum.

pjcozzi · 2017-02-01T00:29:51Z

Source/Scene/DebugCameraPrimitive.js

+            var outlineIndices = new Uint16Array(8 * (2 * numFrustums + 1));
+            // Build the far planes
+            for (f = 0; f < numFrustums + 1; ++f) {
+                outlineIndices[f * 8] = f * 4;


Here and in the loop below, assign f * 8 and f * 4 to locals. I do not know if the JS engine would optimize. This primitive is just for debugging, but in general primitive.update functions are carefully coded for performance.

Same comment throughout.

pjcozzi · 2017-02-01T00:32:00Z

Source/Scene/FrameState.js

+         * @type {Number[]}
+         * @default [1.0, 1000.0]
+         */
+        this.frustumSplits = [1.0, 1000.0];


Are you sure about these defaults? Perhaps set this to []. At the start of the frame, set the length to zero (which does not free the memory), then push each distance.

pjcozzi · 2017-02-01T00:32:34Z

CHANGES.md

@@ -32,6 +32,7 @@ Change Log
 * The attribute `perInstanceAttribute` of `DebugAppearance` has been made optional and defaults to `false`.
 * Fixed a bug that would cause a crash when `debugShowFrustums` is enabled with OIT. [#4864](https://github.com/AnalyticalGraphicsInc/cesium/pull/4864)
 * Added the ability to run the unit tests with a [WebGL Stub](https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Documentation/Contributors/TestingGuide#run-with-webgl-stub), which makes all WebGL calls a noop and ignores test expectations that rely on reading back from WebGL.  Use the web link from the main index.html or run with `npm run test-webgl-stub`.
+* Added support to `DebugCameraPrimitive` to draw multifrustum planes. `CesiumInspector` also displays this toggle.


Explicitly mention the new properties and arguments.

Also move this to a new section for 1.31 since this is unlikely to make the 1.30 release, which is going out tomorrow morning.

pjcozzi · 2017-02-01T00:36:00Z

Source/Scene/Scene.js

+                return this._debugShowFrustumPlanes;
+            },
+            set : function(value) {
+                if (!value) {


Here and below, do not call destroy and new DebugCameraPrimitive in the set function; it could result in WebGL calls being made outside of request animation frame.

Instead check if this flag changed in the render loop and then create or destroy.

For example, see modelMatrix in DebugModelMatrixPrimitive.

pjcozzi · 2017-02-01T00:37:48Z

Source/Scene/Scene.js

+         *
+         * @default false
+         */
+        debugShowFrustumPlanes : {


When this is merged into the 3d-tiles branch, please remove the code that explicitly creates the debug frustum in the Cesium3DTileset and just have the inspector set this property.

pjcozzi · 2017-02-01T00:38:45Z

This will be very cool for performance and visual artifact debugging.

austinEng · 2017-02-01T17:39:29Z

Are we sure it is correct? It seems like it doesn't draw the farthest frustum.

As in it's the wrong size or it doesn't appear at all? I thought that planes were located wrongly at first, but I realized that since I'm computing the splits based on the distances computed in Scene, and we clamp the planes distances down to the size of the contained bounding spheres, I think it makes sense that the last far plane doesn't extend as far as expected.

pjcozzi · 2017-02-01T18:38:18Z

Looks good, but try a case when the viewer is near terrain and looking slightly up and there are 3 frustums.

austinEng · 2017-02-01T19:21:48Z

Here's three frustums

pjcozzi · 2017-02-01T21:28:52Z

OK, not sure what we where running to offline.

I am good with this!

@lilleyse did you want to look at the latest changes?

lilleyse

Changes look good.

lilleyse · 2017-02-01T21:49:25Z

Source/Scene/Scene.js

+        if (defined(scene._debugFrustumPlanes)) {
+            scene._debugFrustumPlanes.update(frameState);
+        }
+


Cleaner if all this is placed in its own function updateDebugFrustumPlanes

lilleyse · 2017-02-01T21:50:49Z

Source/Scene/DebugCameraPrimitive.js

@@ -149,8 +165,38 @@ define([
                values : positions
            });

+            var offset, idx;


Avoid abbreviations, index is fine.

austinEng · 2017-02-02T19:13:36Z

@lilleyse is this ready to be merged then?

lilleyse · 2017-02-02T19:46:40Z

Yes, thanks @austinEng .

austinEng added 3 commits January 30, 2017 19:24

add show frustum planes to inspector

afad284

update CHANGES.md

58f2879

Merge remote-tracking branch 'agi/master' into frustum-debug

3b69d8d

lilleyse reviewed Jan 30, 2017

View reviewed changes

austinEng added 2 commits January 31, 2017 08:41

don't explicitly add draw commands; various fixes

9e575cc

Merge remote-tracking branch 'agi/master' into frustum-debug

270a0fd

austinEng force-pushed the frustum-debug branch from 9372246 to 270a0fd Compare January 31, 2017 13:44

Merge branch 'master' into frustum-debug

672405a

pjcozzi reviewed Feb 1, 2017

View reviewed changes

austinEng added 5 commits February 1, 2017 11:04

factor out index offsets

06c8f7c

use empty frustumSplits by default

495631e

move DebugCameraPrimitive creation to updatePrimitives

4b7843a

update CHANGES.md

0b44434

use camera near/far planes if no splits created

82b88a9

austinEng force-pushed the frustum-debug branch from aee3c96 to 82b88a9 Compare February 1, 2017 17:26

lilleyse reviewed Feb 1, 2017

View reviewed changes

cleanup debug frustum update in scene. remove idx abbreviation

58783cb

austinEng force-pushed the frustum-debug branch from 32d84fa to 98622d8 Compare February 2, 2017 13:39

austinEng force-pushed the frustum-debug branch from 98622d8 to 58783cb Compare February 2, 2017 13:42

Merge remote-tracking branch 'agi/master' into frustum-debug.

9d28ee1

austinEng force-pushed the frustum-debug branch from a0bcd74 to 9d28ee1 Compare February 2, 2017 18:50

lilleyse merged commit b6afe7e into CesiumGS:master Feb 2, 2017

lilleyse mentioned this pull request Feb 3, 2017

3d Tiles Inspector #4872

Merged

4 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add multifrustum near/far planes to DebugCameraPrimitive #4932

Add multifrustum near/far planes to DebugCameraPrimitive #4932

austinEng commented Jan 30, 2017

lilleyse left a comment

lilleyse Jan 30, 2017

lilleyse Jan 30, 2017

lilleyse Jan 30, 2017

lilleyse Jan 30, 2017

lilleyse Jan 30, 2017

lilleyse Jan 30, 2017

lilleyse Jan 30, 2017

lilleyse Jan 30, 2017

lilleyse Jan 30, 2017

lilleyse Jan 30, 2017

austinEng commented Jan 31, 2017

lilleyse commented Jan 31, 2017

pjcozzi commented Feb 1, 2017

pjcozzi Feb 1, 2017

pjcozzi Feb 1, 2017

pjcozzi Feb 1, 2017

pjcozzi Feb 1, 2017

pjcozzi Feb 1, 2017

pjcozzi Feb 1, 2017

pjcozzi Feb 1, 2017

pjcozzi commented Feb 1, 2017

austinEng commented Feb 1, 2017

pjcozzi commented Feb 1, 2017

austinEng commented Feb 1, 2017 •

edited

Loading

pjcozzi commented Feb 1, 2017

lilleyse left a comment

lilleyse Feb 1, 2017

lilleyse Feb 1, 2017

austinEng commented Feb 2, 2017

lilleyse commented Feb 2, 2017

		* Added the ability to run the unit tests with a [WebGL Stub](https://github.com/AnalyticalGraphicsInc/cesium/tree/master/Documentation/Contributors/TestingGuide#run-with-webgl-stub), which makes all WebGL calls a noop and ignores test expectations that rely on reading back from WebGL. Use the web link from the main index.html or run with `npm run test-webgl-stub`.
		>>>>>>> agi/master

		@@ -93,17 +93,13 @@ define([
		}

		var frustumCornersNDC = new Array(8);

Add multifrustum near/far planes to DebugCameraPrimitive #4932

Add multifrustum near/far planes to DebugCameraPrimitive #4932

Conversation

austinEng commented Jan 30, 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

austinEng commented Jan 31, 2017

lilleyse commented Jan 31, 2017

pjcozzi commented Feb 1, 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

Choose a reason for hiding this comment

pjcozzi commented Feb 1, 2017

austinEng commented Feb 1, 2017

pjcozzi commented Feb 1, 2017

austinEng commented Feb 1, 2017 • edited Loading

pjcozzi commented Feb 1, 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

austinEng commented Feb 2, 2017

lilleyse commented Feb 2, 2017

austinEng commented Feb 1, 2017 •

edited

Loading