Refactor and Tests #49

lilleyse · 2017-03-14T15:09:34Z

This refactor fixes a few long-standing issues with this project:

Obj objects and groups were getting merged automatically based on material, which should be left as an optional optimization for gltf-pipeline. The gltf converted here is very true to the original obj.
Trouble with loading large models. This will now load obj files that are about 1 GB. Due to node memory limitations, it can't get much higher than that. In the future we can look into out-of-core loading, but the simplicity here is nice.
Some material improvements, especially for textured models without normals.
Added documentation and tests

To-do:

More testing - most models convert well when using --bypassPipeline, so there are some issues to look into for gltf-pipeline

lilleyse · 2017-03-14T16:05:53Z

Fixes #2 - power plant converts with the --bypassPipeline flag
Fixes #17 - no longer combines by material unless optimize is set.
Fixes #29 - speeds should improve since gltf-pipeline optimizations are disabled be default. Also --bypassPipeline is an available option for the fastest conversion.
Fixes #22 - same as above
Partial fix for #32 - using --bypassPipeline no techniques or shaders are generated. A material is still generated and this is required by the gltf 1.0 spec, but that will change in 2.0.
Fixes #34 - lighting should be improved for models without normals
Fixes #37 - however .mtl file had to change Tr 0 to Tr 1
Fixes #47 - normalization bugs have been fixed in CesiumGS/cesium#5032

mramato · 2017-03-14T16:07:28Z

Due to node memory limitations, it can't get much higher than that.

Can you elaborate on this? Because even without out-of-core processing, this seems like an architectural issue somewhere in the code.

lilleyse · 2017-03-14T16:26:46Z

Yes maybe, I'm doing some more tests but the actual numbers do look better than that.

lilleyse · 2017-03-14T18:06:51Z

Maybe obj size isn't the best metric, but I ran through two highly tessellated spheres, one with smooth normals and one with face normals, both 3.2 GB. The first one converts while the second has enough indices that the index array exceeds the maximum allowable typed array length. This could be solved by splitting primitives, something I was trying to avoid - also maybe future work because I want to get these changes in pretty soon.

I actually don't think I have any "real" objs that fail at the moment, @shehzan10 had one that I still need to try out sometime tomorrow.

mramato · 2017-03-14T18:10:45Z

Thanks, that makes sense and sounds like a perfectly fine limitation until we need to handle massive obj files.

pjcozzi · 2017-03-14T18:30:32Z

@likangning93 can you please review this?

lilleyse · 2017-03-14T20:47:59Z

41956dd allows @shehzan10's model to be converted, but --separateTextures should be set. The problem before was there were enough images base64 encoded in the glTF that JSON.stringify fails (due to the same limitations that Node has on string sizes - can only be 192 MB or less)

lilleyse · 2017-03-15T17:06:57Z

Ready now.

likangning93

Looking at obj.js and spec to start off.

likangning93 · 2017-03-16T01:42:53Z

lib/obj.js

+var vertexPattern = /v( +[\d|\.|\+|\-|e|E]+)( +[\d|\.|\+|\-|e|E]+)( +[\d|\.|\+|\-|e|E]+)/;                                                      // v float float float
+var normalPattern = /vn( +[\d|\.|\+|\-|e|E]+)( +[\d|\.|\+|\-|e|E]+)( +[\d|\.|\+|\-|e|E]+)/;                                                     // vn float float float
+var uvPattern = /vt( +[\d|\.|\+|\-|e|E]+)( +[\d|\.|\+|\-|e|E]+)/;                                                                               // vt float float
+var facePattern1 = /f( +-?\d+)\/?( +-?\d+)\/?( +-?\d+)\/?( +-?\d+)?\/?/;                                                                        // f vertex vertex vertex ...


I'm pretty inexperienced with regexes, so I could have used a note that the facePattern regexes return something like [generatedVertexID, position, texCoord, Normal]

oh wait, actually just saw the comments to the side... nevermind.

likangning93 · 2017-03-16T01:45:00Z

lib/obj.js

