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

Optimize b3dm #30

Merged
merged 11 commits into from
Dec 12, 2016
Merged

Optimize b3dm #30

merged 11 commits into from
Dec 12, 2016

Conversation

lasalvavida
Copy link
Contributor

@lasalvavida lasalvavida commented Nov 11, 2016

Adds a tool for optimizing individual b3dm files with gltf-pipeline. The binary glTF is extracted, processed, and then placed back with the header and batch table.

Depends on: CesiumGS/gltf-pipeline#183.

To try this now, manually copy the expose-argument-parsing branch of gltf-pipeline into your node_modules. Currently package.json is set to depend on the next release, so this can be merged when that happens.

* @param {Object} [options] Options specifying custom gltf-pipeline behavior.
* @returns {Buffer} The optimized b3dm buffer.
*/
function optimizeB3dm(buffer, options) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we separate this into explicit functions:

  • b3dm to glTF (and provide this as a command-line tool)
  • Running gltf-pipeline
  • glTF to b3dm (and provide this as a command-line tool)
    • There should be an option to pass in the parameters needed for the header and batch table

### optimizeB3dm

Optimize a b3dm using [gltf-pipeline](https://github.com/AnalyticalGraphicsInc/gltf-pipeline/blob/master/README.md). Since this tool does not
process an entire tileset, it cannot be used with the Pipeline tool.
Copy link
Contributor

Choose a reason for hiding this comment

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

@lilleyse can you add to the roadmap a traversal utility that can be used to apply per-tile stages like this to all tiles in a tileset?

Copy link
Contributor

Choose a reason for hiding this comment

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

Ah yes, I can add that to the roadmap.

@lilleyse
Copy link
Contributor

By the way, @lasalvavida you should have write access now to this repo.


Quantize floating-point attributes and oct-encode normals
```
node ./bin/3d-tiles-tools.js optimizeB3dm -i ./specs/data/batchedWithBatchTable.b3dm -o ./output/optimized.b3dm --options -q -n
Copy link
Contributor

Choose a reason for hiding this comment

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

These two should reference batchedWithBatchTableBinary.b3dm right?

readGlbWriteB3dm(input, force, argv);
readGlbWriteB3dm(input, output, force);
} else if (command === 'optimizeB3dm') {
// optimizeB3dm is not a pipeline tool, so handle it separately.
Copy link
Contributor

Choose a reason for hiding this comment

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

Factor this whole section into its own function.

optionArgs.push('null');
options = GltfPipeline.parseArguments(optionArgs);
}
output = defaultValue(output, input.slice(0, input.length - 4) + 'b3dm');
Copy link
Contributor

Choose a reason for hiding this comment

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

The default path here will be equal to the input path.

module.exports = optimizeB3dm;

/**
* Given an input, possibly gzipped buffer containing a b3dm, optimize it using gltf-pipeline with the provided options
Copy link
Contributor

Choose a reason for hiding this comment

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

I think it would make more sense for the potential unzip to happen the bin file instead.

expect(header.readUInt32LE(8)).toEqual(optimizedBuffer.length);
expect(header.readUInt32LE(12)).toEqual(batchTableJSONByteLength);
expect(header.readUInt32LE(16)).toEqual(batchTableBinaryByteLength);
expect(header.readUInt32LE(20)).toEqual(batchLength);
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you also check here that the gltf content was modified in some way. Even just comparing buffers is ok.

@lilleyse
Copy link
Contributor

lilleyse commented Nov 16, 2016

Seems like a possible gltf-pipeline issue - I'm running the command

node ./bin/3d-tiles-tools.js optimizeB3dm -i ./specs/data/batchedWithBatchTableBinary.b3dm -o ./output/optimized.b3dm

And getting this result:

unoptimized

optimized

@lilleyse
Copy link
Contributor

CesiumGS/gltf-pipeline#183 is merged now. Once the above issue is resolved I'll publish gltf-pipeline to npm.

@lasalvavida
Copy link
Contributor Author

batchedWithBatchTableBinary.b3dm has oct-encoded normals which gltf-pipeline currently cannot process. Blowing out the normals with -r -f yields the expected result.

@lilleyse
Copy link
Contributor

batchedWithBatchTableBinary.b3dm has oct-encoded normals which gltf-pipeline currently cannot process. Blowing out the normals with -r -f yields the expected result.

Ah, that explains it. Let's use this one instead that doesn't have oct-encoded normals
batchedWithBatchTableBinary.b3dm.txt

@lilleyse
Copy link
Contributor

Also given that, I'm going to publish gltf-pipeline to npm now.

@lilleyse
Copy link
Contributor

Ready now.

@lasalvavida
Copy link
Contributor Author

lasalvavida commented Nov 18, 2016

@lilleyse Updated with the requested changes.

@lilleyse
Copy link
Contributor

lilleyse commented Dec 2, 2016

For #34

Perhaps some explicit tools for b3dm->glb and i3dm->glb will be needed in another PR, but all the glb extraction code is here.

optionArgs.push('null');
options = GltfPipeline.parseArguments(optionArgs);
}
outputPath = defaultValue(outputPath, inputPath);
Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of overwriting if no output path is given, it should add a suffix to the filename, like batchedWithBatchTableBinary-optimized.b3dm

batchLength : batchLength
},
batchTable : {
JSON : batchTableJSONBuffer,
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe json instead.

@lilleyse
Copy link
Contributor

lilleyse commented Dec 2, 2016

Sorry about the delay - but everything looks great.

@lasalvavida
Copy link
Contributor Author

@lilleyse, updated.

@pjcozzi
Copy link
Contributor

pjcozzi commented Dec 5, 2016

We should also do this for i3dm, but it is not priority now and a good beginner issue so I submitted #36.

@lasalvavida
Copy link
Contributor Author

@lilleyse Can you look at this when you get a chance?

@lilleyse
Copy link
Contributor

The changes look good!

@lilleyse lilleyse merged commit 8ca7dfc into CesiumGS:master Dec 12, 2016
@pjcozzi pjcozzi mentioned this pull request Jan 12, 2017
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.

3 participants