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

Fix chunk headers in GLB file writer #107

Merged
merged 3 commits into from Mar 1, 2019

Conversation

Projects
None yet
3 participants
@MGraefe
Copy link
Contributor

MGraefe commented Mar 1, 2019

Fix chunk headers in GLB file writer
- Chunk type is supposed to be a 4 byte magic number: ASCII "JSON" or "BIN\0". See https://github.com/KhronosGroup/glTF/blob/master/specification/2.0/README.md#chunks
- Added proper padding after chunks
@CLAassistant

This comment has been minimized.

Copy link

CLAassistant commented Mar 1, 2019

CLA assistant check
All committers have signed the CLA.

@ibgreen

ibgreen approved these changes Mar 1, 2019

Copy link
Contributor

ibgreen left a comment

@MGraefe Nice catches! Took a quick look and it seems like this has already been fixed in the parser side of things, so makes total sense to make the same changes here.

Ideally we should add some test assertions, but this is good to land.

copyArrayBuffer(glbArrayBuffer, binChunk, binChunkOffset + GLB_CHUNK_HEADER_SIZE);
for (let i = 0; i < binChunkLengthPadded - binChunk.byteLength; ++i) {
// json chunk is padded with spaces (ASCII 0x20)

This comment has been minimized.

Copy link
@ibgreen

ibgreen Mar 1, 2019

Contributor

Nit: I suppose this comment should be changed

This comment has been minimized.

Copy link
@MGraefe

MGraefe Mar 1, 2019

Author Contributor

Classic copy&paste error from the "eslint fix" commit...

Fixed incorrect comment
Copy & Paste error in last commit

@ibgreen ibgreen merged commit 9cbe034 into uber-web:master Mar 1, 2019

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
license/cla Contributor License Agreement is signed.
Details
@ibgreen

This comment has been minimized.

Copy link
Contributor

ibgreen commented Mar 1, 2019

@MGraefe Thanks. Do you need this to be published quickly, or can this wait a few days until next time we publish a path?

@MGraefe

This comment has been minimized.

Copy link
Contributor Author

MGraefe commented Mar 1, 2019

No need to rush things, I don't need the writer right now.

@MGraefe MGraefe deleted the MGraefe:patch-1 branch Mar 1, 2019

@ibgreen

This comment has been minimized.

Copy link
Contributor

ibgreen commented Mar 7, 2019

v0.8.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.