+    }
+
+    function useMaterial(name) {
+        // Look to see if this material has already been used by a primitive in the mesh


Would it be better to use a per-Mesh dictionary for materials?

Yeah I could have done a material -> primitive dictionary. Since nodes and meshes are also arrays I just wanted to keep it consistent.

likangning93 · 2017-03-16T01:46:26Z

lib/obj.js

+        for (var i = 0; i < primitivesLength; ++i) {
+            primitive = primitives[i]; // Sets the active primitive in case of returning early
+            if (primitive.material === material) {
+                return;


clearer to do
primitive = primitives[i]; // Sets the active primitive in case of returning early
here, does that break anything?

Yes that is clearer.

likangning93 · 2017-03-16T01:51:18Z

lib/obj.js

+        .then(function(materials) {
+            var imagePaths = getImagePaths(materials);
+            return loadImages(imagePaths, objPath)
+                .then(function(images) {


less indentation:

return loadMaterials(...) .then(function(materials) { /// }) .then(function(images) { /// });

The downside here is then materials is not in the closure of the final return (without a helper var).

ah that's right... ok.

likangning93 · 2017-03-16T01:52:10Z

lib/obj.js

-    // Collect all the image files from the materials
-    var images = [];
+function getImagePaths(materials) {
+    var imagePaths = [];


Does indexOf do a linear search? Maybe a dictionary would be better here for an .obj with a huge number of textures.

Yup, will fix that.

likangning93 · 2017-03-16T01:56:15Z

specs/lib/objSpec.js

+
+describe('obj', function() {
+    it('loads obj with positions, normals, and uvs', function(done) {
+        expect(loadObj(objUrl)


Out of curiosity, is there a difference between wrapping in expect(...).toResolve() and something like:

loadObj(..) .then(function(data)) { /// done(); });

In that way if the promise failed it wouldn't reach done(). The test would still fail due to a timeout, but the version in objSpec would fail faster.

likangning93 · 2017-03-16T01:57:29Z

specs/lib/objSpec.js

+            }), done).toResolve();
+    });
+
+    it('loads obj with triangle faces', function(done) {


duplicate, was this supposed to be quads?

Ah, good catch. The other tests all use quads.

likangning93 · 2017-03-16T13:33:21Z

lib/image.js

+        .then(function(data) {
+            var extension = path.extname(imagePath);
+            var uriType = getUriType(extension);
+            var uri = uriType + ';base64,' + data.toString('base64');


Should this include a check on the buffer length for large textures? Otherwise, maybe we should add a troubleshooting section in the README.md with things to try, like writing out separate images if there's a buffer.toString error.

Yeah a common check like this could be in a few places.

likangning93 · 2017-03-16T13:35:34Z

lib/image.js

+                info.format = getFormat(channels);
+
+                if (channels === 4) {
+                    // Need to do a finer grained check over the pixels to see if the image is actually transparent


As discussed offline, a lot of images may be 4-channel PNGs that don't have any transparent pixels. I wonder if it's okay to just make transparency a flag option, since a full pixel level check might be kind of slow.

Good idea for adding an option for this.

With one model I noticed a difference from 200 ms to 6000 ms just by checking this. (It had lots of textures though).

lilleyse · 2017-03-17T20:14:49Z

Updated. The uri handling is better now.

mramato · 2017-04-10T13:42:56Z

specs/lib/convertSpec.js

            }), done).toResolve();
    });
+
+    it('bypassPipeline flag bypasses gltf-pipeline', function(done) {
+        var spy1 = spyOn(convert, '_outputJson');


No need to create these variables, just call spyOn.

mramato · 2017-04-10T13:43:30Z

specs/lib/convertSpec.js

+        expect(convert(objPath, gltfPath, options)
+            .then(function() {
+                expect(spy1.calls.count()).toBe(1);
+                expect(spy2.calls.count()).toBe(0);


This is better written as expect(GltfPipeline.processJSONToDisk).not.toHaveBeenCalled();

mramato · 2017-04-10T13:44:05Z

specs/lib/convertSpec.js

+        };
+        expect(convert(objPath, gltfPath, options)
+            .then(function() {
+                expect(spy1.calls.count()).toBe(1);


This is better written as expect(convert._outputJson).toHaveBeenCalled();

mramato · 2017-04-10T13:45:53Z

specs/lib/convertSpec.js

+            }), done).toResolve();
+    });
+
+    it('uses a custom logger', function(done) {


This test isn't really useful (and is also fragile if we change what we're logging). We should be testing for specific log messages in each instance we expect one to be generated, not just running something and seeing if the logger gets called.

Removed this.

I checked and the all the actually places things are logged are accounted for.

mramato · 2017-04-10T13:46:40Z

specs/lib/gltfSpec.js

+var fsExtra = require('fs-extra');
+var path = require('path');
+var Promise = require('bluebird');
+var clone = require('../../lib/clone.js');


Remove .js from these requires.

mramato · 2017-04-10T13:47:50Z

specs/lib/gltfSpec.js

+    var diffuseTexture;
+    var transparentDiffuseTexture;
+
+    beforeAll(function(done) {


Change this to a beforeEach and get rid of all of the clone usage. You can then delete the clone.js.

mramato · 2017-04-10T13:49:51Z

lib/image.js


-var fsReadFile = Promise.promisify(fs.readFile);
+var defaultValue = Cesium.defaultValue;
+var WebGLConstants = Cesium.WebGLConstants;

 module.exports = loadImage;


Filename and function name don't match. Shouldn't the file be called loadImage?

This is a problem with most of the files in lib. They should all be fixed. We can do that in a quick separate PR after this is merged if you want, but please write up an issue.

mramato · 2017-04-10T13:52:29Z

lib/image.js

+ * @private
+ */
+function loadImage(imagePath, options) {
+    options = defaultValue(options, defaultValue.EMPTY_OBJECT);


This function should check if imagePath is a string and throw a DeveloperError before doing anything else (also add a test for it).

mramato · 2017-04-10T13:54:29Z

specs/lib/mtlSpec.js

+    return path.normalize(path.join(path.dirname(objPath), relativePath));
+}
+
+describe('mtl', function() {


Can be a separate PR, but there are no tests here for how we handle invalid mtl files and we don't do any sort of parameter validation either. Please write up an issue to review all functions for error handling and parameter validation.

mramato · 2017-04-10T13:57:44Z

OK, that's all I have for the first go around. Given the size of this PR, there are definitely things we should do as separate PRs after this is merged, but let's make sure we write issues so nothing drops on the floor.

Thanks @lilleyse!

lilleyse · 2017-04-10T14:04:38Z

Thanks for the thorough review @mramato, I'll get to these soon.

lilleyse · 2017-04-11T21:33:19Z

I got through a lot of the comments here. I may briefly scan everything again but I believe this is ready.

Some things I haven't addressed in this PR but will handle in others:

Better regexes with capture groups for loadMtl and loadObj
Simple validation (for obj, mtl, and images)
- Use Check in all functions
Renaming files
Rethinking loadMtl and loadObj local functions approach.
Better logger messages Refactor and Tests #49 (comment)
Better error handling
Progress tracking
Add ability to find external files: Ability to find external files and textures #54
Get around node string limitations. Build our own buffer ->utf-8 string stream?

Breaking change index.js will just export the convert function. Projects including obj2gltf do not need to call require('obj2gltf').convert(), just require('obj2gltf')().

mramato · 2017-04-12T00:09:57Z

Thanks @lilleyse! This is all good with me. I don't have commit access to this repository so someone else needs to his the merge button. (CC @pjcozzi)

Some of the future PR issues will make great beginner issues.

pjcozzi · 2017-04-12T00:12:24Z

@lilleyse please make sure there are issues for everything in #49 (comment).

pjcozzi · 2017-04-12T00:13:00Z

@lilleyse please comment close all the issues in #49 (comment).

mramato · 2017-04-12T00:13:25Z

@lilleyse when you get a chance, it would be great to publish a new version to npm as well.

lilleyse added 3 commits March 14, 2017 10:43

Refactor and tests

b8b118b

Merge branch 'master' into major-cleanup

3d5e72b

Added .editorconfig

5d0511b

This was referenced Mar 14, 2017

obj2gltf convert error #44

Closed

conversion error #47

Closed

Set diffuse to black when constant lighting is used

d46d58f

lilleyse force-pushed the major-cleanup branch from 2ba97df to 28f1c03 Compare March 14, 2017 17:39

Fix saving .bin

998dcfe

lilleyse force-pushed the major-cleanup branch from 28f1c03 to 998dcfe Compare March 14, 2017 17:49

Better buffer handling

cf27c8b

lilleyse force-pushed the major-cleanup branch from 15354eb to cf27c8b Compare March 14, 2017 18:59

Output separate resources even when --bypassPipeline is set

41956dd

Check for transparent pixels and fix for khr_materials_common

5f6d845

Remove typings

56c4fde

likangning93 reviewed Mar 16, 2017

View reviewed changes

lilleyse added 4 commits March 17, 2017 11:40

Updates

dea55fe

Better handling of encoding base64 uris

0bf726c

Added flag checkTextureAlpha

05b1e3a

Updated LICENSE.md

4583b38

lilleyse force-pushed the major-cleanup branch from adb9fc1 to 4583b38 Compare March 17, 2017 20:13

mramato reviewed Apr 10, 2017

View reviewed changes

Many updates

cc8fee1

lilleyse force-pushed the major-cleanup branch from de92b58 to cc8fee1 Compare April 11, 2017 21:38

pjcozzi merged commit 74a7349 into master Apr 12, 2017

pjcozzi deleted the major-cleanup branch April 12, 2017 00:12

Refactor and Tests #49

Refactor and Tests #49

Conversation

lilleyse commented Mar 14, 2017 • edited Loading

lilleyse commented Mar 14, 2017

mramato commented Mar 14, 2017

lilleyse commented Mar 14, 2017

lilleyse commented Mar 14, 2017 • edited Loading

mramato commented Mar 14, 2017

pjcozzi commented Mar 14, 2017

lilleyse commented Mar 14, 2017

lilleyse commented Mar 15, 2017

likangning93 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

likangning93 Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

likangning93 Mar 16, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lilleyse commented Mar 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mramato Apr 10, 2017 • edited Loading

Choose a reason for hiding this comment

mramato commented Apr 10, 2017

lilleyse commented Apr 10, 2017

lilleyse commented Apr 11, 2017 • edited Loading

mramato commented Apr 12, 2017

pjcozzi commented Apr 12, 2017

pjcozzi commented Apr 12, 2017

mramato commented Apr 12, 2017

lilleyse commented Mar 14, 2017 •

edited

Loading

lilleyse commented Mar 14, 2017 •

edited

Loading

likangning93 Mar 16, 2017 •

edited

Loading

likangning93 Mar 16, 2017 •

edited

Loading

mramato Apr 10, 2017 •

edited

Loading

mramato Apr 10, 2017 •

edited

Loading

lilleyse commented Apr 11, 2017 •

edited

Loading