glTF 2.0

[lasalvavida]

Large-scale refactor for glTF 2.0 support in the C++ model converter. Note: 2.0 is still changing and this PR will likely change a bit more before it is finalized.

Sample models seem to be in good shape coming through this so far; code-base is much more modular and maintainable.

Adds support for binary glTF export as well.

@pjcozzi, @lexaknyazev

[lasalvavida]

What is the plan for glTF 1.0 in COLLADA2GLTF? Is it no longer supported?

Yes, this will not output 1.0. I think keeping the old converter as a 1.0 branch is a good solution.

[lilleyse]

From the outset the code looks really nice and organized.

Before getting too deep into the code I tried out some different cases and had some notes:

README notes:

• -t --separateImage - may be a good opportunity to call it --separateTextures
• collada2gltlf -f [file] [options] is outdated
• collada2gltf doesn't get saved in a bin folder anymore.

Other notes:

• Can the -i and -o flags be optional, allowing for commands like collada2gltf model.dae model.gltf. Your choice on this.
• An output file should not be required. collada2gltf -i model.dae should work.
• Add a help command line flag and print it if no input is given.
• After putting textures in an assets folder, the commandCOLLADA2GLTF.exe -i Cesium_Air.dae -o Cesium_Air.gltf -b --basePath assets exports incorrectly while COLLADA2GLTF.exe -i Cesium_Air.dae -o Cesium_Air.gltf -b --basePath assets/ is successful. I can understand why this is happening, is it worth fixing?
• COLLADA2GLTF.exe -i Cesium_Air.dae -o Cesium_Air/CesiumAir.gltf fails because the Cesium_Air folder doesn't exist. The tool should create the folder.
• COLLADA2GLTF.exe -i Cesium_Air.dae -o CesiumAir.gltf -b -s fails but only when the -b flag is there. -t however is fine, so I bet it has to do with saving the .bin while being a binary gltf.
• When converting a dae that references images from a subfolder like data/0_Cesium_Air.png, it exports a broken gltf - but data\0_Cesium_Air.png works (I'm on Windows). Both formats should be handled properly.
• Similarly when it succeeds the uri in the glTF is data\\0_Cesium_Air.png. May be worth standardizing on using foward slashes for the converted glTF.
• It would be nicer if the separate resources (.bin .glsl) had the name of the model appended to them, like Ceisum_Air_0.bin
• I assume this is intended by the spec, but when the material has a diffuse texture it looks like diffuse : [0] It's a little odd to see it this way, but that's not super relevant here.
• materialsCommon - the node references a light 0, but it doesn't exist - I think the extensions.KHR_materials_common is formatted improperly and needs a light array. Side note: why not set ambient color in the material and do away with the lights? We may have had this conversation before.
• The transparent tag of the dae is not taken into account for regular export or materialsCommon. Models exported from blender will have this tag

<transparent>
    <color>1 1 1 0.4195011</color>
</transparent>

• Should type:"arraybuffer" be removed? Is this still part of gltf 2.0?
• The z-up to y-up node should be named so its obvious
• In gltf 2.0 is binary gltf still an extension? Maybe the discussion on this just hasn't converged 100% yet
• Possible other models to test with https://github.com/assimp/assimp/tree/master/test/models/Collada
• I'm running into a normalization error when loading the converted Cesium_Air model in the 2.0 branch in Cesium - have you seen this as well? Do you have any other recommendations for viewing the exported models?
• It would be nice to preserve the names of COLLADA nodes in the gltf nodes
• Does it check whether textures are translucent and add the appropriate states?
• The error handling for non-existent textures should be better. The conversion succeeds but outputs a very simple gltf.
• When the dae specifies Y-UP there is an empty root node that has an identity matrix, just a single node would make more sense.

[lasalvavida]

-t --separateImage - may be a good opportunity to call it --separateTextures

Agreed

collada2gltlf -f [file] [options] is outdated

Good catch

collada2gltf doesn't get saved in a bin folder anymore.

What do you mean by this?

Can the -i and -o flags be optional, allowing for commands like collada2gltf model.dae model.gltf. Your choice on this.

The way it is now, this is very doable. I've been shopping around for a modern c++ argument parsing library with low overhead, and I'll keep this in mind while I'm looking. I will make this change to the existing code though.

An output file should not be required. collada2gltf -i model.dae should work.

Agreed

Add a help command line flag and print it if no input is given.

Agreed

After putting textures in an assets folder, the commandCOLLADA2GLTF.exe -i Cesium_Air.dae -o Cesium_Air.gltf -b --basePath assets exports incorrectly while COLLADA2GLTF.exe -i Cesium_Air.dae -o Cesium_Air.gltf -b --basePath assets/ is successful. I can understand why this is happening, is it worth fixing?

When converting a dae that references images from a subfolder like data/0_Cesium_Air.png, it exports a broken gltf - but data\0_Cesium_Air.png works (I'm on Windows). Both formats should be handled properly.

Similarly when it succeeds the uri in the glTF is data\0_Cesium_Air.png. May be worth standardizing on using foward slashes for the converted glTF.

I'm using OpenCOLLADA for most URI resolution so I'll take a look and make sure it is doing what it is supposed to be doing. If not, it would be pretty straightforward to add a few small path utilities to make the output more uniform.

It would be nicer if the separate resources (.bin .glsl) had the name of the model appended to them, like Ceisum_Air_0.bin

Agreed

I assume this is intended by the spec, but when the material has a diffuse texture it looks like diffuse : [0]

Yes that is part of the spec as far as I'm aware since we don't reference by id anymore.

materialsCommon - the node references a light 0, but it doesn't exist - I think the extensions.KHR_materials_common is formatted improperly and needs a light array. Side note: why not set ambient color in the material and do away with the lights? We may have had this conversation before.

When is this happening? This was written to match the 1.0 KHR_materials_common in the current sample models. As far as I know 2.0 KHR_materials_common hasn't converged yet.

The transparent tag of the dae is not taken into account for regular export or materialsCommon. Models exported from blender will have this tag

Good catch

Should type:"arraybuffer" be removed? Is this still part of gltf 2.0?

Yes it should be removed.

The z-up to y-up node should be named so its obvious

Agreed.

In gltf 2.0 is binary gltf still an extension? Maybe the discussion on this just hasn't converged 100% yet

I was working off of what was currently merged into the 2.0 spec branch. Looking now, it looks like this change hasn't been made yet. This will definitely change, especially given that objects don't have ids anymore so the buffer can't be found by name

I'm running into a normalization error when loading the converted Cesium_Air model in the 2.0 branch in Cesium - have you seen this as well? Do you have any other recommendations for viewing the exported models?

I haven't done Cesium Air. Do we want to add it to the sample models?

It would be nice to preserve the names of COLLADA nodes in the gltf nodes

We do actually, but not the ids. Maybe we should set the name to the id if a collada name isn't present.

Does it check whether textures are translucent and add the appropriate states?

Right now it does not. I'm not totally clear on when this, or the invert transparency code is necessary, so currently it is omitted. If I have a test case, I will add support.

When the dae specifies Y-UP there is an empty root node that has an identity matrix, just a single node would make more sense.

What model is this happening in? That is not expected behavior

[pjcozzi]

@lasalvavida great progress! @lilleyse great review.

In gltf 2.0 is binary gltf still an extension? Maybe the discussion on this just hasn't converged 100% yet

I was working off of what was currently merged into the 2.0 spec branch. Looking now, it looks like this change hasn't been made yet. This will definitely change, especially given that objects don't have ids anymore so the buffer can't be found by name

This is getting close, just small changes from the extension: KhronosGroup/glTF#828

[lasalvavida]

Updated. I addressed most of your comments.

Binary glTF 2.0 spec has been merged, and this has been updated to match.

I'm using std::filesystem::path for path operations; output directories will be created, and referenced files will always be created in the same directory as the glTF, so separator characters don't need to be considered.

I've added argparse, a lightweight argument parser, so this now has a nice clean help and usage message.

You can pass input and output files implicitly without the flags.

If a collada name is not present for nodes, materials, or meshes, the collada original id will be set as the name.

Fixed the binary and separate crash.

If an image cannot be resolved, the name of the image file is placed as the uri, and conversion continues.

Still TODO KHR_materials_common and PBR spec changes (i.e. GLSL will probably be an extension) once they converge completely.

[lasalvavida]

Oh this also includes an appveyor and travis config from my fork for CI. The secure keys will have to be updated to run on here if we want to enable that.

[pjcozzi]

Thanks @lasalvavida. Can you please merge in master?

What do you think about replacing this with a new PR that merges into a new 2.0 branch that we can merge soon? Then all changes to keep up with the 2.0 spec can be PRs into that branch, which we'll merge to master when ready.

[lasalvavida]

I am closing this PR as I've created a 2.0 branch that we will do final review on when it is time to replace the 1.0 converter.

[lilleyse]

The build for the 2.0 branch is not working right now.

CMake Error at dependencies/OpenCOLLADA/modules/COLLADASaxFrameworkLoader/GeneratedSaxParser/LibXML/CMakeLists.txt:70 (add_library):
  Cannot find source file:

    ../../../../OpenCOLLADA/Externals/LibXML/c14n.c

  Tried extensions .c .C .c++ .cc .cpp .cxx .m .M .mm .h .hh .h++ .hm .hpp
  .hxx .in .txx

[lasalvavida]

Thanks @lilleyse, it looks like I lost the OpenCOLLADA submodule in the migration. I've added it back now, and the build is working for me; let me know if you still have any issues.

[lilleyse]

All good now, thanks for the quick fix 👍