3D Tiles Traversal cleanup #6390

lilleyse · 2018-03-29T19:15:36Z

This is mainly a cleanup of the 3D Tiles traversal code. The base traversal and skip traversal are now one code path which should make it easier to understand and maintain. The fundamentals haven't changed.

Besides cleanup some of the fixes are

Fixed a bug where skip lods would load all descendant tiles, without skipping, until the parent is requested. For a static view deep in a photogrammetry tileset the number of tiles with content ready dropped from 65 to 53 after this fix. All other selection stats stayed the same.
Traditional replacement refinement (skip-lods off) is much faster since it doesn't wait for a tile to refine before traversing and loading more tiles. This will be good for vector tiles and other things that don't use skip-lods.
Replacement tiles now use SSE as their request priority rather than distance. Additive tiles continue to use distance. Subjectively it looks slightly better this way and loads faster when skip-lods is off. It should be straightforward to add other priority metrics now, like closeness to view center.
Sort requests by priority before issuing them. Less chance of getting cancelled requests.
Pick statistics in inspector are correct now
Debug outlines get drawn for empty tiles
Debug labels get drawn for external tilesets
If a tileset has all additive tiles it avoids the final selection traversal required by skip lods.

Includes changes from these PRs which should be merged before this:
~~#6240~~
~~#6247~~
~~#6364~~
~~#6391~~

~~Part of #6243~~

To do

More performance tests, especially for dynamic views
Continue to test out different tilesets

lilleyse · 2018-07-05T00:58:40Z

After too long.. updated.

ggetz

Looks a lot cleaner overall! Looks like adding new functionality like other tile priority function will be straightforward.

ggetz · 2018-07-20T20:18:56Z

Source/Scene/Cesium3DTilesetTraversal.js

