GltfImporter: Support morph target attributes

[mrxz]

This PR adds parsing of the morph targets of a mesh primitive to the GltfImporter. The morph target attributes are added to the attributeOrder collection so that they follow the same code path as other attributes. The stable sort is adjusted to first sort on morph target id followed by attribute name, ensuring only duplicates within a morph target/base attributes are filtered and not across morph targets.

[mosra]

Thank you!

[mosra]

The Trade::MeshAttributeData constructor asserts that the morph target isn't a weights, joints or object ID attribute, so the importer has to check that and fail the import in that case.

Best place to do that would probably be in the if cascade below that translates the string names to MeshAttribute.

[mrxz]

I've updated it to behave similarly to the unsupported vertex format case. So it can still get imported as a custom attribute. Not entirely happy about the resulting error message, as it indicates the format being unsupported for said attribute, which isn't incorrect per se, but misleading in the sense that it could imply there exists a valid format (instead of the entire attribute not being supported in morph targets).

[mosra]

I ended up making this an import error with a clear message instead. I don't expect these to be hit much, if at all, in practice, as they're disallowed by glTF anyway. Originally the unsupported vertex formats were also an import error, I just made those non-fatal after discovering there actually are such assets in the wild, in particular non-normalized 16-bit vertex colors. So if something similar happens for morph targets, I can reconsider, but until then I'm keeping it simple and just failing.

The case where an unsupported format is in an otherwise supported morph target attribute (such as those non-normalized colors) is still handled as custom attribute, that stays.

[mrxz]

Worth mentioning that it's quite common for morph target attributes to use sparse accessors. These aren't supported by the GltfImporter, but this has the peculiar side effect that .gltf models with morph targets and sparse accessor do load without this change (albeit without morph targets), but with this change will skip those meshes. Not sure if such a (pseudo-)regression is acceptable or not.

Now it would be nice if sparse accessor could be supported. But seeing the mention of memory-mapped file support, I take it that sparse accessor support doesn't really fit that picture?

[mosra]

Now it would be nice if sparse accessor could be supported.

Yup, I expected this to become the "next step" :)

But seeing the mention of memory-mapped file support, I take it that sparse accessor support doesn't really fit that picture?

For starters I'd just "unsparse" the buffer first -- I assume that's how it would usually be uploaded to the GPU as well, right? That of course would mean the file contents can't be zero-copy imported from a memory-mapped anymore, but better than not having any support for sparse buffers at all. Eventually there could be dedicated builtin support in MeshData for such buffers (I imagine some sort of a per-attribute index buffer or something?) but so far I don't have any clear idea on how that would be done, or if it would be a good UX at all.

But before implementing support for sparse buffers I'd like to deal with the problem in #124, as those two have quite a lot of overlap. I have a semi-complete idea of how this would be done in a way that preserves both memory mappability and doesn't cause insane memory use, just need to find some time to actually implement it.

These aren't supported by the GltfImporter, but this has the peculiar side effect that .gltf models with morph targets and sparse accessor do load without this change (albeit without morph targets), but with this change will skip those meshes. Not sure if such a (pseudo-)regression is acceptable or not.

Let's ignore this regression for now, I'd say. If it becomes an issue in practice (let's say, models that loaded fine for some important project no longer load), then it'll be a question of whether it's easier to add some fallback to ignore morph targets, or just go and implement sparse buffer support.

[codecov]

Codecov Report

Patch coverage: 98.82% and project coverage change: -0.01% ⚠️

Comparison is base (90a0da6) 97.05% compared to head (f5673e9) 97.05%.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #141      +/-   ##
==========================================
- Coverage   97.05%   97.05%   -0.01%     
==========================================
  Files         143      143              
  Lines       16318    16348      +30     
==========================================
+ Hits        15838    15867      +29     
- Misses        480      481       +1     

Files Changed	Coverage Δ
src/MagnumPlugins/GltfImporter/GltfImporter.cpp	99.43% <98.82%> (-0.04%)	⬇️

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[mosra]

Merged, thank you!

In addition to what I mentioned re disallowed attributes in the comment above, I expanded some tests in 4cfe3b1 to test also that single uncovered line. The rest went in with no changes, thanks for top-class work :)

Is there anything else I should expected related to morph targets, like hooking up the animation support? I'll definitely let you know once I have #124 tackled so it's possible to implement sparse buffers as well.