Skip to content
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

Merged
merged 59 commits into from
Jun 7, 2022

Conversation

ptrgags
Copy link
Contributor

@ptrgags ptrgags commented May 23, 2022

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 in buildDrawCommands().

To Do:

  • Sweep through the diff and see what other files like StructuralMetadata need unit tests.
  • In the test Cesium3DTileset > verify memory usage statistics the batch table statistics seems to have the right value, but the line in Cesium3DTilesetStatistics 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.
  • There's a test in 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.
  • In the 3D Tiles Inspector, the selected features/points/triangles are all returning 0, not sure why that is.
  • Sync with main and fix merge conflicts.
  • Update Add statistics to ModelExperimental, ModelExperimental3DTileContent #9886 to include details about the ResourceCache.statistics idea.

@cesium-concierge
Copy link

Thanks for the pull request @ptrgags!

  • ✔️ Signed CLA found.
  • CHANGES.md was not updated.
    • If this change updates the public API in any way, please add a bullet point to CHANGES.md.
  • ❔ Unit tests were not updated.
    • Make sure you've updated tests to reflect your changes, added tests for any new code, and ran the code coverage tool.

Reviewers, don't forget to make sure that:

  • Cesium Viewer works.
  • Works in 2D/CV.

Copy link
Contributor Author

@ptrgags ptrgags left a 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.

Specs/Scene/BatchTextureSpec.js Outdated Show resolved Hide resolved
Specs/Scene/BatchTableHierarchySpec.js Outdated Show resolved Hide resolved
Source/Scene/StructuralMetadata.js Outdated Show resolved Hide resolved
Source/Scene/PropertyTable.js Show resolved Hide resolved
Source/Scene/PointCloud3DTileContent.js Show resolved Hide resolved
Source/Scene/ModelExperimental/ModelFeatureTable.js Outdated Show resolved Hide resolved
@ptrgags
Copy link
Contributor Author

ptrgags commented Jun 6, 2022

@j9liu updated!

Copy link
Contributor

@j9liu j9liu left a 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.

Source/Scene/ModelExperimental/WireframePipelineStage.js Outdated Show resolved Hide resolved
});
});

it("_countMaterialTextures updates memory statistics for specular glossiness", function () {
Copy link
Contributor

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

Copy link
Contributor Author

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

Specs/Scene/ModelExperimental/MaterialPipelineStageSpec.js Outdated Show resolved Hide resolved
Specs/Scene/ModelExperimental/MaterialPipelineStageSpec.js Outdated Show resolved Hide resolved
Specs/Scene/ModelExperimental/MaterialPipelineStageSpec.js Outdated Show resolved Hide resolved
@j9liu
Copy link
Contributor

j9liu commented Jun 7, 2022

@ptrgags also, does this need an entry in CHANGES.md?

@ptrgags
Copy link
Contributor Author

ptrgags commented Jun 7, 2022

@j9liu updated!

@j9liu
Copy link
Contributor

j9liu commented Jun 7, 2022

@ptrgags I noticed a typo in CHANGES.md and a leftover console.log, so I just pushed those changes. Will merge when Travis passes!

@j9liu j9liu merged commit b70e22d into main Jun 7, 2022
@j9liu j9liu deleted the model-experimental-statistics branch June 7, 2022 18:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants