-
Notifications
You must be signed in to change notification settings - Fork 250
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
updated getBinaryGltf for gltfs with special characters #253
Conversation
var sceneString = JSON.stringify(gltf); | ||
var sceneLength = Buffer.byteLength(sceneString); | ||
sceneLength += 4 - (sceneLength % 4); | ||
var padding = new Array(sceneLength + 1).join(' '); |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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...
lib/getBinaryGltf.js
Outdated
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(' '); |
There was a problem hiding this comment.
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.
lib/getBinaryGltf.js
Outdated
var paddingLength = 4 - (sceneLength % 4); // pad out to 4 bytes as per glb specification | ||
sceneLength += paddingLength; | ||
var paddingBuffer = new Buffer(paddingLength).fill(' '); | ||
var scene = (Buffer.concat([sceneBuffer, paddingBuffer])); |
There was a problem hiding this comment.
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 sceneString = JSON.stringify(gltf); | ||
var sceneLength = Buffer.byteLength(sceneString); | ||
sceneLength += 4 - (sceneLength % 4); | ||
var padding = new Array(sceneLength + 1).join(' '); |
There was a problem hiding this comment.
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...
@lilleyse updated! Looks like a texture compression test is failing to finish in time though. |
lib/getBinaryGltf.js
Outdated
@@ -120,18 +120,18 @@ function getBinaryGltf(gltf, embed, embedImage) { | |||
|
|||
// Create padded binary scene buffer and calculate total length | |||
var sceneString = JSON.stringify(gltf); | |||
var sceneBuffer = new Buffer(sceneString); | |||
var sceneBuffer = Buffer.alloc(Buffer.byteLength(sceneString), sceneString); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Use Buffer.from(string)
instead
@lilleyse yayyy updated and everything passes! |
getBinaryGltf
used to have a problem where gltfs with special characters would cause weird byte offset issues, since the stringified gltf's string length wouldn't be the same as the byte length.