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.

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

Added gltf-pipeline command line interface #53

Merged
merged 26 commits into from Jun 9, 2016
Merged

Added gltf-pipeline command line interface #53

merged 26 commits into from Jun 9, 2016

Conversation

@JoshuaStorm
Copy link
Contributor

JoshuaStorm commented Jun 1, 2016

Added gltf-pipeline command line interface.
Refactored pipeline to separate file writing and optimizations.
Added gltfPipelineSpec tests.

Pair programming with @likangning93

JoshuaStorm added 2 commits Jun 1, 2016
…parate file writing and optimizations. Added gltfPipelineSpec tests.
@pjcozzi

This comment has been minimized.

Copy link
Member

pjcozzi commented Jun 1, 2016

@pjcozzi

This comment has been minimized.

Copy link
Member

pjcozzi commented Jun 1, 2016

@lilleyse do you want to do a first review before I look at this?

@leerichard42 are you also interested in looking at this?

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Jun 1, 2016

@lilleyse do you want to do a first review before I look at this?

Sure.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 1, 2016

Coverage Status

Coverage increased (+1.7%) to 94.036% when pulling 1028288 on cmdlineInterface into 3db3306 on master.

}
else if (fileExtension === '.gltf') {
gltf = JSON.parse(data);
addPipelineExtras(gltf);

This comment has been minimized.

Copy link
@likangning93

likangning93 Jun 1, 2016

Contributor

We moved addPipelineExtras here from further down to resolve a bug where the gltfPipeline couldn't load glbs with the KHR_binary_glTF extension. It looks like glbs with that extension have an empty data URI, which isn't a problem for the pipeline if the data that should be there is in the assets' extra attribute. parseBinaryGltf was handling this, but then the later call to addPipelineExtras seemed to wipe out the extra attributes, causing an error later down in loadGltfUris.

var defined = Cesium.defined;
var writeGltf = require('../').writeGltf;
var writeBinaryGltf = require('../').writeBinaryGltf;

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Change these two includes to be like the new ones: require('../lib/writeGltf'); (unless there was a good reason to keep it the original way)

'Usage: node ' + path.basename(__filename) + ' [path-to.gltf or path-to.bgltf] [OPTIONS]\n' +
' -o=PATH Write optimized glTF to the specified file.\n';

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Put each of the options of their own line. For organization purposes, -o should go after -i.

'Usage: node ' + path.basename(__filename) + ' [path-to.gltf or path-to.bgltf] [OPTIONS]\n' +
' -o=PATH Write optimized glTF to the specified file.\n';
' -i, input=PATH Read unoptimized glTF from the specified file.\n -b, write binary glTF file.\n' +
' -s, output non-embedded files.\n -o, output=PATH write optimized glTF to the specified file.\n';

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Change the -s description, maybe like this from ModelConverter:

'writes out separate geometry/animation data files, shader files and textures instead of embedding them in the glTF file.'

if (err) {
throw err;
}
var outputPath = argv.o;

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Also worth mirroring -i with something like:

var outputPath = defaultValue(argv._[1], argv.o);

if (exportBinary) {
var outputExtension = path.extname(outputPath);
if (outputExtension !== ".glb") {
outputPath = path.basename(outputPath, outputExtension) + ".glb";

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Usually we use single quotes around strings.

if (exportBinary) {
var outputExtension = path.extname(outputPath);
if (outputExtension !== ".glb") {
outputPath = path.basename(outputPath, outputExtension) + ".glb";
}

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

You can move this check to writeBinaryGltf.

}
writeBinaryGltf(gltf, outputPath, true);

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Note: we'll also want to pass isSeparate in here too soon.

index.js Outdated
@@ -26,5 +26,6 @@ module.exports = {
combineMeshes : require('./lib/combineMeshes'),
combinePrimitives : require('./lib/combinePrimitives'),
removeUnusedVertices : require('./lib/removeUnusedVertices'),
OptimizationStatistics : require('./lib/OptimizationStatistics')
OptimizationStatistics : require('./lib/OptimizationStatistics'),
gltfPipeline : require('./lib/gltfPipeline')

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

I'm starting to wonder if optimizeGltf is a better name here now that it doesn't input and output files.

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Jun 2, 2016

Member

Keep in mind that it will have stages to do more than optimization, e.g., add AO.

var removeUnusedMeshes = require('../').removeUnusedMeshes;
var removeUnusedNodes = require('../').removeUnusedNodes;
var removeUnusedAccessors = require('../').removeUnusedAccessors;
var removeUnused = require('./removeUnused');

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Same comment here making the includes use the same style.

var defaultValue = Cesium.defaultValue;
var DeveloperError = Cesium.DeveloperError;


This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Remove extra line.

@@ -106,6 +106,8 @@ function writeBinaryGltf(gltf, outputPath, createDirectory, callback) {
var scene = new Buffer(sceneString);
var glb = Buffer.concat([header, scene, body], glbLength);

console.log(outputPath);

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Remove this.

var glbPath = './specs/data/boxTexturedUnoptimized/CesiumTexturedBoxTest.glb';

describe('gltfPipeline', function() {
it('checks that gltf is defined when passing in a gltf file', function(done) {

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Make the test name a little more descriptive, like optimizes a gltf file.

inputPath : gltfPath,
separate : false
}, function(gltf) {
expect(defined(gltf)).toEqual(true);

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Use .toBeDefined() instead. Also is there any way to make this more thorough? Maybe at least check that the gltfs don't equal each other.

});
});

it('checks that gltf is defined when passing in a glb file', function(done) {

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

And here: "optimized a binary glTF file"


module.exports = gltfPipeline;

function gltfPipeline(options, callback) {

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Since this will be the starting point for projects that use gltf-pipeline as a library, we should also accept gltf objects directly. Definitely the obj2gltf will use it this way instead of writing a file first, then calling this. Maybe there should be another file that reads gltf files? @pjcozzi what do you think?

This comment has been minimized.

Copy link
@pjcozzi

pjcozzi Jun 2, 2016

Member

Yes, exactly. At the top-level there should be the command-line app, which just calls a function like this, which then calls a function that takes glTF in-memory as input.

Same deal for output - one function would just provide glTF in-memory and another would write to disk.

So the complete Node.js server -> obj2gltf -> gltf-pipeline flow will not write to disk.

Note that the challenge here is to define the in-memory data structure, i.e., it's one thing for the JSON to be loaded, but what about the .bin, .glsl, and images? Perhaps gltf-pipeline should document its simple in-memory structure and provide helpers to read and write it (which it already does).

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 2, 2016

Coverage Status

Coverage increased (+1.6%) to 93.911% when pulling b17fac6 on cmdlineInterface into 3db3306 on master.

@JoshuaStorm

This comment has been minimized.

Copy link
Contributor Author

JoshuaStorm commented Jun 2, 2016

Updated, addressed @lilleyse's comment, aside from last (awaiting @pjcozzi's input).

Currently working to add more gltfPipelineSpec tests.

@@ -1,114 +1,53 @@
#!/usr/bin/env node
'use strict';
var fs = require('fs');
var argv = require('yargs').argv;

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Add yargs to package.json, and remove minimist.

var defined = Cesium.defined;
var writeGltf = require('../').writeGltf;
var writeBinaryGltf = require('../').writeBinaryGltf;
var gltfPipeline = require('../lib/gltfPipeline');

if (!defined(argv._[0]) || defined(argv.h) || defined(argv.help)) {

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Now that we support -i for input files, argv._[0] may be undefined but shouldn't go the the help screen. As an alternative you can check for process.argv.length < 3.

var defined = Cesium.defined;
var writeGltf = require('./writeGltf');
var writeBinaryGltf = require('./writeBinaryGltf');
var gltfPipeline = require('../lib/gltfPipeline');

if (!defined(argv._[0]) || defined(argv.h) || defined(argv.help)) {

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 2, 2016

Contributor

Adding this comment again because the previous comment is hidden:

Now that we support -i for input files, argv._[0] may be undefined but shouldn't go the the help screen. As an alternative you can check for process.argv.length < 3.

var gltf;
var gltfCopy;
var options = { 'resourcePath' : path.dirname(gltfPath) };
fs.readFile(gltfPath, function(err, data) {

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

Why not use readGltf here?

This comment has been minimized.

Copy link
@JoshuaStorm

JoshuaStorm Jun 8, 2016

Author Contributor

readGltf includes the loadGltfUris step, and this test is checking that processJSON is properly handling a JSON that has yet to have that step applied

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

Ah right.

expect(gltf).not.toBe(gltfCopy);
});
});
});

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

You could probably drop these two tests (related to my other comment).

var gltfCopy;
var options = {};
readGltf(gltfPath, function(gltf) {
gltfCopy = gltf;

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

Not actually doing a clone here.

var gltfCopy;
var options = {};
readGltf(glbPath, function(gltf) {
gltfCopy = gltf;

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

And here.

expect(gltf).not.toBe(gltfCopy);
});
});
});

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

All of these tests should have done callbacks.

expect(spy).toHaveBeenCalled();
done();
});
});

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

