-
Notifications
You must be signed in to change notification settings - Fork 3.6k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Add basic memory statistics in ModelExperimental #10397
Conversation
Thanks for the pull request @ptrgags!
Reviewers, don't forget to make sure that:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Did a pass through the code to make a checklist of files to add unit tests for. Got a couple of possible leads for unit test debugging along the way.
@j9liu updated! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ptrgags did another sweep -- just a few cases where the stage assumes things exist, when in reality they're not required by the schema.
}); | ||
}); | ||
|
||
it("_countMaterialTextures updates memory statistics for specular glossiness", function () { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we test that _countMaterialTextures
doesn't update for a model without material textures? (e.g. simple texture model, or simple model without a material?)
Also I don't think getAllTextureReaders
accounts for if a model doesn't have a material
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried this with Triangle.gltf
and discovered that it still has a default material! Though surprisingly, it wasn't any GltfLoader code, it was https://github.com/CesiumGS/gltf-pipeline this time! I left some comments explaining this, because it was non-obvious
@ptrgags also, does this need an entry in |
@j9liu updated! |
@ptrgags I noticed a typo in |
This PR partially addresses #9886 by adding memory statistics to the various pipeline stages. Unfortunately, due to the global
ResourceCache
, it's hard to tell if memory has been shared across multiple models. Some things are likely double-counted in this case. I have some ideas for creating a separate global statistics, but that'll be in a separate PR.Along the way I tweaked
GltfIndexBufferLoader
to load the original indices as a Buffer in addition to the typed array. This way it doesn't need to be re-uploaded every time the ModelExperimental pipeline is re-run. It also simplifies some of of the code inbuildDrawCommands()
.To Do:
StructuralMetadata
need unit tests.Cesium3DTileset > verify memory usage statistics
the batch table statistics seems to have the right value, but the line inCesium3DTilesetStatistics
that increments the total for the tileset doesn't seem to be run until later. Not sure why that is, I haven't changed the tileset statistic code much.BatchTableHierarchy
that works when run in isolation but not when run with other tests in the same suite... sounds like some state is being modified between tests, but I haven't yet found the problem.ResourceCache.statistics
idea.