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

Web3d quantized attributes #56

Merged
merged 40 commits into from
Jun 10, 2016
Merged

Web3d quantized attributes #56

merged 40 commits into from
Jun 10, 2016

Conversation

lasalvavida
Copy link
Contributor

Adds support for quantized attributes.

Related to #308

@lasalvavida
Copy link
Contributor Author

@pjcozzi

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 3, 2016

I'll do a first review here since I have also been looking at CesiumGS/cesium#3891, but @lilleyse can you please do the final review and merge?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 3, 2016

@JoshuaStorm @likangning93 can you make a new branch and include this in your interface?


module.exports = getBinaryGltf;

function getBinaryGltf(gltf, callback) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there unit tests for this?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There are unit tests for writeBinaryGltf which uses this directly.

Copy link
Contributor

Choose a reason for hiding this comment

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

Sounds good, can you double check the coverage and update if needed?

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 3, 2016

Just those comments from me.

This might also need more unit tests.

@lasalvavida
Copy link
Contributor Author

This might also need more unit tests.

Yes, it definitely needs more robust testing

var writeSource = require('./writeSource');
var mergeBuffers = require('./mergeBuffers');
var removePipelineExtras = require('./removePipelineExtras');
var getBinaryGltf = requilre('./getBinaryGltf');
Copy link
Contributor

Choose a reason for hiding this comment

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

typo?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Whoops, yes that's a typo

@lasalvavida
Copy link
Contributor Author

@pjcozzi I think I hit everything here. Fixing removeUnusedVertices will be part of a separate pull request; it's outside the scope of these changes.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 94.332% when pulling deb1c54 on web3d-quantized-attributes into 8a6f4bb on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.5%) to 94.332% when pulling 5d7e99f on web3d-quantized-attributes into 8a6f4bb on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 94.753% when pulling c4d3626 on web3d-quantized-attributes into 30a5565 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.7%) to 94.716% when pulling 84dbc41 on web3d-quantized-attributes into 30a5565 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 94.12% when pulling 74f46e8 on web3d-quantized-attributes into 6fa5301 on master.

@@ -18,7 +18,8 @@ if (process.argv.length < 3 || defined(argv.h) || defined(argv.help)) {
' -i, input=PATH Read unoptimized glTF from the specified file.\n ' +
' -o, output=PATH write optimized glTF to the specified file.\n' +
' -b, write binary glTF file.\n' +
' -s, writes out separate geometry/animation data files, shader files and textures instead of embedding them in the glTF file.\n ';
' -s, writes out separate geometry/animation data files, shader files and textures instead of embedding them in the glTF file.\n' +
' -q, quantize the attributes of this model.\n';
Copy link
Contributor

Choose a reason for hiding this comment

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

Also update README.md please.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 10, 2016

Just those trivial comments. Looking forward to merging this!

@lasalvavida
Copy link
Contributor Author

Updated.

@pjcozzi
Copy link
Contributor

pjcozzi commented Jun 10, 2016

Awesome!

@pjcozzi pjcozzi merged commit 883d01f into master Jun 10, 2016
@pjcozzi pjcozzi deleted the web3d-quantized-attributes branch June 10, 2016 13:27
@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 94.129% when pulling 05d8ad5 on web3d-quantized-attributes into 6fa5301 on master.

@coveralls
Copy link

Coverage Status

Coverage increased (+0.2%) to 94.129% when pulling 05d8ad5 on web3d-quantized-attributes into 6fa5301 on master.

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.

4 participants