These two tests can be merged into one. Don't forget to check that the expected output path.

This comment has been minimized.

Copy link
@JoshuaStorm

JoshuaStorm Jun 8, 2016

Author Contributor

I had an issue with trying to check for an expected output path. Because multiple files are written, it is difficult to test exactly what the output path would be (or at least I am not sure how I would go about it).

For example:
I tried expect(spy).toHaveBeenCalledWith(outputPath, jasmine.any(String), jasmine.any(Function)); but the result ends up being

[ outputPath/output.gltf, <a stringified JSON blob>, <a function>], [ outputPath/output0VS.glsl, <a stringified JSON>, <a function>], ...

Overall, I am just unsure how I can write a Jasmine expect for a spy that seems to see multiple outputs with different expected files. Maybe something with regular expressions?

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

Oh ok, it may help if you set the isEmbedded options to true. Or isEmbedded should default to true instead of false..

This comment has been minimized.

Copy link
@JoshuaStorm

JoshuaStorm Jun 8, 2016

Author Contributor

That is a way easier solution, thanks!

This comment has been minimized.

Copy link
@JoshuaStorm

JoshuaStorm Jun 8, 2016

Author Contributor

I am having issues with merging these two tests into one where it is throwing: Error: EISDIR: illegal operation on a directory, open 'output/output' at Error (native)

