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

Updates to samples generator to reflect 3D Tiles 1.0 changes #149

Merged
merged 5 commits into from
Jul 9, 2018

Conversation

ggetz
Copy link
Contributor

@ggetz ggetz commented Jul 5, 2018

Updates for 3D Tiles 1.0 - CesiumGS/cesium#6697

TODO:

  • Set version to 1.0
  • Cleanup so tilesets do not use deprecated functionality by default

@ggetz ggetz requested a review from lilleyse July 5, 2018 15:09
@ggetz ggetz changed the title Add RTC_CENTER tileset and tilesets with URI content Updates to samples generator to reflect 3D Tiles 1.0 changes Jul 5, 2018
Copy link
Contributor

@lilleyse lilleyse left a comment

Choose a reason for hiding this comment

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

Thanks @ggetz.

@@ -252,7 +258,8 @@ var promises = [
createCompositeOfComposite(),
createCompositeOfInstanced(),
// Hierarchy
createHierarchy(),
createHierarchyExtension(),
createLegacyHierarchy(),
createHierarchyMultipleParents(),
createHierarchyNoParents(),
createHierarchyBinary(),
Copy link
Contributor

Choose a reason for hiding this comment

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

Just nitpicking on names. I think it's fine to drop Extension since the other hierarchy samples don't have that. And for symmetry, change createLegacyHierarchy to createHierarchyLegacy.

createHierarchyExtension
createLegacyHierarchy
createHierarchyMultipleParents
createHierarchyNoParents
createHierarchyBinary

to

createHierarchy
createHierarchyLegacy
createHierarchyMultipleParents
createHierarchyNoParents
createHierarchyBinary

});
return fsExtra.emptyDir(outputDirectory).then(function () {
return Promise.all(promises);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Tweak the promise style slightly:

return fsExtra.emptyDir(outputDirectory).then(function () {
    return Promise.all(promises);
});
return fsExtra.emptyDir(outputDirectory)
    .then(function() {
        return Promise.all(promises);
    });

Also thanks for removing the testing code I forgot about in this branch.

@@ -489,6 +506,12 @@ function createBatchedWithVertexColors() {
return saveBatchedTileset('BatchedWithVertexColors', tileOptions);
}

function createBatchedWithDataURI() {
return saveBatchedTileset('BatchedWithContentUri', undefined, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Should the tileset name be BatchedWithContentDataUri? Just so it's clear it's a data uri.

// TODO delete b3dm file
tilesetOptions.tileName = content;
return saveTilesetJson(tilesetPath, createTilesetJsonSingle(tilesetOptions), prettyJson);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

This section is a little awkward. Why not just create a data uri (synchronously) from b3dm rather than saving the file first?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also consider renaming tilesetOptions.tileName to tilesetOptions.contentUri throughout, and update documentation in createTilesetJsonSingle.

@@ -1478,7 +1522,7 @@ function createTilesetRefinementMix() {
geometricError : smallGeometricError,
refine : 'ADD',
content : {
url : 'parent.b3dm',
uri : 'parent.b3dm',
Copy link
Contributor

Choose a reason for hiding this comment

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

There are a few other samples that need to be updated like these: createTileset, createTilesetEmptyRoot, createTilesetOfTilesets, createTilesetWithExternalResources,

}

extensions[extensionName] = extension;
};
Copy link
Contributor

Choose a reason for hiding this comment

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

Add newline.

*
* @param {Object} options An object with the following properties:
* @param {String} options.directory Directory in which to save the tileset.
* @param {Boolean} [options.batchTableBinary=false] Create a batch table binary for the b3dm tile.
* @param {Boolean} [options.noParents=false] Don't set any instance parents.
* @param {Boolean} [options.multipleParents=false] Set multiple parents to some instances.
* @param {Boolean} [options.legacy=false] Generate the batch table hierarchy as part of the base tileset JSON file, now deprecated.
Copy link
Contributor

Choose a reason for hiding this comment

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

Tweak the description a bit: Generate the batch table hierarchy as part of the base batch table, now deprecated.

* @param {String} extensionName The name of the extension to add.
* @param {*} extension The contents of the extension.
*/
Extensions.addExtension = function(objectJson, extensionName, extension) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Rename objectJson to tilesetJson.

* @param {Object} options An option object with the following properties:
* @param {Boolean} [options.legacy=false] Generate the batch table hierarchy as part of the base tileset JSON file, now deprecated.
*/
function createBatchTableJson(instances, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Since the function is internal the documentation isn't needed.

*
* @param {Object} options An object with the following properties:
* @param {String} options.directory Directory in which to save the tileset.
* @param {Boolean} [options.batchTableBinary=false] Create a batch table binary for the b3dm tile.
* @param {Boolean} [options.noParents=false] Don't set any instance parents.
* @param {Boolean} [options.multipleParents=false] Set multiple parents to some instances.
* @param {Boolean} [options.legacy=false] Generate the batch table hierarchy as part of the base tileset JSON file, now deprecated.
* @param {Matrix4] [options.transform=Matrix4.IDENTITY] The tile transform.
Copy link
Contributor

Choose a reason for hiding this comment

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

Wasn't part of this PR, but I pushed a small commit to fix this doc.

@ggetz
Copy link
Contributor Author

ggetz commented Jul 9, 2018

Thanks @lilleyse, cleaned up!

@@ -506,7 +507,7 @@ function createBatchedWithVertexColors() {
return saveBatchedTileset('BatchedWithVertexColors', tileOptions);
}

function createBatchedWithDataURI() {
function createBatchedWithContentDataUri() {
return saveBatchedTileset('BatchedWithContentUri', undefined, {
Copy link
Contributor

Choose a reason for hiding this comment

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

Make sure to fix the name of the tileset itself to BatchedWithContentDataUri as well.

@@ -872,7 +896,7 @@ function saveInstancedTileset(tilesetName, tileOptions, tilesetOptions) {
tileOptions.eastNorthUp = defaultValue(tileOptions.eastNorthUp, true);

tilesetOptions = defaultValue(tilesetOptions, {});
tilesetOptions.tileName = tileName;
tilesetOptions.contentUri = contentUri;
Copy link
Contributor

Choose a reason for hiding this comment

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

Also change tileName to contentUri in createBatchTableHierarchy.js

    var tilesetJson = createTilesetJsonSingle({
        tileName : tileName,
        geometricError : geometricError,
        box : box,
        transform : transform
    });

@@ -959,10 +991,20 @@ function savePointCloudTileset(tilesetName, tileOptions, tilesetOptions) {

function createHierarchy() {
return createBatchTableHierarchy({
directory : path.join(outputDirectory, 'Hierarchy', 'BatchTableHierarchy'),
directory: path.join(outputDirectory, 'Hierarchy', 'BatchTableHierarchyExtension'),
Copy link
Contributor

@lilleyse lilleyse Jul 9, 2018

Choose a reason for hiding this comment

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

Change the tileset name to BatchTableHierarchy

@ggetz
Copy link
Contributor Author

ggetz commented Jul 9, 2018

@lilleyse Updated!

@lilleyse
Copy link
Contributor

lilleyse commented Jul 9, 2018

Thanks!

@lilleyse lilleyse merged commit 7cd3634 into 2.0-samples-generator Jul 9, 2018
@lilleyse lilleyse deleted the 3d-tiles-1.0-updates branch July 9, 2018 20:02
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