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

3D Tiles - Memory statistics for models and tiles #5050

Merged
merged 8 commits into from
Mar 10, 2017
Merged

Conversation

lilleyse
Copy link
Contributor

@lilleyse lilleyse commented Feb 28, 2017

For #3241

This PR adds support for getting the size in bytes of a texture, model, tile, and adds statistics for these to the inspector.

To-do:

  • Does not report memory correctly for i3dm tilesets since tiles share resources.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 8, 2017

Does not report memory correctly for i3dm tilesets since tiles share resources.

Two options

  • Do this as part of this PR
  • Add it to the roadmap for now and add a warning in the inspector report that i3dm tiles are not yet counted

I suggest the first unless there is something major stopping it.

@@ -144,6 +146,28 @@ define([
/**
* @private
*/
componentLength : function(pixelFormat) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Perhaps componentSizeInBytes? "Length" sounds like, for example, RGB would be three: R, G, and B.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This actually is for that case where RGB is three.

PixelDatatype figures out the sizeInBytes for each component, and the two numbers are multiplied for a final size.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

numberOfComponents? Or componentsLength (note the "s")?

/**
* @private
*/
textureSize : function(pixelFormat, pixelDatatype, width, height) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment throughout.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 8, 2017

Feel free to cherry pick some changes, like those in PixelFormat.js, into a PR into master.

@@ -313,6 +321,9 @@ define([
gl.bindTexture(target, this._texture);
gl.generateMipmap(target);
gl.bindTexture(target, null);

// The mipmap adds approximately 1/3 of the original texture size
this._sizeInBytes = Math.floor(this._sizeInBytes * 4 / 3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A user could call generateMipmap more than once. Either save the original size and recompute here, or just compute every time in the get function based on if this is mipmapped.

switch (pixelDatatype) {
case PixelDatatype.UNSIGNED_BYTE:
return 1;
case PixelDatatype.UNSIGNED_SHORT:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like this fall-through case style as well; I was going to suggest to use it in the PixelFormat.js changes. Perhaps it's a good idea.

@@ -561,6 +575,9 @@ define([
gl.bindTexture(target, this._texture);
gl.generateMipmap(target);
gl.bindTexture(target, null);

// The mipmap adds approximately 1/3 of the original texture size
this._sizeInBytes = Math.floor(this._sizeInBytes * 4 / 3);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same comment.

@@ -132,6 +134,21 @@ define([
this._textureStep = textureStep;
}

defineProperties(Cesium3DTileBatchTable.prototype, {
textureMemoryInBytes : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, I did not think about batch table vs. texture memory. Perhaps it is better to report these separately so it is less confusing to the end users that might not realize that the batch table is stored in a texture.

/**
* Part of the {@link Cesium3DTileContent} interface.
*/
vertexMemoryInBytes : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Throughout "MemoryInBytes" should probably be "SizeInBytes."

*
* @private
*/
vertexMemoryInBytes : {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

When a model is cached, 3D Tiles should only report the memory for one model even if, for example, two b3dm tiles reference it.

get : function() {
var memory = 0;
var buffers = this._rendererResources.buffers;
for (var id in buffers) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here and below seem like something we should cache in a property.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 8, 2017

Looks great!

@lilleyse lilleyse force-pushed the memory-statistics branch 2 times, most recently from a3dcc24 to fa8d5b4 Compare March 9, 2017 21:47
@lilleyse lilleyse force-pushed the memory-statistics branch from fa8d5b4 to b9eee23 Compare March 9, 2017 22:00
@@ -316,7 +354,7 @@ define([
gltf : gltfView,
cull : false, // The model is already culled by the 3D tiles
releaseGltfJson : true, // Models are unique and will not benefit from caching so save memory
basePath : getBaseUri(this._url, true),
basePath : getAbsoluteUri(getBaseUri(this._url, true)),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This change here was added because tiles pointing to the same resources might have different paths due to ../../ and other path separators. In effect this normalizes the uri.

@lilleyse
Copy link
Contributor Author

lilleyse commented Mar 9, 2017

Updated now. I have a TODO left in the code for when #5051 is merged. The memory tracking in Model was written with that in mind, so it should be easy to handle a global texture cache.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 10, 2017

Updated now. I have a TODO left in the code for when #5051 is merged...

OK, will look at that as soon as I can.

@pjcozzi pjcozzi merged commit 0a2aceb into 3d-tiles Mar 10, 2017
@pjcozzi pjcozzi deleted the memory-statistics branch March 10, 2017 01:40
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.

2 participants