-
Notifications
You must be signed in to change notification settings - Fork 156
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
Make composite tile #4
Conversation
var defined = Cesium.defined; | ||
|
||
var argv = require('yargs') | ||
.usage('Usage: $0 \<tilesets\> [options]') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
"tilesets" -> "tiles", right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup! Thank you
Thanks @lasalvavida! Please add doc to the README.md, and add unit tests. @lilleyse please review and merge when this is ready. |
@lasalvavida @lilleyse also please keep the roadmap, CesiumGS/3d-tiles-tools#9, updated when things are merged. I'll leave this 100% up to you guys. |
Updated |
|
||
## Example | ||
`node ./bin/makeCompositeTile.js batched.b3dm instanced.i3dm points.pnts -o output/composite.cmpt` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work with input wildcards? Not a huge deal, but would be fine if it is trivial to implement.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the output
directory created if it doesn't exist? I am not familiar with all the Node IO routines.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work with input wildcards? Not a huge deal, but would be fine if it is trivial to implement.
It does not, I'll see how hard that is to do.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is the output directory created if it doesn't exist? I am not familiar with all the Node IO routines.
It is because writeTile
uses fs-extra
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this work with input wildcards? Not a huge deal, but would be fine if it is trivial to implement.
It does not, I'll see how hard that is to do.
Actually I just tried it and it does work. Your shell handles the wildcards and passes the resolved arguments.
Looks great at a quick glance! @lilleyse review and merge please. |
Updated |
Thanks @lasalvavida. @lilleyse can you please do the final review and merge? |
Sure, I haven't reviewed this at all yet but I'm going to take some time later to review this and finish up #3 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This utility fits better in the tools project instead. The generator project will be more for constructing sample tilesets. My comments below assume that it's in the tools repo.
Overall things look good here. Just some organizational comments now that #3 is in.
options.gzip = true; | ||
} | ||
return writeTile(outputPath, makeCompositeTile(tiles), options); | ||
}); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Integrate this into the 3d-tiles-tools.js
file.
* @param {Buffer} data A buffer containing the data to test. | ||
* @returns {Boolean} True if the data is gzipped, False if not. | ||
* | ||
* @throws {DeveloperError} Will throw an error if data is undefined. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Typically we don't mention Errors that are thrown because of missing arguments.
throw new DeveloperError('data must be defined.'); | ||
} | ||
return data[0] === 0x1f && data[1] === 0x8b; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another file called isGzipped
exists that checks if a file is gzipped, so maybe that one can be renamed to isGzippedFile
.
I can see several areas where gzipTileset
can improve by using this one as well as using readTile
and writeTile
. I'll make those changes after this PR is in.
/** | ||
* Combines an array of tile buffers into a single composite tile. | ||
* | ||
* @param {Array.<Buffer>} tileBuffers An array of buffers holding tile data. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At least in Cesium, we go with the Buffer[]
version.
} | ||
return buffer; | ||
}); | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This file and writeTile
can probable be condensed. See this comment: #3 (comment)
options = defaultValue(options, {}); | ||
var gzip = defaultValue(options.gzip, false); | ||
if (gzip) { | ||
return zlibGzip(tileData, undefined); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think there was some talk that gzip
is faster as a sync operation because it's not actually doing any file reading. @mramato is this correct?
it('throws DeveloperError if data is undefined', function() { | ||
expect(function() { | ||
isGzipped(undefined); | ||
}).toThrowError(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to toThrowDeveloperError
|
||
it('detects when data is gzipped', function(done) { | ||
var data = new Buffer(40); | ||
expect(isGzipped(data)).toBeFalsy(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
toBe(false)
and toBe(true)
are more precise.
.then(function(tileData) { | ||
var magic = tileData.toString('utf8', 0, 4); | ||
expect(magic).toEqual('i3dm'); | ||
}),done).toResolve(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
, done
spacing
}) | ||
.then(function(tileData) { | ||
expect(isGzipped(tileData)).toBeTruthy(); | ||
}),done).toResolve(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Spacing
Add |
…ols into make-composite-tile
@lilleyse This is ready for a look. |
return fsExtraOutputFile(outputFile, data); | ||
function getDefaultWriteCallback() { | ||
return function(file, data, options) { | ||
return writeTile(file, data, options); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's best to just keep this really simple like it was before where it just gets data and write to a file. Gzipping or anything else should be handled by the calling code. Maybe this makes writeTile
not needed anymore.
.on('data', function (item) { | ||
if (!item.stats.isDirectory()) { | ||
++numberOfFiles; | ||
return getFilesInDirectory(inputDirectory, { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I used to do a similar approach here, but changed it for this comment: #3 (comment). I'm fine with merging the changes in this file in though.
I have a branch that cleans up gzipTileset
and the end result looks similar to this, but walks the directory without buffering the file names up front (through a helper function). And really that too is probably going to get blown away again at some later point.
|
||
module.exports = isTileFile; | ||
|
||
function isTileFile(file) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Mark as @Private
}; | ||
var gzipOptions = { | ||
inputDirectory : tilesetDirectory, | ||
outputDirectory : gzippedDirectory, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea behind custom write callbacks was that you didn't need to supply an output directory to the stage because that information only needs to be known by the callback. I think the old way with using a closure for getDefaultWriteCallback
was more flexible.
Also add |
Brought the |
Adds a tool for generating composite tilesets.