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

Fix bug where gltfPipeline was not writingSources in all cases #94

Merged
merged 6 commits into from
Jun 22, 2016
Merged

Fix bug where gltfPipeline was not writingSources in all cases #94

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
1417c88
Fix write sources bug. Addresses #93
Jun 17, 2016
a9d6f99
Missing semicolon
Jun 17, 2016
241d2f3
Always embed when using and
Jun 21, 2016
e288636
jsHint failure fix
Jun 21, 2016
75fa5d0
Addressed comments
Jun 21, 2016
d6970eb
Use gltfEmbeddedPath
Jun 21, 2016
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
38 changes: 32 additions & 6 deletions lib/gltfPipeline.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
'use strict';
var async = require('async');
var addDefaults = require('./addDefaults');
var addPipelineExtras = require('./addPipelineExtras');
var removeUnused = require('./removeUnused');
Expand All @@ -16,6 +17,7 @@ var loadGltfUris = require('./loadGltfUris');
var quantizeAttributes = require('./quantizeAttributes');
var cacheOptimizeIndices = require('./cacheOptimizeIndices');
var encodeImages = require('./encodeImages');
var writeSource = require('./writeSource');
var Cesium = require('cesium');
var defaultValue = Cesium.defaultValue;

Expand All @@ -26,13 +28,27 @@ module.exports = {
processFileToDisk : processFileToDisk
};

function writeSources(gltf, callback) {
var embed = true;
var embedImage = true;
async.each(['buffers', 'images', 'shaders'], function(name, asyncCallback) {
writeSource(gltf, undefined, name, embed, embedImage, asyncCallback);
}, function() {
callback();
});
Copy link
Contributor

Choose a reason for hiding this comment

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

You need a callback for when the async finishes, and instead you should send in asyncCallback to writeSource. Check out how writeGltf handles this.

}

function processJSON(gltf, options, callback) {
addPipelineExtras(gltf);
loadGltfUris(gltf, options, function(err, gltf) {
if (err) {
throw err;
}
processJSONWithExtras(gltf, options, callback);
processJSONWithExtras(gltf, options, function(gltf) {
writeSources(gltf, function() {
callback(gltf);
});
});
});
}

Expand Down Expand Up @@ -66,7 +82,9 @@ function processJSONWithExtras(gltfWithExtras, options, callback) {
function processFile(inputPath, options, callback) {
readGltf(inputPath, options, function(gltf) {
processJSONWithExtras(gltf, options, function(gltf) {
callback(gltf);
writeSources(gltf, function() {
callback(gltf);
});
});
});
}
Expand All @@ -84,14 +102,22 @@ function writeFile(gltf, outputPath, options, callback) {
}

function processJSONToDisk(gltf, outputPath, options, callback) {
processJSON(gltf, options, function(gltf) {
writeFile(gltf, outputPath, options, callback);
addPipelineExtras(gltf);
loadGltfUris(gltf, options, function(err, gltf) {
if (err) {
throw err;
}
processJSONWithExtras(gltf, options, function(gltf) {
writeFile(gltf, outputPath, options, callback);
});
Copy link
Contributor

Choose a reason for hiding this comment

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

Why the change here and in processFileToDisk?

Copy link
Contributor

Choose a reason for hiding this comment

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

This can change back to its original version now, right?

Copy link
Contributor

Choose a reason for hiding this comment

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

Never mind, these functions can't use processJson anymore due to the recent changes.

});
}

function processFileToDisk(inputPath, outputPath, options, callback) {
processFile(inputPath, options, function(gltf) {
writeFile(gltf, outputPath, options, callback);
readGltf(inputPath, options, function(gltf) {
processJSONWithExtras(gltf, options, function(gltf) {
writeFile(gltf, outputPath, options, callback);
});
});
}

Expand Down
2 changes: 1 addition & 1 deletion lib/writeGltf.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ var removePipelineExtras = require('./removePipelineExtras');
module.exports = writeGltf;

function writeGltf(gltf, outputPath, embed, embedImage, createDirectory, callback) {
//Create the output directory if specified
// Create the output directory if specified
if (createDirectory) {
outputPath = path.join(path.dirname(outputPath), 'output', path.basename(outputPath));
mkdirp.sync(path.dirname(outputPath));
Expand Down
4 changes: 2 additions & 2 deletions lib/writeSource.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -14,7 +14,7 @@ module.exports = writeSource;
function writeSource(gltf, basePath, name, embed, embedImage, callback) {
var objects = gltf[name];

//Iterate through each object and write out its uri
// Iterate through each object and write out its uri
if (defined(objects)) {
var ids = Object.keys(objects);
async.each(ids, function(id, asyncCallback) {
Expand All @@ -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') || !defined(basePath)) {
var sourceDataUri = new dataUri();
if (name === 'shaders') {
sourceDataUri.format('.txt', source);
Expand Down
30 changes: 27 additions & 3 deletions specs/lib/gltfPipelineSpec.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -107,19 +107,43 @@ describe('gltfPipeline', function() {
createDirectory : false
};
readGltf(gltfPath, options, function(gltf) {
var options = { basePath : path.dirname(gltfPath) };
processJSONToDisk(gltf, outputPath, options, function() {
expect(path.normalize(spy.calls.first().args[0])).toEqual(path.normalize('./output/'));
expect(path.normalize(spy.calls.first().args[0])).toEqual(path.normalize('output/output'));
done();
});
});
});

it('will write sources from JSON', function(done) {
Copy link
Contributor

Choose a reason for hiding this comment

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

This test and the other may not be needed based on the comments above.

var options = {};
readGltf(gltfEmbeddedPath, options, function (gltf) {
var initialUri = gltf['buffers'].CesiumTexturedBoxTest.uri;
processJSON(gltf, options, function () {
var finalUri = gltf['buffers'].CesiumTexturedBoxTest.uri;
expect(initialUri).not.toEqual(finalUri);
Copy link
Contributor

Choose a reason for hiding this comment

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

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.

done();
Copy link
Contributor

Choose a reason for hiding this comment

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

For these two tests it's more accurate to compare the data uris since we are checking that it wrote the data back to those.

});
});
});

it('will write sources from file', function(done) {
var options = {};
readGltf(gltfEmbeddedPath, options, function (gltf) {
var initialUri = gltf['buffers'].CesiumTexturedBoxTest.uri;
processFile(gltfEmbeddedPath, options, function (gltfFinal) {
var finalUri = gltfFinal['buffers'].CesiumTexturedBoxTest.uri;
expect(initialUri).not.toEqual(finalUri);
done();
});
});
});

it('will add image processing extras if this is a pipeline with image processing', function(done) {
var options = {
imageProcess: true
imageProcess : true
};
readGltf(gltfEmbeddedPath, options, function(gltf) {
var gltfCopy = clone(gltf);
processJSON(gltf, options, function (gltf) {
expect(gltf).toBeDefined();
var images = gltf.images;
Expand Down