-
-        if (getScreenSpaceError(tileset, tileset._geometricError, root, frameState) <= maximumScreenSpaceError) {
-            // The SSE of not rendering the tree is small enough that the tree does not need to be rendered
+        // The SSE of not rendering the tree is small enough that the tree does not need to be rendered


This comment isn't super clear - The root tile doesn't meet the SSE requirement, therefore the tree does not need to be rendered?

ggetz · 2018-07-20T20:24:03Z

Source/Scene/Cesium3DTilesetTraversal.js

-    function isVisibleAndMeetsSSE(tileset, tile, frameState) {
+    /**
+     * Traverse the tree and check if their selected frame is the current frame. If so, add it to a selection queue.
+     * Furthermore, this is a preorder traversal so children tiles are selected before ancestor tiles.


~~Furthermore~~

ggetz · 2018-07-20T20:27:31Z

Source/Scene/Cesium3DTilesetTraversal.js

+     * no deeper than 255. It is very, very unlikely this will cause a problem.
+     *
+     * NOTE: when the scene has inverted classification enabled, the stencil buffer will be masked to 4 bits. So, the
+     * selected tiles must be no deeper than 15. This is still very unlikely.


Can we check the selection depth at this point and throw an error if either of these cases arise? Since it's so unlikely to happen, it won't immediately be apparent what is happening.

Throwing an error is probably too extreme since the tileset can still render there just may be artifacts.

ggetz · 2018-07-25T17:14:11Z

Source/Scene/Cesium3DTile.js

+                color = Color.GREEN;
+            } else {
+                color = Color.RED;
+            }


A little picky, but since red usually means "bad" and green usually mean "good" or "go", I would mark the colors something like so:

!finalResolution -> Yellow
empty -> gray
hasContentBoundingVolume -> green
otherwise -> blue

I ended up going with:

!finalResolution -> yellow
empty -> gray
boundingVolume -> white
contentBoundingVolume -> blue (the code for drawing content bounding volumes is further down)

I like white as the "base" color because it's really neutral.

ggetz · 2018-07-25T17:15:35Z

Source/Scene/Cesium3DTile.js

@@ -253,14 +253,16 @@ define([
        this.hasTilesetContent = false;

        /**
-         * The corresponding node in the cache replacement list.
+         * The corresponding node in the cache.


I would briefly mention what the cache is used for here.

ggetz · 2018-07-25T17:19:26Z

Source/Scene/Cesium3DTile.js

@@ -458,13 +456,13 @@ define([
         */
        contentAvailable : {
            get : function() {
-                return this.contentReady || (defined(this._expiredContent) && this._contentState !== Cesium3DTileContentState.FAILED);
+                return (this.contentReady && this.hasRenderableContent) || (defined(this._expiredContent) && this._contentState !== Cesium3DTileContentState.FAILED);


Shouldn't contentReady determine if the content is renderable? Would it make more sence to make sure it's non-empty and also if the content is ready?

Overall, it seems like there is a mix of setting variables like hasRenderableContent explicitly, along with using states like this._contentState === Cesium3DTileContentState.READY. hadRenderableContent can be a getter that checks the state, but I think we should only be setting one or the other.

Yes this was confusing, I removed this.hasRenderableContent and instead check hasEmptyContent or hasTilesetContent as needed.

ggetz · 2018-07-25T17:23:12Z

Source/Scene/Cesium3DTilesetTraversal.js

+        var add = tile.refine === Cesium3DTileRefine.ADD;
+        if (add) {
+            return tile._distanceToCamera;
+        } else if (replace) {


Do we want to return some priority as a default, or leave the priority undefined?

Also you don't need the else if because of the early return above.

ggetz · 2018-07-25T17:39:33Z

Source/Scene/Cesium3DTilesetTraversal.js

+            if (hasEmptyContent(tile)) {
+                // Add empty tile just to show its debug bounding volume
+                addEmptyTile(tileset, tile, frameState);
+                loadTile(tileset, tile, frameState);


Why are we loading an empty tile? Won't it just return from the loadTile function?

hasEmptyContent is a catch-all for anything that has Empty3DTileContent or Tileset3DTileContent. The load is needed for the Tileset3DTileContent case to load the external tileset. I left a comment to clarify.

ggetz · 2018-07-25T17:47:28Z

Source/Scene/Cesium3DTilesetTraversal.js


-        tile._childrenVisibility = flag;
+    function executeEmptyTraversal(tileset, root, frameState) {
+        // Depth-first traversal that checks if all nearest descendants with content are loaded. Ignores visibility.


I believe you mentioned you'd like to combine this function with the normal traversal function if possible. Taking a look at both, I would say they are fine being separate as the logic would get too complex.

ggetz · 2018-07-25T17:49:33Z

Source/Scene/Cesium3DTilesetTraversal.js

+                    lastAncestor = tile;
+                    if (!traverse) {
+                        selectTile(tileset, tile, frameState);
+                        tile._finalResolution = true;


So _finalResolution is a tile marked for rendering at the end of this round of traversal, correct? I think we should call this _selectedForRender or something a little more specific (as it is used externally to this class for debug code).

_selectedForRender is a bit too broad, _finalResolution says a tile is rendered AND none of its descendants are rendered. I wonder if there's a better name specifically for that condition...

But that's where the !finalResolution -> yellow comes in, it indicates a tile and its descendant are rendering simultaneously and the tile is not entirely "resolved".

I did clean up some of the _finalResolution code though so that _finalResolution = false is set in one place instead of _finalResolution = true being set everywhere.

lilleyse · 2018-07-30T17:17:16Z

@ggetz Updated

lilleyse · 2018-08-09T22:38:27Z

@ggetz bump - we should merge this sooner than later so it has time to sit in master.

ggetz · 2018-08-10T14:37:24Z

Sorry for the delay @lilleyse!

Code looks good to me, the only thing I see is that http://cesium-dev.s3-website-us-east-1.amazonaws.com/cesium/traversal-cleanup/Apps/Sandcastle/index.html?src=3D%20Tiles%20Terrain%20Classification.html&label=3D%20Tiles takes a significantly longer time to load the tileset then it did previously. Is that a consequence of something in this PR?

lilleyse · 2018-08-10T21:34:38Z

I believe it is an unrelated bug - #6908.

ggetz · 2018-08-16T17:23:35Z

@lilleyse Yep, that fixed it!

Thanks for all the work here!

lilleyse added 30 commits March 29, 2018 15:12

Change request queue from a heap to a sorted array

d683e4d

Set highest priority for globe tiles that you are inside

366a1d2

Move tileset cache to its own class

3f09e0c

Temp

15700e4

Stable

7500d7d

Optimizations

b285439

Fix bug with skipping. Solid

b44933d

Combine base and skip traversal together

b975f1e

Remove touched frame checks

5a36e0b

Remove desiredTiles

66f977d

Better handling of processing tiles

e0a2a86

Sort children by distance in main traversal

dbdd7ef

Select empty tiles so their debug bounding volumes get drawn

2894551

Limit selection traversal by SSE and visibility

b9219db

Update tile outline properly

5e7e764

Cleanup

63ad7c3

Fix for outlines

c804dd3

Fixing tests

a6aad57

Cleanup

4bbacf8

Better handling of request volume

248c443

Include additive tiles in final selection

db216c1

Add empty tiles to a different selection list

1f51e9b

Reorganization of checking refinement

978f9f6

Fix geometric errors on TilesetOfTilesets

582a51f

Add back loadSiblings and fix more tests

49c005b

Fix 2 tests

4509017

Fix expiration

7d21281

Remove unneeeded scene.renderForSpecs

ca07240

Fix remaining tests and starting to reorg for pick pass

fae93f7

Fix picking statistics

72ec719

lilleyse added 4 commits July 4, 2018 15:54

Simplify visibility code and tweak comments

6bda10e

Tweak and fix tests to get better code coverage

79e77c7

Draw empty bounding volumes as a different color (green)

716c6a4

Comments and .peek

a0d5c5c

lilleyse force-pushed the traversal-cleanup branch from e49d8c2 to a0d5c5c Compare July 5, 2018 00:08

Updated CHANGES.md

6a8fcfa

lilleyse changed the base branch from request-performance to master July 5, 2018 00:52

Remove RequestQueue code

2705996

lilleyse force-pushed the traversal-cleanup branch from 23e71dc to 2705996 Compare July 5, 2018 00:58

lilleyse added 4 commits July 6, 2018 16:44

Added visited frame to tile

c8c981e

Fix update visibility code again

1dd5e68

Merge branch 'master' into traversal-cleanup

8df3ddb

Remove TODO

6e84390

ggetz requested changes Jul 25, 2018

View reviewed changes

lilleyse added 4 commits July 25, 2018 15:45

Merge branch 'master' into traversal-cleanup

e95a9a2

Updates

8d85e60

More concise handling of finalResolution

4fc6f7e

Remove tile.renderableContent

6991103

Merge branch 'master' into traversal-cleanup

30c0459

Merge branch 'master' into traversal-cleanup

b527503

ggetz approved these changes Aug 16, 2018

View reviewed changes

ggetz merged commit 939e4c3 into master Aug 16, 2018

ggetz deleted the traversal-cleanup branch August 16, 2018 17:24

lilleyse mentioned this pull request Sep 3, 2018

3D Tiles style fixes #6998

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

3D Tiles Traversal cleanup #6390

3D Tiles Traversal cleanup #6390

lilleyse commented Mar 29, 2018 •

edited

Loading

lilleyse commented Jul 5, 2018

ggetz left a comment

ggetz Jul 20, 2018

ggetz Jul 20, 2018

ggetz Jul 20, 2018

lilleyse Jul 28, 2018

ggetz Jul 25, 2018

lilleyse Jul 28, 2018

ggetz Jul 25, 2018

ggetz Jul 25, 2018

ggetz Jul 25, 2018

lilleyse Jul 28, 2018

ggetz Jul 25, 2018

ggetz Jul 25, 2018

ggetz Jul 25, 2018

lilleyse Jul 28, 2018

ggetz Jul 25, 2018

ggetz Jul 25, 2018

lilleyse Jul 28, 2018 •

edited

Loading

lilleyse commented Jul 30, 2018

lilleyse commented Aug 9, 2018

ggetz commented Aug 10, 2018

lilleyse commented Aug 10, 2018

ggetz commented Aug 16, 2018

3D Tiles Traversal cleanup #6390

3D Tiles Traversal cleanup #6390

Conversation

lilleyse commented Mar 29, 2018 • edited Loading

lilleyse commented Jul 5, 2018

ggetz 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

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 Jul 28, 2018 • edited Loading

Choose a reason for hiding this comment

lilleyse commented Jul 30, 2018

lilleyse commented Aug 9, 2018

ggetz commented Aug 10, 2018

lilleyse commented Aug 10, 2018

ggetz commented Aug 16, 2018

lilleyse commented Mar 29, 2018 •

edited

Loading

lilleyse Jul 28, 2018 •

edited

Loading