feat: Added submesh import support (#30)

[Kibsgaard]

I've got a first version working and would love some feedback when you have time.

As it requires changes to both DracoUnity and glTFast, I've set up a development repo here:
https://github.com/Kibsgaard/glTFast-Development
It auto-reloads the models on recompile if Unity is configured to Recompile and Continue Playing.

Related glTFast PR:
Unity-Technologies/com.unity.cloud.gltfast#33

The main changes:

1. Setup entire MeshData buffers in DracoNative before first submesh decode. Also setup BoneWeightsData buffers
2. Deleted DracoMeshLoader.cs as it is marked obsolete and updating it didn't seem productive
3. Created DracoDecoder.DecodeMesh that accepts submeshIndex, vertexIntervals[], indicesIntervals[], and bounds[] which are read from accessors in DracoMeshGenerator(glTFast). This is done so we can create the entire mesh buffer up front. The bounds are still generated if they are missing.
4. The main (private) DecodeMesh parameters are null by default and using it the old way should still work, so the original public DecodeMesh methods still work, however, I'm unsure on how to test them all.

I forgot about SubMeshAssignment until I was almost done and I'm unsure what it is for / how I would use it with DracoMeshGenerator? Do you have some sample models that use SubMeshAssignments or are they not relevant to draco compressed models?

I hope this is the right place to make a pull request, else please let me know 😄

Thanks!

[Kibsgaard]

Just added a fix for the combined bounds. They are now correctly loaded for all my test models.

We plan on using this in a release soon. @atteneder, do you have time to review it for any glaring issues?

[atteneder]

Hi @Kibsgaard ,

I'll look at it the next week latest. Sorry for the delay, I've been out of office the last weeks.

[Kibsgaard]

I've found a model that can't be loaded on this branch. One of the meshes is corrupted and the Unity Editor even crashes after exiting play mode. I haven't yet had time to investigate what the issue is and what makes this model different.
Suspension_draco.zip

[atteneder]

Thanks for letting me know. I started looking at it and found some minor things I'll fix on my own. I'll continue next week.

[atteneder]

Thanks for kicking off the effort. I was able to figure out why it crashes in certain scenarios and suggested a change that would fix that.

Let me know what you think and how you intend to go forward.

[atteneder]

I'd prefer to use plain NativeArray<byte>.ReadOnly here and in all API calls going forward.

Using NativeSlice<byte> instead of just NativeArray<byte> is a mistake I made not really knowing what NativeSlice's purpose is and that NativeArray.GetSubArray solves the actual problem much better.

This is not something you should care about, but I'd prefer to fix this first before adding more public methods that have odd type choices.

[atteneder]

This is problematic as it shifts the responsibility of determining the vertex buffer layouts, verifying that all submeshes are compatible and determining the index data type to the first DracoNative instances, which is unaware of the other submeshes.

I can see that this was the most pragmatic way of getting a result quickly, but it already causes problems.

For example, the glTF that crashes the Editor (Suspension_draco.glb) calculates inconsistent vertex stream layouts resulting in chaos.

Solution:
There needs to be a layer between DracoDecoder and DracoNative that handles creating all DracoNative instances, calculating vertex stream layout, submesh descriptors and setting the MeshData properties. This way the responsibilities are clear.

[Kibsgaard]

Thanks for the review 😄

I've finally found some time to investigate the issue.

Regarding stream layout mismatch, the first submesh now uses the combined vertex count when determining the stream layout and the format of the index buffer (16 vs 32 bit). This fixes the issue, but it does still assume each submesh has the same attributes.

I guess the gltf spec does not explicitly require all primitives/submeshes in a mesh to have the same vertex attributes, however, I can't image a scenario where they would not have the same attributes. Do you have an example of this? I can't make one with Blender, as it adds attributes from all meshes when combining meshes into one. I could hand-edit a gltf, but that also seems a bit contrived 😄

What should the desired behavior be if submeshes have different attributes?
I think it should just log an error and return false/null as this is not a supported mesh format in Unity.
It could also split the misaligned submehes into separate meshes, but I think that is a lot of work for something that I can't see being a real use-case.

Would this be a GLTFAST_SAFE only validation? Maybe when the flag is not defined, the first submesh creates the attribute-mapping and the other submeshes use it blindly (similar to the current solution, but skipping the redundant mapping-creation on each submesh)?

[atteneder]

What should the desired behavior be if submeshes have different attributes? I think it should just log an error and return false/null as this is not a supported mesh format in Unity. It could also split the misaligned submehes into separate meshes, but I think that is a lot of work for something that I can't see being a real use-case.

In a first version I'd only expect a proper error (and no crash). I agree, most if not all valid glTFs will not mix up vertex buffer structures. In your particular use case the issue was clearly on our side though, hence the recommendation to determine buffer structures and index type overall and not per sub-mesh.

The next, more advanced thing would be filling in missing attributes (e.g. one sub-mesh has colors, another does not), but that's not expected in a first version.

Would this be a GLTFAST_SAFE only validation? Maybe when the flag is not defined, the first submesh creates the attribute-mapping and the other submeshes use it blindly (similar to the current solution, but skipping the redundant mapping-creation on each submesh)?

I'd expect it to be safe by default (so not behind that scripting define).

Thanks

[NumaNumaNuma]

Hello :) I've been following this PR 😍, are you guys still working on it?

[atteneder]

It's not forgotten. I'm currently working on the package, but have to fix Android support first. Best case but low probability is I'll finish it this week. After that I'm on vacation, so it will probably take another month or two.

Thanks for your patience.

[NumaNumaNuma]

Awesome, thanks!

[atteneder]

Update: I've started to refactor Draco for Unity (e.g. splitting up the monolithic DracoNative) so that it's fit for submeshes and properly calculates the various buffer layouts from multiple draco meshes.

I think I might be able to clean it up soon so that it'll ship in October.

Thanks for the PR. Even though we ended up not merging it, it helped us learning about the problems and shaping the solution.