Add statistics to VoxelPrimitive #12463
Conversation
|
Thank you for the pull request, @lukemckinstry! ✅ We can confirm we have a CLA on file for you. |
| @@ -170,7 +170,6 @@ Cesium3DTilesetStatistics.clone = function (statistics, result) { | |||
| result.selected = statistics.selected; | |||
| result.visited = statistics.visited; | |||
| result.numberOfCommands = statistics.numberOfCommands; | |||
| result.selected = statistics.selected; | |||
There was a problem hiding this comment.
removing a duplicate line, not directly related to this PR
|
Thanks @lukemckinstry, I will take a pass soon! Could you please update the testing plan and |
|
@ggetz updated change log and test instructions, ready for review |
|
I had one comment on the inline documentation, but this is looking good to me! Over to @jjhembd for review and merge. |
improve inline docs Co-authored-by: Gabby Getz <gabby@cesium.com>
lukemckinstry
force-pushed
the
voxel-stats
branch
from
29c7f17 to
1a32118
Compare
There was a problem hiding this comment.
Thanks @lukemckinstry! This is looking good overall. I have a couple minor comments, and also some input on how textureByteLength is computed.
| primitive.allTilesLoaded.numberOfListeners > 0 || | ||
| primitive.initialTilesLoaded.numberOfListeners > 0; | ||
| if (!that._debugPrint && !checkEventListeners) { | ||
| return; |
There was a problem hiding this comment.
If there are no event listeners, can we exit before calling traverseRecursive? recursive traversal is not cheap.
There was a problem hiding this comment.
The way I have it set up now, statistics.numberOfTilesWithContentReady and statistics.visited rely on the stats gathered in the traversal. Perhaps these stats could be gathered another way during the loadAndUnload function?
There was a problem hiding this comment.
And unlike events where we can check if events have listeners subscribed we do not really know if anybody is seeking these stats, unless perhaps we had a flag to turn off/on collection of this data.
| scratchDimensions, | ||
| ); | ||
| primitive.statistics.textureByteLength = | ||
| VoxelTraversal.getApproximateTextureMemoryByteLength( |
There was a problem hiding this comment.
The texture memory size is set once in the Megatexture constructor. We might want to expose the actual size (something like (textureDimension * channelCount * componentByteLength) ** 2) in that class, rather than re-computing the "approximate" size.
Once the Texture is constructed, its size doesn't change. The occupiedCount number indicates how much of the texture is filled with useful data. But even if we haven't loaded anything yet, the texture has already been allocated and is using its full amount of memory.
I think the textureByteLength is mainly used to get an idea of how much memory has been allocated.
We also need to keep track of how many megatextures have been allocated. This is based on how many metadata fields the voxel dataset contains. See the new Megatexture() calls in the VoxelTraversal constructor.
| @@ -83,6 +83,9 @@ describe( | |||
| }); | |||
| expect(spyUpdate1.calls.count()).toEqual(1); | |||
| expect(spyUpdate2.calls.count()).toEqual(1); | |||
| expect(primitive.statistics.numberOfTilesWithContentReady).toEqual(1); | |||
There was a problem hiding this comment.
Can we update the name/description of this spec to indicate that we are checking more than just "initial tiles loaded and all tiles loaded events" ?
For performance & profiling purposes, add additional statistics from Cesium3DTilesetStatistics to be tracked in VoxelPrimitive as they are in Cesium3DTileset.
Based on my understanding of voxels it seems that selected and numberOfTilesWithContentReady mean the same thing in the voxel context, so I just implemented numberOfTilesWithContentReady.
Issue number and link
#12297
Follow up to PR #12430
Testing plan
VoxelPrimitive.statisticsare not exposed in the public api. A good way to test is to set debug statements and look at a sandcastle with voxels (either generated or loaded).VoxelTraversal.postPassesUpdate, console.log statistics.visited, statistics.numberOfTilesWithContentReady, statistics.textureByteLength after line 800. The first 2 stats are identical to the existing values forLOADEDandTOTALin the debug message, which can be turned on by settingVoxelTraversal._debugPrint = true. textureByteLength should roughly be a multiple ofLOADED.Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change