tiles: Tile traversal optimizations #1183

Avnerus · 2021-02-16T12:31:25Z

Hi! this is a long one.
As part of our work on Cesium 3d tiles with photogrammetry and three.js I ended up doing some changes to the loaders.gl tiles code which I detail in this PR:

Consolidate the tile getPriority function:
When working with heavy street-view-style photogrammetry tilesets, I always had to use requestThrottling, or the browser could not handle the amount of tiles being loaded. However, the priority function used for the request scheduler was using only lodMetricValue, not taking into account the distance to the camera. A priority function which resembles more the CesiumJS function and takes both sse and camera distance into account was used in the traversal code when loading the tile, I therefore moved that code to the tile and use that one instead, both on tile load and for request scheduling.
Regarding Skip VS Base traversal, we do not yet have a skip traversal implementation in three.js, which requires the use of the stencil buffer for tile depth (do you by the way have an implementation in deck.gl or other?). However, I have done some tests and noticed that there might be some lines in the traversal and priority functions that are specific for either skip or base traversal:

loaders.gl/modules/tiles/src/tileset/traversers/tileset-traverser.js

Lines 172 to 174 in 480b38a

if (!childRefines) {

return childRefines;

}

This line breaks the iteration of a parent tile if at least one child is not yet refined. I think this is specific for base traversal because in skip traversal you might want to load a certain child even if siblings are not loaded. This break does not happen in the original CesiumJS code. I then changed it to:

refines = refines && childRefines;
if (!refines && !skipLevelOfDetail) {
  return false;
}

And then here:

loaders.gl/modules/tiles/src/tileset/tile-3d.js