I initially thought this was because it was trying to create a new directory in a path that already exists, but even after adding an option to not create a new directory I am still getting the same error.

Any thoughts? Should I just leave it as separate tests?

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

I'm tried calling just the second one and it passes for me.

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

The new changes look good, just check this last thing and it will be ready.

});
});
});
});

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

Add newline.

readGltf(glbPath, function(gltf) {
expect(gltf).toBeDefined();
});
});

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

You still need the done callback for these two tests.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 8, 2016

Coverage Status

Coverage increased (+0.2%) to 94.008% when pulling 3cdf83d on cmdlineInterface into 8a6f4bb on master.

@JoshuaStorm

This comment has been minimized.

Copy link
Contributor Author

JoshuaStorm commented Jun 8, 2016

Updated.

I believe I addressed all the comments, aside from the one about combining the two file writing tests in gltfPipelineSpec (see comment above).

gltfCopy = clone(gltf);
processFile(glbPath, options, function (gltf) {
expect(gltf).toBeDefined();
expect(gltf).not.toBe(gltfCopy);

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

I missed these before, but it should be .not.toEqual for all these tests. (.toBe checks that they are the same object, .toEqual checks values, like === vs ==).

This comment has been minimized.

Copy link
@JoshuaStorm

JoshuaStorm Jun 8, 2016

Author Contributor

That explains why they weren't failing when I was forgetting to clone them. Thanks for the explanation

processFileToDisk(gltfPath, outputPath, options, function() {
expect(spy).toHaveBeenCalled();
expect(spy).toHaveBeenCalledWith('./output/', jasmine.any(String), jasmine.any(Function));

This comment has been minimized.

Copy link
@lilleyse

lilleyse Jun 8, 2016

Contributor

Cleaner with something like expect(spy.calls.first().args[0]).toEqual('./output/');

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Jun 8, 2016

Just those few comments, this is getting close.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 8, 2016

Coverage Status

Coverage increased (+0.2%) to 94.008% when pulling 2607af7 on cmdlineInterface into 8a6f4bb on master.

@JoshuaStorm

This comment has been minimized.

Copy link
Contributor Author

JoshuaStorm commented Jun 8, 2016

Updated

JoshuaStorm added 2 commits Jun 9, 2016
@JoshuaStorm

This comment has been minimized.

Copy link
Contributor Author

JoshuaStorm commented Jun 9, 2016

Addressed those last two comments about the gltfPipelineSpec and combinePrimitivesSpec. Am I missing anything?

@lilleyse

This comment has been minimized.

Copy link
Contributor

lilleyse commented Jun 9, 2016

I think that's everything, I'm going to merge and if we missed anything we can make another PR.

@lilleyse lilleyse merged commit e5359fa into master Jun 9, 2016
3 checks passed
3 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
coverage/coveralls Coverage increased (+0.2%) to 94.008%
Details
@lilleyse lilleyse deleted the cmdlineInterface branch Jun 9, 2016
@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 9, 2016

Coverage Status

Coverage increased (+0.2%) to 94.008% when pulling 61600c0 on cmdlineInterface into 8a6f4bb on master.

@coveralls

This comment has been minimized.

Copy link

coveralls commented Jun 9, 2016

Coverage Status

Coverage decreased (-12.6%) to 81.206% when pulling 64cd2ea on cmdlineInterface into 8a6f4bb on master.

@pjcozzi

This comment has been minimized.

Copy link
Member

pjcozzi commented Jun 10, 2016

This is a good start for the command-line interface; is the ability to configure custom pipelines via command-line, JSON input, and a function taking a JSON object coming as a separate PR?

@JoshuaStorm

This comment has been minimized.

Copy link
Contributor Author

JoshuaStorm commented Jun 10, 2016

Unless I am misunderstanding, we currently have functions to take a JSON as input in the gltfPipeline.js file --processJSON and processJSONToDisk.

Also, could you elaborate a bit more on what you mean by "ability to configure custom pipelines via command-line"

Thanks!

@pjcozzi

This comment has been minimized.

Copy link
Member

pjcozzi commented Jun 10, 2016

From #1:

  • Command-line and JSON input, e.g., like pdal (see here and here)
@lasalvavida

This comment has been minimized.

Copy link
Contributor

lasalvavida commented Jun 10, 2016

@JoshuaStorm I could be wrong, but I think the idea is that the pipeline should be able to output to stdout, read from stdin, and specify specific operations to perform. Then you could chain commands together using pipes. Something like:
./bin/gltf-pipeline.js --stdout myModel.gltf --stage addDefaults | ./bin/gltf-pipeline.js --stdin --stage quantizeAttributes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
7 participants
You can’t perform that action at this time.