Per tile stats #5188

rahwang · 2017-04-07T19:51:27Z

For #5156. Not ready yet! Is this on the right track, @lilleyse ? Seems wrong to just be using the tileset._statistics for draw command, texture memory and vertex memory numbers, but I'm not sure how to compute these per tile.

lilleyse · 2017-04-07T21:53:13Z

Yes this is on the right track. I didn't look too deeply at the code yet, but just a few things:

You can access the memory stats from the tile's content
Number of draw commands is tougher and not as easily accessible per-tile. PointCloud3DTileContent is always 1, Batched and Instanced can probably be determined in their update functions by looking at the number of commands before and after. Either way, add a commandsLength getter to each Content. I haven't fully thought this case through...
Now that triangles length is reported, also add that to tile info (add points length as well)
Use the new depth-test off feature so labels aren't clipped Billboard depth fail #5166
Use a smaller font, or adjust based on the number of properties showing
Maybe this is already on your radar, but the ability to only show the label for the tile that is hovered over, similar to the Picking Sandcastle. A checkbox in the tile info tab can control which mode is being used.

rahwang · 2017-04-20T17:44:40Z

Hey @lilleyse , I redid my changes to reflect Hannah's recent refactoring of the inspector and addressed all the things you mentioned above (except the picking, which I'm working on). How should the tile info stats changes I've made so far be tested? Just follow the template of the debugShowGeometricError tests in the Cesium3DTilesetSpec?

lilleyse · 2017-04-20T19:32:00Z

Those tests can be renamed to show tile info labels with _____. In at least one of them all the debug options should be set to true.

Then a separate test can check how the labels change when new debug settings are toggled. Setting viewRootOnly at the beginning will keep the code more concise for that test.

rahwang · 2017-04-26T17:44:23Z

Ah, as per discussion with @lilleyse , going to refactor the debug label assembly to happen in the inspector view model instead of in the tileset.

rahwang · 2017-04-27T17:40:13Z

Ah, sorry about that @lilleyse -- I still had the original labels drawing for debugging purposes. Here is my properly broken code. XD

lilleyse · 2017-04-27T17:54:32Z

The problem is that the update is pushing commands after the render loop, and so they just get ignored. The same would happen with preRender. Maybe this means storing the label collection in the view model is bad design.

rahwang · 2017-04-27T18:00:49Z

Ah, thanks for explaining, @lilleyse . So shall I just revert to the previous version?

lilleyse · 2017-04-27T18:06:55Z

Yeah, that's best for now.

This reverts commit 19e4179.

This reverts commit f627ddd.

rahwang · 2017-04-27T19:44:00Z

Ready for another look @lilleyse

lilleyse · 2017-04-27T21:14:25Z

Just to summarize the PR breakdown...

This one should be ready soon. Then open a 2nd PR to fix picking for tilesets that don't use a batch table. And then a 3rd PR to add this feature:

the ability to only show the label for the tile that is hovered over, similar to the Picking Sandcastle. A checkbox in the tile info tab can control which mode is being used.

lilleyse

Looks great! Some of these new options will be super helpful.

lilleyse · 2017-04-27T21:23:58Z

Source/Scene/Cesium3DTile.js

        applyDebugSettings(this, tileset, frameState);
        this._content.update(tileset, frameState);
        this._transformDirty = false;
+        this._content.commandsLength = frameState.commandList.length - initCommandLength;


It will be simpler if Cesium3DTile holds onto this._commandsLength instead of the contents.

lilleyse · 2017-04-27T21:24:48Z

Source/Scene/Cesium3DTileset.js

+         */
+        this.debugShowNumberOfPoints = defaultValue(options.debugShowNumberOfPoints, false);
+
+


Remove empty line.

lilleyse · 2017-04-27T21:26:46Z

Source/Scene/Cesium3DTileset.js

-                text: tile.geometricError.toString(),
-                position: position
+
+            var labelString = "";


Style: Use single quotes instead double quotes throughout.

lilleyse · 2017-04-27T21:28:41Z

Source/Scene/Cesium3DTileset.js

-                position: position
+
+            var labelString = "";
+            var attribCounter = 0;


Style: avoid abbreviations.

lilleyse · 2017-04-27T21:29:28Z

Source/Scene/Cesium3DTileset.js

