Refactor Tileset Traversal #5254

austinEng · 2017-04-28T15:35:16Z

Traversal and functions relevant to traversal have been moved into a new file Cesium3DTilesetTraversal.js
Tileset has an added property baseScreenSpaceError which defines the screen space error which must be met before LOD skipping starts
Traversal has been generalized as a pipeline of tree traversals. The base traversal is a DFS, the leaves of that are used to initialize a BFS for the skipping traversal which internally uses a DFS. The traversal has been refactored so each traversal method has functions execute, visit, getChildren, shouldVisit, and leafHandler.
To prevent tiles from being processed or requested twice (which would happen when the leaf of one traversal becomes the root of the following, _requestedFrame and _lastVisitedFrame have been added to Cesium3DTile
Some loops after the old traversal have been removed and simplified by keeping track of _ancestorWithContent and _ancestorWithLoadedContent during the top-down traversal. It is no longer necessary to do a bottom-up traversal.
_sse has been renamed to _screenSpaceError
SettingskipLODs = false now just sets baseScreenSpaceError to _maximumScreenSpaceError and runs a base traversal.
The refactored traversal better manages the _hasMixedContent flag so that the stencil test is only applied when necessary. Previously, it would always set _hasMixedContent to true if we had to go up the tree to select a tile. It is more correct to do this only when _selectionDepth > 0 which implies a selected tile has a selected ancestor. Tests have been updated to account for this.

…avoid double loading

lilleyse · 2017-04-28T18:25:55Z

Source/Scene/Cesium3DTile.js

@@ -338,6 +339,10 @@ define([
        this._selectionDepth = 0;
        this._lastFinalResolution = undefined;


No longer used.

lilleyse · 2017-04-28T18:34:20Z

Source/Scene/Cesium3DTileset.js

-        if (b.distanceToCamera === 0 && a.distanceToCamera === 0) {
-            return b._centerZDepth - a._centerZDepth;
+    function sortForLoad(a, b) {
+        var diff = a.distanceToCamera - b.distanceToCamera;


Rename to distanceDifference and screenSpaceErrorDifference

lilleyse · 2017-04-28T18:44:09Z

Source/Scene/Cesium3DTileset.js

@@ -124,6 +126,7 @@ define([
     * @param {Boolean} [options.debugShowGeometricError=false] For debugging only. When true, draws labels to indicate the geometric error of each tile
     * @param {ShadowMode} [options.shadows=ShadowMode.ENABLED] Determines whether the tileset casts or receives shadows from each light source.
     * @param {Boolean} [options.skipLODs=true] Determines if level-of-detail skipping optimization should be used.
+     * @param {Number} [options.baseScreenSpaceError=1024] The screen-space error that must be reached before skipping LODs
     * @param {Number} [options.skipSSEFactor=10] Multiplier defining the minimum screen space error to skip when loading tiles. Used in conjuction with skipLevels to determine which tiles to load.


While we're doing the other renamings, this could be skipScreenSpaceErrorFactor.

lilleyse · 2017-04-28T19:21:15Z

Source/Scene/Cesium3DTilesetTraversal.js

+
+        //>>includeStart('debug', pragmas.debug);
+        Check.typeOf.number.greaterThanOrEquals('baseScreenSpaceError', baseScreenSpaceError, this.tileset._maximumScreenSpaceError);
+        //>>includeEnd('debug');


This should be checked in Cesium3DTileset somewhere, or always just clamped.

lilleyse · 2017-04-28T19:28:51Z

Source/Scene/Cesium3DTilesetTraversal.js

+
+        if (defined(stack.trim)) {
+            stack.trim(maxLength);
+        }


When would stack.trim be undefined?

Also this comment still applies right?

// on the next frame, we will likely need an array about the same size

Could be worth adding back.

lilleyse · 2017-05-01T15:00:14Z

Source/Scene/Cesium3DTilesetTraversal.js

+                                child.selected = true;
+                                child._finalResolution = true;
+                                child._selectedFrame = frameState.frameNumber;
+                            }


Same comment here.

lilleyse · 2017-05-01T15:04:31Z

Source/Scene/Cesium3DTilesetTraversal.js

+        if (tile.hasTilesetContent) {
+            if (!tile.contentReady) {
+                return emptyArray;
+            }


Same as before, is this check needed? Plus the situation where this actually returns the empty array is somewhat low, since tile.hasTilesetContent will be false until the content has been loaded.

lilleyse · 2017-05-01T15:05:01Z

Source/Scene/Cesium3DTilesetTraversal.js

+        }
+
+        var childrenVisibility = updateChildren(tileset, tile, this.frameState);
+        var showAdditive = tile.refine === Cesium3DTileRefine.ADD && tile._screenSpaceError > maximumScreenSpaceError;


Same comment about the SSE check being redundant.

It's needed if tile.hasTilesetContent === true

lilleyse · 2017-05-01T15:21:14Z

Source/Scene/Cesium3DTileset.js

@@ -263,6 +267,7 @@ define([
        this.show = defaultValue(options.show, true);

        this._maximumScreenSpaceError = defaultValue(options.maximumScreenSpaceError, 16);
+        this._baseScreenSpaceError = defaultValue(options.baseScreenSpaceError, 1024);


I'm getting a crash when changing the the maximum sse in the inspector with skipLODs as false.

lilleyse · 2017-05-01T16:06:32Z

Source/Scene/Cesium3DTilesetTraversal.js

+            var length = leaves.length;
+            for (var i = 0; i < length; ++i) {
+                tileset._desiredTiles.push(leaves.get(i));
+            }


When skipLODs is false, should the bivariateVisibilityTest ever be on? I am seeing some cases where it is.

lilleyse · 2017-05-01T16:17:00Z

After testing the old performance/ behavior is still there which is good, minus the low-lod geometry blips that happened sometimes. I like the way the traversal code is organized now.

lilleyse · 2017-05-02T15:42:37Z

The new code doesn't use the child union optimization. Is that intentional?

…e traversal

austinEng · 2017-05-02T20:05:25Z

@lilleyse Updated based on comments.

When would stack.trim be undefined?

Whoops. Just an old thing

The new code doesn't use the child union optimization. Is that intentional?

Accidentally removed. Added back.

tile._screenSpaceError > baseScreenSpaceError will always be true by this point.

Fixed. The one with maximum screen space error isn't always true. We need the check if tile.hasTilesetContent === true

pjcozzi · 2017-05-03T18:12:58Z

@austinEng have you ran a few JavaScript profiles to make sure this didn't introduce any high CPU or memory usage?

pjcozzi · 2017-05-03T18:13:38Z

There's a few CI test failures: https://travis-ci.org/AnalyticalGraphicsInc/cesium/builds/228357786

austinEng · 2017-05-04T14:46:17Z

@lilleyse Updated.

Spent a lot of time trying to solve what seemed to be bugs in the traversal. This turned out to be problems with translucency. Translucent commands were being put in the CESIUM_3D_TILE render pass and taken out of the translucency pass. Tilesets with translucent geometry seemed to be randomly occluding content behind them. @bagnell I've added a check so that translucent commands do not get put in the CESIUM_3D_TILE pass.

@pjcozzi Performance and memory usage is about the same.

lilleyse · 2017-05-05T14:57:29Z

Source/Scene/Cesium3DTilesetTraversal.js

+    };
+
+    function baseUpdateAndCheckChildren(tileset, tile, baseScreenSpaceError, frameState) {
+        if (tile.hasTilesetContent) {


I still don't think this block is needed.

I think you're right, but I think we should then add to the condition below

tile._screenSpaceError <= baseScreenSpaceError && !tile.hasTilesetContent

so that the base traversal doesn't stop at the root of external tilesets

lilleyse · 2017-05-05T15:04:07Z

Source/Scene/Cesium3DTilesetTraversal.js

+        // stop traversal when we've attained the desired level of error
+        if (tile._screenSpaceError <= baseScreenSpaceError) {
+            // When skipping LODs, require an existing base level of content first
+            updateChildren(tileset, tile, frameState);


I don't completely see why the children are updated here.

leafHandler checks if at least one child is visible for the children union bound optimization

Ok, leave a comment about that.

lilleyse · 2017-05-05T15:04:24Z

Source/Scene/Cesium3DTilesetTraversal.js

+    }
+
+    InternalBaseTraversal.prototype.execute = function(root) {
+        this.allLoaded = true;


Should be defined in the class.

lilleyse · 2017-05-05T15:05:10Z

Source/Scene/Cesium3DTilesetTraversal.js

+    BaseTraversal.prototype.visitStart = function(tile) {
+        if (tile._lastVisitedFrame !== this.frameState.frameNumber) {
+            visitTile(this.tileset, tile, this.frameState, this.outOfCore);
+        }


This last visited check is pretty much the same throughout, why not put back in visitTile?

Also this goes to the second point, are visitStart and visitEnd needed? If InternalBaseTraversal.prototype.visitStart is changed based on other comments, visitStart would be the same throughout. The code in visitEnd can be incorporated in visit along with my comment right above.

visitStart

getChildren

if no children, leafHandler

visitEnd

sometimes the leaf handler checks tile._lastVisitedFrame !== this.frameState.frameNumber so we don't add tiles twice

For that case it can stay, but do you think the others can be simplified?

Never mind, I see now. Maybe a short isVisited helper would be cleaner.

lilleyse · 2017-05-05T15:08:57Z

Source/Scene/Cesium3DTilesetTraversal.js

+        }
+    }
+
+    function touch(tileset, tile, outOfCore) {


Should there be a touchedFrame? Some things will be touched twice - first in getChildren and then visitTile.

Is it worth adding this? The other flags are important because they prevent tiles from getting put onto a list twice, but this just moves the tile in the doubly linked list.

Well it's a bit like the sse check.

lilleyse · 2017-05-05T15:12:24Z

Source/Scene/Cesium3DTilesetTraversal.js

+        }
+        if (!tile.contentReady) {
+            this.allLoaded = false;
+        }


Since shouldVisit() will return false for any tile with renderable content, this code would not got reached for those tiles. This would create a situation allLoaded is true even if one of its descendants with renderable content is not ready.

…s when multiple final tiles have different selection depths

austinEng · 2017-05-05T21:48:17Z

@lilleyse Updated

lilleyse · 2017-05-08T13:51:42Z

Thanks @austinEng, the recent updates look good.

austinEng added 11 commits April 26, 2017 10:39

initial work to load base sse first

d53e065

refactor traversal

53da697

skip after base traversal completed

ecb1d70

set base error to max error when skip LODs is disabled; add flags to …

4d640ae

…avoid double loading

remove double visits

815628c

fix selection for additive refinement

c714501

bugfixes and update tests

85ff19c

cleanup

eedd70f

Merge remote-tracking branch 'agi/3d-tiles' into improved-tile-loading

6b61a4a

unused flags

4d976f9

update sort for load

a9786fb

austinEng force-pushed the improved-tile-loading branch from 33cc46b to a9786fb Compare April 28, 2017 15:39

austinEng mentioned this pull request Apr 28, 2017

Add Skip LOD options to 3D Tiles Inspector #5239

Closed

lilleyse reviewed May 1, 2017

View reviewed changes

lilleyse mentioned this pull request May 1, 2017

3D Tiles - Support tile expiration #4136

Merged

6 tasks

austinEng added 3 commits May 2, 2017 12:01

updates; wait for nearest descendents to load before refining for bas…

0e6b512

…e traversal

add children union culling optimization

67964c0

bugfixes

2b858d6

austinEng force-pushed the improved-tile-loading branch from 777c3b9 to 2b858d6 Compare May 2, 2017 19:53

fix root of tileset of tilesets

50ec7c8

austinEng force-pushed the improved-tile-loading branch from c2ca6ff to 50ec7c8 Compare May 3, 2017 14:16

austinEng added 3 commits May 4, 2017 09:36

test fixes and avoid adding twice to _desiredTiles

1170c92

Merge remote-tracking branch 'agi/3d-tiles' into improved-tile-loading

f6853ab

do not put translucent commands in CESIUM_3D_TILE render pass

82570f1

use ManagedArray for temporary BFS front

1326d5b

austinEng force-pushed the improved-tile-loading branch 2 times, most recently from 8099a04 to b8ccf80 Compare May 4, 2017 16:14

optimizations

80806ea

austinEng force-pushed the improved-tile-loading branch from b8ccf80 to 80806ea Compare May 4, 2017 17:05

lilleyse reviewed May 5, 2017

View reviewed changes

austinEng added 3 commits May 5, 2017 13:41

backface commands for all tiles, not just unresolved tiles. fixes bug…

538ab35

…s when multiple final tiles have different selection depths

updates

3bd95ab

Merge remote-tracking branch 'agi/3d-tiles' into improved-tile-loading

b653d6c

lilleyse merged commit 85fd29c into CesiumGS:3d-tiles May 8, 2017

SunBlack mentioned this pull request May 24, 2017

Crash in Cesium3DTilesetTraversal #5353

Open

jbo023 mentioned this pull request Jun 16, 2017

3D Tiles loading to many tiles #5477

Closed

SunBlack mentioned this pull request Aug 24, 2017

3D Tiles selection bug? #5766

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Refactor Tileset Traversal #5254

Refactor Tileset Traversal #5254

austinEng commented Apr 28, 2017

lilleyse Apr 28, 2017

lilleyse Apr 28, 2017

lilleyse Apr 28, 2017

lilleyse Apr 28, 2017

lilleyse Apr 28, 2017

lilleyse May 1, 2017

lilleyse May 1, 2017

lilleyse May 1, 2017

austinEng May 2, 2017

lilleyse May 1, 2017 •

edited

Loading

lilleyse May 1, 2017

lilleyse commented May 1, 2017

lilleyse commented May 2, 2017

austinEng commented May 2, 2017 •

edited

Loading

pjcozzi commented May 3, 2017

pjcozzi commented May 3, 2017

austinEng commented May 4, 2017

lilleyse May 5, 2017

austinEng May 5, 2017

lilleyse May 5, 2017

austinEng May 5, 2017

lilleyse May 5, 2017

lilleyse May 5, 2017

lilleyse May 5, 2017

lilleyse May 5, 2017

austinEng May 5, 2017

lilleyse May 5, 2017

lilleyse May 5, 2017

lilleyse May 5, 2017

austinEng May 5, 2017

lilleyse May 5, 2017

lilleyse May 5, 2017

austinEng commented May 5, 2017

lilleyse commented May 8, 2017

		@@ -338,6 +339,10 @@ define([
		this._selectionDepth = 0;
		this._lastFinalResolution = undefined;

Refactor Tileset Traversal #5254

Refactor Tileset Traversal #5254

Conversation

austinEng 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

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 May 1, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented May 1, 2017

lilleyse commented May 2, 2017

austinEng commented May 2, 2017 • edited Loading

pjcozzi commented May 3, 2017

pjcozzi commented May 3, 2017

austinEng commented May 4, 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

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 May 5, 2017

lilleyse commented May 8, 2017

lilleyse May 1, 2017 •

edited

Loading

austinEng commented May 2, 2017 •

edited

Loading