GLTFExporter: Export images as binary in GLB. #12877
Conversation
Voting for Promises. |
|
I'd vote for promises too. I'd also add an option to define the behaviour for the images if the user wants to export then as data URIs or bufferView. |
donmccurdy
force-pushed
the
feat-gltfexporter-image-bufferviews
branch
from
95b249b
to
14c146d
Compare
donmccurdy
force-pushed
the
feat-gltfexporter-image-bufferviews
branch
from
14c146d
to
9f67353
Compare
|
Rebased and switched to Promises, thanks! I don't think we want an option for data URIs instead of bufferView; they use 25–30% more space in a GLB with no benefit I'm aware of. |
donmccurdy
changed the title
| * @param {Blob} blob | ||
| * @return {Promise<Integer>} | ||
| */ | ||
| function processBufferViewImage ( blob ) { |
There was a problem hiding this comment.
You don't need an space after function name
|
@donmccurdy regading the data uri option I was thinking on engines that doesn't have implemented bufferview for images, but I'm not aware of the current state of most of the engines so you may know more about it. |
|
It is part of the core spec, engines should consider it a requirement and I'm not aware of any that don't support this. Inclined to avoid the extra API surface unless the issue is brought up. |
|
Yep, so in that case 👎 to the extra params |
|
Is this ready to go then? |
|
|
||
| } | ||
|
|
||
| return new Promise( function (resolve) { |
There was a problem hiding this comment.
Need spaces before and after resolve.
|
|
||
| var reader = new window.FileReader(); | ||
| reader.readAsArrayBuffer( blob ); | ||
| reader.addEventListener('loadend', function () { |
There was a problem hiding this comment.
Need a space before 'loadend'.
There was a problem hiding this comment.
And how about using .onloadend for the consistency with other parts in the exporter?
|
@donmccurdy looking great! 👏 |
|
Sweet! Thanks! |
Work in progress, I'd like to (1) get feedback and (2) test this a bit more.The main change here is that when
options.binary === trueandoptions.embedImages === true, the images will be written to bufferViews instead of Data URIs, taking up less space. For the example scene, it's a ~25% file size difference.I wasn't sure how to manage the async parts, since the processing so far hasn't needed as much of that. Didn't want to add callbacks to everything, and wasn't sure it was worth it to add Promises just for this change, but I could imagine the
next()counters I'm using here might be confusing to read... Any preferences?/cc @fernandojsg