+            var attribCounter = 0;
+
+            if (tileset.debugShowGeometricError) {
+                labelString += "\nGeometric error: " + tile.geometricError.toString();


toString() for these isn't needed.

lilleyse · 2017-04-27T21:39:31Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

+         */
+        this.tileInfoVisible = false;
+
+


Remove empty line.

lilleyse · 2017-04-27T21:40:53Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

+         * @type {Boolean}
+         * @default false
+         */
+        this.textureMemory = false;


Place the showGeometricError stuff closer to these.

Also it would be better to arrange these in the order they are shown in the UI.

lilleyse · 2017-04-27T21:41:29Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

+            get : function() {
+                return numberOfTriangles();
+            },
+            set : function(val) {


Change all val to value.

lilleyse · 2017-04-27T21:44:37Z

Specs/Scene/Cesium3DTilesetSpec.js

+            expect(tileset._tileInfoLabels.length).toEqual(5);
+
+            var root = tileset._root;
+            expect(tileset._tileInfoLabels._labels[0].text).toEqual('Geometric error: ' + root.geometricError.toString());


Remove uses of toString() throughout this file.

lilleyse · 2017-04-27T21:46:35Z

Specs/Scene/Cesium3DTilesetSpec.js

+                                                                    'Points: ' + content.pointsLength.toString() + '\n' +
+                                                                    'Triangles: ' + content.trianglesLength.toString() + '\n' +
+                                                                    'Texture Memory: ' + content.textureMemorySizeInBytes.toString() + '\n' +
+                                                                    'Vertex Memory: ' + content.vertexMemorySizeInBytes.toString());


To avoid the large indentation, create a var for the string.

lilleyse · 2017-04-28T14:09:22Z

Fixes #5029 too
I forgot that issue existed...

@rahwang can you also add Number of features to this PR?

rahwang · 2017-04-28T14:14:05Z

Sure! I'll do that now.

rahwang · 2017-04-28T14:39:17Z

Ok @lilleyse, I added feature counts and updated according to your review. Ready for another look.

lilleyse · 2017-04-28T15:42:07Z

Source/Scene/Cesium3DTileset.js

+            var attributes = 0;
+
+            if (tileset.debugShowGeometricError) {
+                labelString += "\nGeometric error: " + tile.geometricError;


Use single-quotes for these.

lilleyse · 2017-04-28T15:44:37Z

Source/Scene/Cesium3DTileset.js

+        // Destroy debug labels
+        if (defined(this._tileInfoLabels)) {
+            this._tileInfoLabels.destroy();
+        }


The destroy convention usually looks like:

tileset._tileInfoLabels = tileset._tileInfoLabels && tileset._tileInfoLabels.destroy();

lilleyse · 2017-04-28T15:44:46Z

Source/Scene/Instanced3DModel3DTileContent.js

        initialize(this, arrayBuffer, byteOffset);
+


Remove empty line.

lilleyse · 2017-04-28T17:31:55Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspector.js

@@ -132,6 +132,7 @@ define([
        var updatePanelContents = document.createElement('div');
        var loggingPanelContents = document.createElement('div');
        var stylePanelContents = document.createElement('div');
+        var tileInfoPanelContents = document.createElement('div');


Also place this before stylePanel for consistency.

lilleyse · 2017-04-28T17:39:50Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

-        this.freezeFrame = false;
+        this.textureMemory = false;
+
+        var vertexMemory = knockout.observable();


I missed this from before. Can you rename all the new properties to showTextureMemory to follow the showGeometricError convention?

You didn't miss it! I accidentally reverted it along with the labels-to-view-model commits. XD

lilleyse · 2017-04-28T17:44:34Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

@@ -681,7 +817,13 @@ define([
                                    'showContentBoundingVolumes',
                                    'showRequestVolumes',
                                    'showGeometricError',


Move showGeometricError below near the others.

lilleyse · 2017-04-28T17:44:49Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

+                                    'vertexMemory',
+                                    'numberOfPoints',
+                                    'numberOfTriangles',
+                                    'numberOfCommands'];


Missing numberOfFeatures.

lilleyse · 2017-04-28T17:45:09Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

@@ -681,7 +817,13 @@ define([
                                    'showContentBoundingVolumes',
                                    'showRequestVolumes',
                                    'showGeometricError',
-                                    'freezeFrame'];
+                                    'freezeFrame',
+                                    'onlyPickedTileInfo',


lilleyse · 2017-04-28T17:50:31Z

Source/Scene/Cesium3DTileset.js

+                attributes++;
+            }
+            if (tileset.debugShowVertexMemoryUsage) {
+                labelString += "\nVertex Memory: " + tile.content.vertexMemorySizeInBytes;


Late suggestion, but can this and texture memory usage show it in megabytes like in the stats panel in the inspector?

rahwang · 2017-04-28T19:42:38Z

Thanks for the review @lilleyse ! Comments addressed.

lilleyse · 2017-05-01T19:40:29Z

Source/Scene/Cesium3DTileset.js

@@ -2221,7 +2221,7 @@ define([
    Cesium3DTileset.prototype.destroy = function() {
        // Destroy debug labels
        if (defined(this._tileInfoLabels)) {
-            this._tileInfoLabels.destroy();
+            this._tileInfoLabels = this._tileInfoLabels && this._tileInfoLabels.destroy();
        }


Remove the if (defined(this._tileInfoLabels)) { wrapper, it is unnecessary now.

lilleyse · 2017-05-01T19:44:11Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspector.js

@@ -196,17 +196,27 @@ define([
        errorBox.setAttribute('data-bind', 'text: editorError');
        stylePanelContents.appendChild(errorBox);

+        tileInfoPanelContents.appendChild(makeCheckbox('showGeometricError', 'Geometric Error'));
+        tileInfoPanelContents.appendChild(makeCheckbox('showNumberOfCommands', 'Number of Commands'));


Change to Number Of Commands

lilleyse · 2017-05-01T19:47:00Z

Source/Widgets/Cesium3DTilesInspector/Cesium3DTilesInspectorViewModel.js

-                                   'dynamicScreenSpaceErrorDensity', 'dynamicScreenSpaceErrorDensitySliderValue', 'dynamicScreenSpaceErrorFactor', 'pickActive'];
+                                   'showContentBoundingVolumes', 'showRequestVolumes', 'freezeFrame', 'maximumScreenSpaceError', 'dynamicScreenSpaceErrorDensity',
+                                   'dynamicScreenSpaceErrorDensitySliderValue', 'dynamicScreenSpaceErrorFactor', 'pickActive', 'showGeometricError',
+                                   'showTextureMemory', 'showVertexMemory', 'showNumberOfCommands', 'showNumberOfPoints', 'showNumberOfTriangles', 'showNumberOfFeatures'];


Also reorder them here to match.

lilleyse · 2017-05-01T19:47:50Z

Source/Scene/Cesium3DTileset.js

+         * @type {Boolean}
+         * @default false
+         */
+        this.debugShowVertexMemoryUsage = defaultValue(options.debugShowVertexMemoryUsage, false);


Can you add these debugShow options to the doc in the constructor?

lilleyse · 2017-05-01T19:49:59Z

Ok I think these are the last points, almost ready!

rahwang · 2017-05-01T21:11:41Z

Done, @lilleyse!

pjcozzi · 2017-05-02T14:52:52Z

Source/Scene/Cesium3DTileset.js

@@ -121,7 +121,13 @@ define([
     * @param {Boolean} [options.debugShowBoundingVolume=false] For debugging only. When true, renders the bounding volume for each tile.
     * @param {Boolean} [options.debugShowContentBoundingVolume=false] For debugging only. When true, renders the bounding volume for each tile's content.
     * @param {Boolean} [options.debugShowViewerRequestVolume=false] For debugging only. When true, renders the viewer request volume for each tile.
-     * @param {Boolean} [options.debugShowGeometricError=false] For debugging only. When true, draws labels to indicate the geometric error of each tile
+     * @param {Boolean} [options.debugShowGeometricError=false] For debugging only. When true, draws labels to indicate the geometric error of each tile.


These are probably too fine-grained. Consider:

Combine debugShowNumberOfCommands, debugShowNumberOfPoints, debugShowNumberOfTriangles, and debugShowNumberOfFeatures into debugShowRenderingStatistics

Combine debugShowTextureMemoryUsage and debugShowVertexMemoryUsage into debugShowMemoryUsage

This will be a lot less code and is inline with how I expect folks will want to use these hopefully without cluttering the display.

If clutter is a problem, try a smaller font that is still readable or a slightly less coarse-grained separation for the rendering stats.

pjcozzi · 2017-05-02T14:53:26Z

Source/Scene/Cesium3DTileset.js

+        /**
+         * This property is for debugging only; it is not optimized for production use.
+         * <p>
+         * When true, draws labels to indicate the number of commands used.


"commands used" -> "commands executed for this tile"

"each tile"

lilleyse · 2017-05-02T16:54:49Z

If going with the combined approach, it would make sense to hide number of triangles and number of points when those values are 0.

rahwang · 2017-05-02T17:27:43Z

Thanks for the suggestions, @pjcozzi and @lilleyse. Fixed!

lilleyse · 2017-05-03T14:38:12Z

Source/Scene/Cesium3DTileset.js

+                }
+
+                labelString += '\nFeatures: ' + tile.content.featuresLength;
+                attributes += 4;


This should be ++ now.

whoops! Fixed @lilleyse

lilleyse · 2017-05-03T16:11:20Z

Nice work @rahwang.

pjcozzi · 2017-05-03T17:53:50Z

Source/Scene/Cesium3DTile.js

@@ -142,6 +142,8 @@ define([
        }
        this._viewerRequestVolume = viewerRequestVolume;

+        this._commandsLength = 0;


This should have a readonly property getter.

Up to you, but this and the other "length" properties for rendering debugging output could be prefixed like _debugCommandsLength

pjcozzi · 2017-05-03T17:55:15Z

Source/Scene/Cesium3DTileset.js

@@ -1838,10 +1862,10 @@ define([

    var scratchCartesian2 = new Cartesian3();

-    function updateGeometricErrorLabels(tileset, frameState) {
+    function updateTileInfoLabels(tileset, frameState) {


TileInfo is very generic; we try to avoid names like "info." Perhaps updateTileDebugLabels.

Same for the _tileInfoLabels naming.

tileInfoVisible, etc.

rahwang added 3 commits April 20, 2017 11:00

creating tile info stats section and updating labels

169760b

fix label clipping

94de1f9

adding per tile commands used counting

5363042

rahwang force-pushed the per-tile-stats branch from 14f83ea to 5363042 Compare April 20, 2017 17:41

cleanup tweaks

3c28604

adding tests

a247ea7

rahwang added 2 commits April 27, 2017 11:14

moving debug label drawing from tilset to view model

19e4179

removing the original debug labels

f627ddd

rahwang added 3 commits April 27, 2017 15:02

Revert "moving debug label drawing from tilset to view model"

d67edf9

This reverts commit 19e4179.

Revert "removing the original debug labels"

8492203

This reverts commit f627ddd.

merge 3d-tiles into per-tile-stats

1af8c5a

rahwang force-pushed the per-tile-stats branch from 5adca6e to 1af8c5a Compare April 27, 2017 19:42

fix jshint

ca38f1a

lilleyse reviewed Apr 27, 2017

View reviewed changes

cleanup

134288f

adding feature count to debug labels

4719841

tweak

5f901dd

lilleyse reviewed Apr 28, 2017

View reviewed changes

cleanup

effbe7b

rahwang force-pushed the per-tile-stats branch from 8660f40 to effbe7b Compare April 28, 2017 19:17

switching memory usage stats to mb

8b0e599

lilleyse reviewed May 1, 2017

View reviewed changes

clean up edits

9bf5138

pjcozzi reviewed May 2, 2017

View reviewed changes

rahwang added 2 commits May 2, 2017 13:15

consolidate debug flags

58ce52a

hide point or triangle count when 0

3429968

lilleyse reviewed May 3, 2017

View reviewed changes

tweak

bed4f78

lilleyse merged commit fca40cb into CesiumGS:3d-tiles May 3, 2017

pjcozzi mentioned this pull request May 3, 2017

Per-tile memory usage statistics #5156

Closed

pjcozzi reviewed May 3, 2017

View reviewed changes

This was referenced May 9, 2017

cleaning up debug label names #5292

Merged

Only show picked tile debug labels #5325

Merged

hpinkos mentioned this pull request Jun 1, 2017

3D Tiles Inspector: report per-tile number of commnads/features/points/etc #5029

Closed

rahwang deleted the per-tile-stats branch June 26, 2017 13:01

		*/
		this.debugShowNumberOfPoints = defaultValue(options.debugShowNumberOfPoints, false);

Per tile stats #5188

Per tile stats #5188

Conversation

rahwang commented Apr 7, 2017

lilleyse commented Apr 7, 2017 • edited Loading

rahwang commented Apr 20, 2017

lilleyse commented Apr 20, 2017

rahwang commented Apr 26, 2017

rahwang commented Apr 27, 2017

lilleyse commented Apr 27, 2017 • edited Loading

rahwang commented Apr 27, 2017

lilleyse commented Apr 27, 2017

rahwang commented Apr 27, 2017

lilleyse commented Apr 27, 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

Choose a reason for hiding this comment

lilleyse commented Apr 28, 2017

rahwang commented Apr 28, 2017

rahwang commented Apr 28, 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

rahwang Apr 28, 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

rahwang commented Apr 28, 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

lilleyse commented May 1, 2017

rahwang commented May 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented May 2, 2017

rahwang commented May 2, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented May 3, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Apr 7, 2017 •

edited

Loading

lilleyse commented Apr 27, 2017 •

edited

Loading

rahwang Apr 28, 2017 •

edited

Loading