Fix bug where gltfPipeline was not writingSources in all cases #94
Conversation
| readGltf(gltfPath, options, function(gltf) { | ||
| processJSON(gltf, outputPath, options, function() { | ||
| process.nextTick(function() { | ||
| expect(spy.calls.count()).toEqual(4); |
There was a problem hiding this comment.
@JoshuaStorm Can you add a check in these tests making sure that source actually does get updated? Maybe make a change to it while it is in pipeline extras then decode the uri after and compare.
There was a problem hiding this comment.
Actually disregard this, it isn't necessary.
There was a problem hiding this comment.
I think this is the right idea for testing this.
|
This looks good to me. @lilleyse is there a good use case to test this with to make sure it works properly? |
| var basePath = path.dirname(outputPath); | ||
| async.each(['buffers', 'images', 'shaders'], function(name) { | ||
| writeSource(gltf, basePath, name, embed, embedImage, callback); | ||
| }); |
There was a problem hiding this comment.
You need a callback for when the async finishes, and instead you should send in asyncCallback to writeSource. Check out how writeGltf handles this.
Mainly just your other comment, running a gltf through |
|
Hit a bit of a snag, but this good for another look now. |
|
Does this ensure that |
| @@ -24,7 +24,7 @@ function writeSource(gltf, basePath, name, embed, embedImage, callback) { | |||
| var extension = object.extras._pipeline.extension; | |||
|
|
|||
| // Write the source object as a data or file uri depending on the embed flag | |||
| if (embed && (embedImage || name !== 'images')) { | |||
| if (embed && (embedImage || name !== 'images') || basePath === undefined) { | |||
|
|
Updated.
Is there currently an issue with |
No issue with that, it's working fine for me. |
| var initialUri = gltf['buffers'].CesiumTexturedBoxTest.uri; | ||
| processJSON(gltf, options, function () { | ||
| var finalUri = gltf['buffers'].CesiumTexturedBoxTest.uri; | ||
| expect(initialUri).not.toEqual(finalUri); |
There was a problem hiding this comment.
The initial uri will be a filepath to the .bin file, so the comparison will always pass. A quick fix for these 2 tests is to read from gltfEmbeddedPath instead of gltfPath.
|
These are good for review whenever you're ready |
|
Looks good to me! |
Addresses #93
gltfPipelinenow writeSources properly when usingprocessJSONorprocessFile. Had to move some logic around which caused a bit more copy-pasted code instead of just reusing theprocessJSONmethod as we were before, but it was only a few lines so it should be fine.