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.

Sign up for GitHub

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

updated getBinaryGltf for gltfs with special characters #253

Merged
merged 5 commits into from
Apr 6, 2017
Merged

updated getBinaryGltf for gltfs with special characters #253

Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
44bd46d
updated getBinaryGltf for gltfs with special characters
likangning93 Apr 5, 2017
0376a1b
removed debug code
likangning93 Apr 6, 2017
1409771
PR comments
likangning93 Apr 6, 2017
4eaf497
removed focus tests
likangning93 Apr 6, 2017
fd4ae6a
use Buffer.from
likangning93 Apr 6, 2017
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
16 changes: 9 additions & 7 deletions lib/getBinaryGltf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -118,12 +118,15 @@ function getBinaryGltf(gltf, embed, embedImage) {
// Remove extras objects before writing
removePipelineExtras(gltf);

// Create padded binary scene string and calculate total length
// Create padded binary scene buffer and calculate total length
var sceneString = JSON.stringify(gltf);
var sceneLength = Buffer.byteLength(sceneString);
sceneLength += 4 - (sceneLength % 4);
var padding = new Array(sceneLength + 1).join(' ');
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not 100% sure if there was a reason for the padding to be so big before, and then get sliced down to size. Any insights?

Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure either...

sceneString = (sceneString + padding).substring(0, sceneLength);
var sceneBuffer = new Buffer(sceneString);
var sceneLength = sceneBuffer.length;
var paddingLength = 4 - (sceneLength % 4); // pad out to 4 bytes as per glb specification
sceneLength += paddingLength;
var paddingBuffer = new Buffer(paddingLength).fill(' ');
Copy link
Contributor

Choose a reason for hiding this comment

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

Use Buffer.alloc(paddingLength, ' ') instead. We should avoid new Buffer throughout but that can be a sweeping change handled later.

var scene = (Buffer.concat([sceneBuffer, paddingBuffer]));
Copy link
Contributor

Choose a reason for hiding this comment

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

No need for the outer parentheses.


var bodyOffset = 20 + sceneLength;
var glbLength = bodyOffset + body.length;

Expand All @@ -135,8 +138,7 @@ function getBinaryGltf(gltf, embed, embedImage) {
header.writeUInt32LE(sceneLength, 12);
header.writeUInt32LE(0, 16);

// Create scene buffer and overall buffer
var scene = new Buffer(sceneString);
// Create overall buffer
var glb = Buffer.concat([header, scene, body], glbLength);

return {
Expand Down