Recompute sector bounds when nodes are transformed

[eiriklegernaes]

Type of change

(or feature?)

Description 📝

Overview

When model nodes are transformed (using CogniteCadModel.setNodeTransform) they might end up outside the original sector bounds. This in turn may lead to a situation where a transformed node is culled erroneously, because the original sector bounds are outside the view frustum. This PR fixes this problem by dynamically growing the sector bounds to account for any transformed nodes in the sector. This assumes the number of transformed nodes are kept relatively low. If every node is transformed a lot, the sector tree will not be very efficient. That being said, the problem of disappearing model nodes is a correctness problem, and it's trivial to encounter. I therefore think this trade off is acceptable.

Details

Before sector bounds can be changed, it is necessary to know which sector a certain tree index has geometry in. In the future, this could be pre-computed and accessed through the backend, but as of now this is done by iterating over the sector data when a sector is first loaded. This is stored in a map, and only done once per sector per geometry type.

A Cognite3DModel now has an instance of the CustomSectorBounds class. When this class is given the following info

• The tree index of the transformed node
• The transform
• The set of sectors node has geometry in

it is able to adjust the sector bounds so that each sector still contains the transformed nodes. Both the transform and the set of sectors may be updated, meaning this book-keeping works even when transforming a node before any sectors are loaded. When transformed nodes are reset back to their original transform, the sector bounds will shrink back to the original value if possible.

Performance impact

The performance impact of the TreeIndexToSectorsMap book-keeping appears to be low, but it's hard to supply conclusive evidence. I've measured the time it takes to loop over the data in a sector, and it's usually roughly around ~100us (sometimes probably due to timer resolution). There are many sectors, but the impact doesn't appear to be large relative to other operations which are performed on each sector.

I've also attempted to measure the memory footprint of this map, as it currently doesn't employ any bounding mechanism. After navigating through a medium sized asset for about 10 minutes, I've measured it to be about 30 MB. This is about 10% of the total footprint, with the majority being buffers with what I assume is sector data (InstancedInterleavedBuffer and BufferAttribute arrays). A sophisticated bounding scheme could be used, dropping entries for tree indices in sectors haven't been loaded in a long time, but this would require more book-keeping.

The only public facing change should be that CogniteCadModel.setNodeTransform is now async.

How has this been tested? 🔍

Both classes have accompanying tests. The non-trivial logic in CustomSectorBounds is tested on a mocked up sector tree, and should hopefully give confidence that the logic is correct. It's also manually tested in the example viewer, by modifying it to translate nodes on click, and visualizing the sector bounds, but it's very hard to test edge cases this way.

[codecov]

Codecov Report

Merging #3391 (dd414e3) into master (d197669) will increase coverage by 0.13%.
The diff coverage is 81.52%.

❗ Current head dd414e3 differs from pull request most recent head 53e6ba2. Consider uploading reports for the commit 53e6ba2 to get more accurate results

@@            Coverage Diff             @@
##           master    #3391      +/-   ##
==========================================
+ Coverage   71.52%   71.65%   +0.13%     
==========================================
  Files         344      346       +2     
  Lines       33241    33667     +426     
  Branches     2583     2640      +57     
==========================================
+ Hits        23776    24125     +349     
- Misses       9361     9438      +77     
  Partials      104      104              

Impacted Files	Coverage Δ
...er/packages/cad-geometry-loaders/src/CadManager.ts	64.31% <13.04%> (-5.41%)	⬇️
...packages/cad-model/src/wrappers/CogniteCadModel.ts	78.65% <13.46%> (-6.12%)	⬇️
...d-model/src/batching/MultiBufferBatchingManager.ts	94.59% <91.66%> (-0.24%)	⬇️
...ages/cad-model/src/utilities/CustomSectorBounds.ts	95.27% <95.27%> (ø)
...s/cad-model/src/utilities/TreeIndexToSectorsMap.ts	98.63% <98.63%> (ø)
viewer/packages/cad-model/src/wrappers/CadNode.ts	86.81% <100.00%> (+1.17%)	⬆️
.../packages/utilities/src/three/VariableWidthLine.ts	87.50% <100.00%> (ø)

[haakonflatval-cognite]

Very nice feature! Thanks for the contribution!

I've made a few comments regarding code style, but otherwise I'm very happy with this.
I've made some testing on the impact of the extra tree-index-sector registration done on loading times, and I've seen about 3-6% lower FPS during the test run. But I'd say that's within tolerance.