Refactor and Tests

[lilleyse]

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:

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

[lilleyse]

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 https://github.com/AnalyticalGraphicsInc/cesium/pull/5032

[mramato]

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]

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

[lilleyse]

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]

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

[pjcozzi]

@likangning93 can you please review this?

[lilleyse]

https://github.com/AnalyticalGraphicsInc/obj2gltf/pull/49/commits/41956dd70d1ab98d180a7944fedaf82592fc97a7 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]

Ready now.

[lilleyse]

Updated. The uri handling is better now.

[likangning93]

On a ~470 MB obj with textures, I'm getting:

[likangning93]

Same thing when running with -s

[likangning93]

Ah. It put the bin and images in a the output directory. Herm.

[lilleyse]

Ok that should be fixed now.

[lilleyse]

Updated.

[likangning93]

New changes look good to me, I think that covers everything I saw.

[lilleyse]

@mramato should reading images/mtl files from absolute paths be disallowed for security reasons? Are there other security issues to handle?

[mramato]

@mramato should reading images/mtl files from absolute paths be disallowed for security reasons? Are there other security issues to handle?

It depends on the use case:

1. For client-side desktop usage, I see no reason we shouldn't read any file that the user running it has access to.
2. For server-side usage, having a way to disallow absolute paths (or more specifically, not allow the model to index outside of the directory of it's source files) is ideal. The goal here would be to avoid leaking data outside of the source input (model and textures) to a client.

[mramato]

Was testing out this branch and ran the following command.

./bin/obj2gltf.js -i ../obj2gltf/specs/data/box-textured/box-textured.obj -o /C/Data/Foo/test.glb

However, instead of test.glb, it output /C/Data/Foo/box-textured.glb.

Is the documentation wrong, or the is there a bug?

[mramato]

It must be a bug, because ommiting the filename just crashes obj2gltf (which is actually a second bug)

./bin/obj2gltf.js -i ../obj2gltf/specs/data/box-textured/box-textured.obj -o /C/Data/Foo/

[lilleyse]

Is the documentation wrong, or the is there a bug?

Yeah that's a bug. Fixed now.

ommiting the filename just crashes obj2gltf (which is actually a second bug)

The error handling for this is better now, but it still throws an error as it expects the output to be a file path and not a directory.

[mramato]

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]

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

[lilleyse]

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 https://github.com/AnalyticalGraphicsInc/obj2gltf/pull/49#discussion_r110651528
• Better error handling
• Progress tracking
• Add ability to find external files: https://github.com/AnalyticalGraphicsInc/obj2gltf/issues/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]

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]

@lilleyse please make sure there are issues for everything in https://github.com/AnalyticalGraphicsInc/obj2gltf/pull/49#issuecomment-293406766.

[pjcozzi]

@lilleyse please comment close all the issues in https://github.com/AnalyticalGraphicsInc/obj2gltf/pull/49#issuecomment-286470171.

[mramato]

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