Lines 160 to 164 in 480b38a

    
           _getPriority() { 
        
             // Check if any reason to abort 
        
             if (!this.isVisible) { 
        
               return -1; 
        
             }

This line aborts the loading of a tile if it is not visible, but in base traversal you might need to load invisible tiles or else the parent would not refine. I had to change it to:

if (!this.isVisible && traverser.options.skipLevelOfDetail) {
  return -1;
}

So I could enable requestThrottling.

Finally, I added to safety checks related to tile unloading. This is related to the fact that if in order to get the traversal working properly in three.js, I had to intervene in the contentState process and decide myself when the tiles are definitely ready once all of the three.js objects are created (can be after an asynchronous gltf parsing operation). This intervening introduced some race conditions in which a tile's content was being accessed even after it was destroyed. Perhaps the proper way to fix this is to support a user defined Promise which defers the tile's contentState setting of READY or UNLOADED, but in the meantime I think it is enough to have those race conditions checks.

No urgency on this one as we are using our fork for now, but would appreciate your comments on these fixes.
Thank you!

/Avner

ibgreen · 2021-02-16T13:27:46Z

@Avnerus Really excited to see that you are making such in-depth use of the 3D Tiles loader.

Regarding your changes, they make perfect sense though I have to admit that they do require quite a bit of effort and context switching for me to review as it has been a while since I worked on this module, and we currently do not really have an active maintainer of the 3D tiles traversal parts.

Since you have done enough research to make changes of this caliber and you are maintaining your own fork of loaders.gl, how would you feel about becoming a maintainer (or even lead) of the 3d tiles module in loaders.gl?

As a maintainer you'd get write access to loaders.gl repo, ability to approve PRs, influence direction etc. We also have public open governance meetings and also steering committee meetings etc that you could join at your option.

Note that we do have active full-time maintainers on the I3SLoader and also the rest of loaders.gl, it is mainly the 3D tiles area that is poorly covered at the moment, so your scope would be limited.

If you are interested in learning more about how this could work, we can chat on the deck.gl forum or contact me via email in my profile.

Avnerus · 2021-02-16T16:47:10Z

Hi @ibgreen, that sounds really interesting. I'll send you an e-mail.
Thank you!
/Avner

Avnerus · 2021-02-26T10:54:39Z

Added to this PR fixes for empty traversal. The refinement check was not working correctly for nested tileset.jsons. I left still some TODOs to keep an eye on, but with this fix the nested traversal seems to be working well.

ibgreen

This looks good.

One of the biggest problems with our traversal is lack of unit tests - which makes any refactor of the already complicated code tricky to review. Any test you could add, no matter how trivial, would help.
We need to check that these changes work in I3S as well, as the tiles module is shared between both formats. You can start by running the I3S example, and @dryabinin-actionengine could also help you test.

ibgreen · 2021-02-26T18:16:54Z

modules/tiles/src/tileset/traversers/tileset-traverser.js

@@ -145,6 +144,8 @@ export default class TilesetTraverser {
      !skipLevelOfDetail && tile.refine === TILE_REFINEMENT.REPLACE && tile.hasRenderContent;

    let hasVisibleChild = false;
+    let refines = true;


Should we make refinement calculation into a separate helper function? And ideally add a unit test case?

Hi, about a helper function, at least for me it helps to try and mirror the CesiumJS code as much as possible so I can somewhat keep track of the differences.
Unit testing would definitely be good, but I will not have the time to add that soon.

ibgreen · 2021-02-26T18:22:19Z

modules/tiles/src/tileset/traversers/tileset-traverser.js

    while (stack.length > 0) {
      const tile = stack.pop();

+      /* TODO: This doesn't seem needed, should check further.


Would be great if we can sort this out.

So far, at least in my test cases, it seems to work well. The tiles are updated in the main traversal and I don't see any glitches.

These turned out to be needed after-all in a more complex model.

Avnerus · 2021-03-02T12:33:34Z

Regarding I3S, I can confirm at least that the examples here seem to work. but would be good if someone could make a performance comparison before/after the fixes. Also you could try adding throttleRequests: true to the options, which I believe should work correctly now.

ibgreen · 2021-03-02T16:23:32Z

@belom88 @dryabinin-actionengine Please test on I3S and land this PR asap.

belom88 · 2021-03-09T07:12:04Z

Looks like traverser requests much more tiles than before during refinement.

Switch to New York;
Wait for "tiles loading: 0";
Zoom in on particular area;
Tiles loading starts from 140-150.

https://drive.google.com/file/d/1HJWoSHTf8cViES0AJntC075cpXpObyEY/view?usp=sharing

Expected behavior (on master):
...
4. Tiles loading starts from 70-80 and loading time is less.

https://drive.google.com/file/d/1ECQEQ7bhzhAZgwhwbnFAMOng5SdXIg0M/view?usp=sharing

Avnerus · 2021-03-09T08:42:40Z

Thanks,
I checked a bit and it seems mostly related to the visibility check in the priority function.
In my use cases invisible tiles must be loaded unless using skip traversal, but in the New York case it seems it may work fine when only loading visible tiles.
I am currently anyway in the process of revisiting skip traversal so I would wait with this merge until I figure out more on how this should be done.
Additionally I would like to reach the state where throttleRequests is enabled by default to merge this properly.

Avnerus · 2021-05-31T20:19:23Z

Hi @belom88!
This is not yet the final PR for merge, but I updated the branch with my latest works and fixes on tiles.
Could you perhaps run another test on i3s and let me know how is the performance on your end? On my tests I seem to be getting equal or better results.
Thank you!
/Avner

ibgreen

@Avnerus Thanks for getting back to this. I think it would be great if you could split this into something like 3-4 PRs:

one simple PR bumping the dependencies and making any necessary changes to the webpack setup etc that we can land quickly (so you can rebase the remaining PRs)
one PR with the request throttling changes
one PR with the worker farm changes
one PR with the more delicate traversal changes

ibgreen · 2021-05-31T23:44:56Z

modules/worker-utils/src/lib/worker-farm/worker-farm.d.ts

@@ -19,7 +19,6 @@ export default class WorkerFarm {
  /** Get the singleton instance of the global worker farm */
  static getWorkerFarm(props: WorkerFarmProps): WorkerFarm;

-  readonly maxConcurrency: number;


Why is this deleted? This should be specifiable on the farm?

ibgreen · 2021-05-31T23:45:23Z

modules/worker-utils/src/lib/worker-farm/worker-farm.js

+
+    const maxConcurrency = isMobile ? this.props.maxMobileConcurrency : this.props.maxConcurrency;
+
+    if (workerPool && workerPool.maxConcurrency !== maxConcurrency) {


This is code that I still need to revisit, because it worked only before the worker logic was refactored. the goal here is to be able to change the maxConcurrency in runtime. I found it useful to be able to quickly test different concurrency configurations using the gui.

Perhaps we could make WorkerFarm.setProps({maxConcurrency}) call workerPool.setProps({maxConcurrency}) on all workerPools?

If so, WorkerPools may need some logic to destroy returned workers if the maxConcurrency has been reduced.

Yes this might be the missing piece. I will work on that one later on. Thanks!

Hi @ibgreen!
I revisited this code now and it does actually work. Earlier I just forgot to alias the new worker-utils module.
The getWorkerFarm method already calls setProps, so eventually maxConcurrency propagates from the load options down to the worker pool. Essentially with this code, every time a loader is passed a maxConcurrency that is different from the existing value, the worker threads are destroyed and then recreated with the updated number.
Do you think this solution can work or do you foresee an issue?

I don't see it.

The workerFarm just passes the props when it creates a new workerPool, not during workerFarm.setProps.

I think it could work if workerFarm iterated over all its current workerPools in setProps.

Avnerus · 2021-06-01T09:44:21Z

Hi @ibgreen, sorry I didn't clarify this. This branch now contains a copy of my fork, which will be split and cleaned. I just wanted to quickly get something out there so @belom88 is able to test the performance on i3s which I now see as the main issue blocking the changes, while I continue working on the PRs.

One note about splitting requestThrottling and other changes to tiles. The large sum of the traversal changes (the changes in the priority function) are required for throttling to work properly. The other part is the fixes for empty traversal, which could maybe be integrated to the same PR because I don't think it was ever working and in use. Other than those there are added options viewDistanceScale, which helped me a lot during debugging, updateTransforms, which improves performance when disabled if you don't plan to transform the tileset in runtime, and added information selectedDepth, which also helps with debugging and would benefit future implementations of skip traversal. Those could be split, but they don't introduce any breaking change.

belom88 · 2021-06-04T09:35:27Z

I checked it. Performance is OK now.
Another good thing is that it fixes overlapping tiles:
Before:

After:

It was mentioned by Alexey Polyashov in Slack.

Avnerus · 2021-06-04T11:27:51Z

Super!
Thank you for the update @belom88.

ibgreen · 2021-06-04T14:41:02Z

@Avnerus This is great news. Since we are wrapping up 3.0 it would be fantastic if we can get these changes landed soon. I'll prioritize review of any PRs you open.

Avnerus · 2021-06-04T20:19:17Z

Thank you! l believe by the end of next week I should have the final PRs ready.
My work also depends on #1436 so hopefully that would also be done in time for 3.0

ibgreen · 2021-06-04T21:57:53Z

@Avnerus Just to be confirm, sounds like you are not planning to do a PR for #1436 but would like us to do it.

Avnerus · 2021-06-06T07:02:19Z

@Avnerus Just to be confirm, sounds like you are not planning to do a PR for #1436 but would like us to do it.

I understood from @dryabinin-actionengine that he is already working on it, but if you haven't started yet I can also add it to my PR.

Avnerus · 2021-06-09T21:05:59Z

@ibgreen and @belom88, this PR now contains only the tile traversal changes. Here is recap and summary of the changes:

Consolidate the getPriority function to be used by both the traverser and request scheduler. Set requestThrottling as true by default, increase maxRequests to 64 by default and allow changing it via options.
Fix the empty traversal method which previously didn't work well (used when there are nested tileset jsons).
Minor changes in traversal so it matches the CesiumJS code and some extra safe-checks.
Revised documentation for throttleRequests and documentation for maxRequests.

Let me know your comments!

Thank yo

ibgreen

Looks pretty clean, given that I3S testing checks out I am fine with this.

ibgreen · 2021-06-09T21:14:19Z

modules/tiles/docs/api-reference/tileset-3d.md

@@ -68,8 +68,9 @@ Parameters:

 - `json`: loaded tileset json object, should follow the format [tiles format](https://loaders.gl/docs/specifications/category-3d-tiles)
 - `options`:
-  - `options.ellipsoid`=`Ellipsoid.WGS84` (`Ellipsoid`) - The ellipsoid determining the size and shape of the globe.
-  - `options.throttleRequests`=`true` (`Boolean`) - Determines whether or not to throttle tile fetching requests.
+  - `options.ellipsoid`=`Ellipsoid.WGS84` Determines whether or not to throttle tile fetching requestDetermines whether or not to throttle tile fetching request(`Ellipsoid`) - The ellipsoid determining the size and shape of the globe.


This looks strange, copy paste?

Yes, fixed!

ibgreen · 2021-06-09T21:16:00Z

modules/tiles/src/tileset/tile-3d.js

@@ -170,16 +170,32 @@ export default class TileHeader {
    }
  }

+  // If skipLevelOfDetail is off try to load child tiles as soon as possible so that their parent can refine sooner.


JSDoc style /** */ shows the comment when hovering in vscode.

ibgreen · 2021-06-09T21:18:42Z

modules/tiles/src/tileset/tile-3d.js

@@ -461,7 +478,7 @@ export default class TileHeader {
    this._centerZDepth = 0;
    this._screenSpaceError = 0;
    this._visibilityPlaneMask = CullingVolume.MASK_INDETERMINATE;
-    this._visible = false;
+    this._visible = null;


What is the type of _visible? Can it be set to null? We have a typescript PR about to land.

Would it work as optional _visible?: boolean?
I used this to indicate when the visibility is not at all updated (headless, tile-covnerter) so the scheduler will still process the request.

If you need, fine. But I certainly wish we didn't need so many optional fields. The firmer we can type a field the better. More variations => more bugs.

Well, another way would be to add a second boolean such as headless which would be falsified when visibility is updated, but I am not sure if it is more readable.
How about using undefined and defining in Typescript:
_visible: boolean | undefined ? I'm not sure what's the standard way to type variables that have an optional value (like this Rust equivalent) or this one).

Sure. If it is only this field that needs three states, then _visible?: boolean would work as opposed to having two fields just for this. However if we need to return this to the undefined state at some point, I'd rather use null (_visible: boolean | null.

If there were multiple fields that could be controlled by one common field that indicates whether things are initialized, then I'd personally separate it out.

This is all moot before we actually land the typescript branch.

OK. I think then we can stick with _visible?: boolean for this one. Seems readable enough to me.

ibgreen · 2021-06-09T21:19:43Z

modules/tiles/src/tileset/tileset-3d.js

@@ -551,7 +555,7 @@ export default class Tileset3D {
  }

  _unloadTile(tile) {
-    this.gpuMemoryUsageInBytes -= tile.content.byteLength || 0;
+    this.gpuMemoryUsageInBytes -= (tile.content && tile.content.byteLength) || 0;


It would be good to maintain a gpuMemoryUsageInBytes on the tile so we didn't need to rake into its internals here. I am not sure content would usually be a typed array.

I am not sure what you mean? All I added here was a check that tile.content exists (I experienced a race condition in which the content was already gone) .

Sorry I see that I wasn't reviewing your change, I was reviewing the original code.

Ah ok. we can then maybe handle this later on.

ibgreen · 2021-06-09T21:22:13Z

modules/tiles/src/tileset/tile-3d.js

      return -1;
    }
    if (this.contentState === TILE_CONTENT_STATE.UNLOADED) {
      return -1;
    }

-    return Math.max(1e7 - this._priority, 0) || 0;
+    const parent = this.parent;
+    const useParentScreenSpaceError =


If you are able to add some more comments (given that you have spent time understanding the algorithm) that would be nice. Imagine the questions a new person would have when looking at this code.

Added some comments.

Avnerus · 2021-06-09T21:51:58Z

@ibgreen Added another change which I understood should be attached to this PR.
tile._selectionDepth indicates the traversal depth of the tile. This is mostly useful if trying to implement Skip Traversal with the use of the ZBuffer or other method for hiding tiles according to depthl
I added it to the documentation. There was a depth property in the documentation but it didn't really exist.
_selectionDepth is how it is called in CesiumJS.

Avnerus · 2021-06-11T18:29:06Z

@ibgreen @belom88 , this is now rebased to Typescript.

belom88

Cannot build the website on this branch;
If I try to do git merge master, there are conflicts

ibgreen approved these changes Feb 26, 2021

View reviewed changes

ibgreen assigned belom88 and dryabinin94 Mar 2, 2021

Avnerus force-pushed the tile-traversal-optimizations branch from 4a7e3b7 to 74005c3 Compare May 31, 2021 20:17

ibgreen reviewed May 31, 2021

View reviewed changes

Avnerus closed this Jun 6, 2021

Avnerus reopened this Jun 9, 2021

Avnerus closed this Jun 9, 2021

Avnerus force-pushed the tile-traversal-optimizations branch from cfb6974 to 1a59877 Compare June 9, 2021 16:43

Avnerus reopened this Jun 9, 2021

ibgreen approved these changes Jun 9, 2021

View reviewed changes

Rebased to typescript

24fb694

Avnerus force-pushed the tile-traversal-optimizations branch from a8b6161 to 24fb694 Compare June 11, 2021 18:25

typo fix

44d719f

belom88 reviewed Jun 15, 2021

View reviewed changes

belom88 merged commit 5e4d910 into visgl:master Jun 16, 2021

Avnerus mentioned this pull request Dec 20, 2023

Update the empty traversal algorithm to match CesiumJS, possible fix for low-res flickering #2847

Merged

	_getPriority() {
	// Check if any reason to abort
	if (!this.isVisible) {
	return -1;
	}


		const maxConcurrency = isMobile ? this.props.maxMobileConcurrency : this.props.maxConcurrency;

		if (workerPool && workerPool.maxConcurrency !== maxConcurrency) {

tiles: Tile traversal optimizations #1183

tiles: Tile traversal optimizations #1183

Conversation

Avnerus commented Feb 16, 2021

ibgreen commented Feb 16, 2021

Avnerus commented Feb 16, 2021

Avnerus commented Feb 26, 2021

ibgreen 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

Avnerus commented Mar 2, 2021

ibgreen commented Mar 2, 2021

belom88 commented Mar 9, 2021

Avnerus commented Mar 9, 2021

Avnerus commented May 31, 2021

ibgreen 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

Avnerus commented Jun 1, 2021 • edited Loading

belom88 commented Jun 4, 2021

Avnerus commented Jun 4, 2021

ibgreen commented Jun 4, 2021

Avnerus commented Jun 4, 2021

ibgreen commented Jun 4, 2021

Avnerus commented Jun 6, 2021 • edited Loading

Avnerus commented Jun 9, 2021

ibgreen 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

ibgreen Jun 9, 2021 • 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

Avnerus Jun 9, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Avnerus commented Jun 9, 2021

Avnerus commented Jun 11, 2021

belom88 left a comment

Choose a reason for hiding this comment

Avnerus commented Jun 1, 2021 •

edited

Loading

Avnerus commented Jun 6, 2021 •

edited

Loading

ibgreen Jun 9, 2021 •

edited

Loading

Avnerus Jun 9, 2021 •

edited

Loading