Upgrade tileset stage #86
Conversation
|
@likangning93 can you please review this? |
tools/lib/extractB3dm.js
Outdated
| if (byteOffset % 4 === 0) { | ||
| return buffer; | ||
| } | ||
| return Buffer.from(buffer); |
There was a problem hiding this comment.
From what I can tell, this is just copying buffer in memory if byteOffset isn't aligned to 4 bytes. Is this so the b3dm buffer can be modified somewhere without damaging the glb? It might be worth documenting.
| return new Promise(function (resolve, reject) { | ||
| var readStream = fsExtra.createReadStream(file, readStreamOptions); | ||
| readStream.on('error', reject); | ||
| readStream.on('data', function(chunk) { |
There was a problem hiding this comment.
This is just checking the first chunk right? Should this be readStream.once('data' //...? Also, I'm still not 100% with streams, but should this get manually closed after the first chunk is read?
tools/lib/optimizeGlb.js
Outdated
| for (var semantic in attributes) { | ||
| if (attributes.hasOwnProperty(semantic)) { | ||
| if (semantic === 'BATCHID') { |
There was a problem hiding this comment.
Is it a bad idea to directly look up attributes.BATCHID? If not, it might be a little cleaner.
| if (parameters.hasOwnProperty(parameterId)) { | ||
| var parameter = parameters[parameterId]; | ||
| if (parameter.semantic === 'BATCHID') { |
There was a problem hiding this comment.
I think this case is different.
| afterEach(function (done) { | ||
| Promise.all([ | ||
| fsExtra.remove(upgradedDirectory), | ||
| fsExtra.remove(gzippedDirectory) |
There was a problem hiding this comment.
since upgradeTileset takes a write callback, should we try to avoid filesystem changing operations in this spec?
There was a problem hiding this comment.
Yeah this needs to be fixed throughout the codebase. I'll write an issue.
|
Addressed your comments @likangning93. I also added support for upgrading cmpt tiles in 2b787f4 and in the process took code from #4. |
Review after #84
This upgrades older tilesets to the latest 3D Tiles version. I've been testing this on the NYC CityGML tileset.
asset.versionto 1.0ADDandREPLACE_BATCHIDsemantic instead ofBATCHIDin the glTFTodo: