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 option to combine multiple tilesets #33

Merged
merged 22 commits into from
Dec 7, 2020
Merged

Add option to combine multiple tilesets #33

merged 22 commits into from
Dec 7, 2020

Conversation

baothientran
Copy link
Contributor

Fixes #19

Add new option to combine multiple tilesets into one.
For example: To combine Elevation with component selectors 1_1 and GSModels with component selectors 2_1 into one, we can do like this:

--combine=Elevation_1_1,GSModels_2_1

You can repeat that option multiple times to group different local tilesets into different global tilesets. For example: To group Elevation and GSModels into one tileset and group all GTModels in different tileset, we can do:

--combine=Elevation_1_1,GSModels_2_1 --combine=GTModels_1_1,GTModels_2_1

With or without the option, the converter still groups each dataset from different GeoCells into a different global tileset for each dataset:

For example, if CDB has two GeoCells with coordinates (N32, W118) and (N32, W119), and each GeoCell has elevation and GSModels dataset, the tiler will combine all the elevation of all GeoCell into one global tileset for Elevation. Same with GSModels. Below is the new organization for global tilesets:

Screenshot_20201123_131852

@lilleyse
Copy link
Contributor

The 3D Tiles Validator is picking up a small error:

slilley@Lithium:~/Code/3d-tiles-validator/validator$ node bin/3d-tiles-validator.js -i ~/Downloads/San_Diego/Elevation_1_1GSModels_1_1GTModels_1_1GTModels_2_1.json 
Tileset must declare its geometricError as a top-level property.

@baothientran
Copy link
Contributor Author

baothientran commented Nov 23, 2020

@lilleyse I've updated the fix in the new commit and update tests to check it as well

@baothientran
Copy link
Contributor Author

@lilleyse I just realize I have an unused function combineTilesetJson(const std::vector<std::filesystem::path> &tilesetJsonPaths, const Core::BoundingRegion &regions, std::ofstream &fs);. This function was in TileFormatIO.h and TileFormatIO.cpp. This is now removed in the new commit

@lilleyse
Copy link
Contributor

lilleyse commented Nov 24, 2020

Overall, the way you've structured the tileset files makes a lot of sense. During testing I did have a few observations that I think could improve this PR.

  • The converter will process all the datasets even if some of them will not be included in the combined tilesets. I think better behavior would be to only process those datasets that are needed. This will speed up processing time if someone is only concerned about one dataset. (Chose which layers are included in the 3D Tiles output #27)
  • The converter should error out if you use a component selector that doesn't exist. E.g. --combine Elevation_1_5 (The converter should error out if you use a component selector that doesn't exist #37)
  • An invalid tileset is produced if only a single dataset is used like --combine Elevation_1_1. The tileset.json will refer to itself recursively.
  • The relationship between elevation and imagery is a little unclear. I think elevation and imagery should both be supplied in --combine if you want to combine them into the same tileset. Someone may want to convert elevation without the imagery. However, the converter would need to print an error if imagery is converted and elevation is not. (Seperate Elevation and Imagery in the --combine option #38)
  • We should consider updating the datasets table in the README so that it follows the Datasets_CS1_CS2 format, that way it's easier to know what datasets can be combined together. (The converter should error out if you use a component selector that doesn't exist #37)
  • If --combined is only supplied once or not supplied at all (e.g. not --combine=Elevation_1_1,GSModels_1_1 --combine=GTModels_2_1,GTModels_1_1) then I think the tileset should be called tileset.json instead of, say, Elevation_1_1GSModels_1_1GTModels_2_1GTModels_1_1.json so it's very obvious that it's the root tileset.
  • The tilesets in the geocell folders should no longer be called tileset.json and instead should follow a naming convention similar to the b3dm tiles.
  • The minimum height and maximum height of the regions in the tileset JSON files all seem to be 0.0 (Generate tighter bounding region for tile #39)

@baothientran
Copy link
Contributor Author

I've created issues for the remaining items:

@lilleyse
Copy link
Contributor

lilleyse commented Dec 7, 2020

Thanks for opening the issues @baothientran. I verified the fixes for everything that was checked off. I also ran the converter and loaded the tilesets in CesiumJS to confirm that nothing strange happened.

I briefly looked at the code, didn't have any comments.

@lilleyse lilleyse merged commit 689bd96 into master Dec 7, 2020
@lilleyse lilleyse deleted the tileset-combine branch December 7, 2020 21:39
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.

Provide utility to let client combine multiple converted tilesets together
2 participants