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

Move tileset cache to its own class #6391

Merged
merged 5 commits into from
Apr 11, 2018
Merged

Conversation

lilleyse
Copy link
Contributor

The 3D tiles caching code was a bit all over the place, this centralizes it to its own class.

@cesium-concierge
Copy link

Signed CLA is on file.

@lilleyse, thanks for the pull request! Maintainers, we have a signed CLA from @lilleyse, so you can review this at any time.

⚠️ I noticed that CHANGES.md has not been updated. If this change updates the public API in any way, fixes a bug, or makes any non-trivial update, please add a bullet point to CHANGES.md and comment on this pull request so we know it was updated. For more info, see the Pull Request Guidelines.


I am a bot who helps you make Cesium awesome! Contributions to my configuration are welcome.

🌍 🌎 🌏

@lilleyse lilleyse mentioned this pull request Mar 29, 2018
2 tasks
@lilleyse
Copy link
Contributor Author

Originally in #6390 but moved out into its own PR for easier reviewing.

@lilleyse lilleyse changed the base branch from master to request-performance March 29, 2018 20:32
*
* @private
*/
function Cesium3DTilesetCache() {
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you want to unit test this alone?

Copy link
Contributor Author

@lilleyse lilleyse Apr 4, 2018

Choose a reason for hiding this comment

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

I don't think it needs to be unit tested alone since it depends pretty heavily on having a tileset. The cache related tests in Cesium3DTileset are pretty good and this file has 100% coverage.

@pjcozzi
Copy link
Contributor

pjcozzi commented Mar 29, 2018

@likangning93 do you want to review this?

Copy link
Contributor

@likangning93 likangning93 left a comment

Choose a reason for hiding this comment

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

Just a couple small suggestions, otherwise this looks solid to me. I also couldn't identify why CI failed on the last commit, so I restarted that.

tile.unloadContent();
totalMemoryUsageInBytes = tileset.totalMemoryUsageInBytes;
}
tileset._cache.unloadTiles(tileset, unloadTile);
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this just be inlined now that it's a one-liner and it's only called from update?

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 kind of like how each operation on tiles has a specific name to it, e.g. processTiles, selectTiles, requestTiles, updateTiles, unloadTiles. Just a style preference.

@@ -1502,10 +1497,7 @@ define([
tileset._statistics.incrementLoadCounts(tile.content);
++tileset._statistics.numberOfTilesWithContentReady;

// Add to the tile cache. Previously expired tiles are already in the cache.
Copy link
Contributor

Choose a reason for hiding this comment

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

Should this doc be kept since checks on add are abstracted away now?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah yes - fixed.

CHANGES.md Outdated
@@ -35,6 +35,7 @@ Change Log
* Added support for ordering in `DataSourceCollection` [#6316](https://github.com/AnalyticalGraphicsInc/cesium/pull/6316)
* All ground geometry from one `DataSource` will render in front of all ground geometry from another `DataSource` in the same collection with a lower index.
* Use `DataSourceCollection.raise`, `DataSourceCollection.lower`, `DataSourceCollection.raiseToTop` and `DataSourceCollection.lowerToBottom` functions to change the ordering of a `DataSource` in the collection.
* Improved processing order of 3D tiles. [#6364](https://github.com/AnalyticalGraphicsInc/cesium/pull/6364)
Copy link
Contributor

Choose a reason for hiding this comment

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

Move to 1.45.

@lilleyse
Copy link
Contributor Author

lilleyse commented Apr 4, 2018

Updated - hopefully CI passes this time.

@pjcozzi
Copy link
Contributor

pjcozzi commented Apr 8, 2018

@likangning93 is this ready to merge?

@likangning93
Copy link
Contributor

@pjcozzi sorry for the slow response! Yes, this is ready to go. We were waiting on CI but that seems to be resolved now.

@pjcozzi pjcozzi merged commit 57627f4 into request-performance Apr 11, 2018
@pjcozzi pjcozzi deleted the tileset-cache branch April 11, 2018 13:48
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