First Refactoring for Tileset Json, Implicit Quadtree, and Implicit Octree#508
Merged
Conversation
kring
reviewed
Jun 6, 2022
Member
kring
left a comment
There was a problem hiding this comment.
Wow @baothientran, a huge piece of work here! Mostly it looks very good, but I have to admit it's a very challenging thing to review thoroughly. A bunch of comments below.
One small but important problem I noticed: it looks like this branch changes the version of a bunch of third-party libraries. Probably from doing a git add . after a pull without first doing a git submodule update --init.
This was referenced Aug 22, 2022
Add up axis to TileLoadResult
Member
|
Looks good! I'm going to merge this, do a final test with Unreal in refactor-staging-main, and then merge that, too. We'll need to update CHANGES.md somewhere along the way. |
Contributor
Author
|
thanks @kring! I will add a comment about Tile::shouldContentContinueUpdating along with Change.md in a separate PR |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Open this as draft since I still needs to finish unit testings and add comments to the API. This will be merged into a staging branch rather than main, to prevent any mistakes
Overview
This PR is the first patch to resolve #481. The main purposes of this PR are:
This is the new diagram for the new loading architecture:

What's new compared to #481?
TileContent
TileownsTileContentLoadResultstruct and check the existing of some of the fields in this struct to determine if the tile is external, empty, or renderable (link). The new refactor is more explicit about it by usingTileEmptyContent,TileExternalContent, orTileRenderContent. I think this is a lot more easy to debug and better-documentedTilesetContentManager
TilesetContentLoader), managing tile loading state machine, and unloading tile. No ones except the TilesetContentManager can set the state of the tile nowTilesetContentLoader
What's removed?
TileContentFactory and TileContentLoader:
GltfConverters. The interface will no longer acceptIAssetRequestand returnFuture<Model>. Instead, it accepts only binary content and convert it directly to gltf Model. Resolving external buffers and images are taken care of byTilesetContentManagerTileContext:
TilesetContentManagerTileset:
requestTileContent(),requestAvailabilitySubtree(), andloadTilesFromJsonare removed, and their implementations are moved to different correspondingTilesetContentLoadersLoadIonAssetEndpoint,LoadTilesetDotJson,LoadTileFromJson,LoadSubtreehelper structs are now removed and their implementations are moved to different correspondingTilesetContentLoaders.TilesetContentManagerwhich is the only object that manages tile loading states and usesTilesetContentLoaderto load tile content.Cesium Ion Asset IDandCesium Ion Token. This information is moved toCesiumIonTilesetContentLoaderwhich is an implementation ofTilesetContentLoaderinterfaceTilesetJsonLoaderwhich is an implementation ofTilesetContentLoaderinterfaceTilesetContentManagerwhich will take care all aspects of loading tiles.ImplicitTraversalpreviously was used to create children for implicit tiles during the traversal. This responsibility is now taken care of byTilesetContentLoader, so it is removed.Tile:
loadContent(),unloadContent(),processLoadedContent()are removed. Loading tile is the responsibilities ofTilesetContentLoadernowTilesetContentManagerwhich is the only class that can do it. No ones can set the state of the tile now