CgltfImporter

[pezcode]

👋 Here's the initial version of CgltfImporter to import glTF 2.0 files using cgltf.

As far as features are concerned, it's a drop-in replacement for TinyGltfImporter, but there are a few minor changes in behaviour (see below). Large parts of its code and tests were re-used and adapted for cgltf, making some parts more trivial than expected.

There aren't any major TODOs, only a few things I'd appreciate some feedback on, all marked with @todo in the code (some are kept from TinyGltfImporter code). So unless CI breaks I won't be changing anything until the first review round.

Noticable improvements

Compared to TinyGltfImporter, using VS 2019, Release build:

• Compile time: 5s -> 3s
• Binary size: 430kb -> 220kb

Subtle differences that may impact users switching from TinyGltfImporter

• Buffers are loaded on demand and references/data are cached. This means the callback policy is now LoadPermanent and returned views on resources have to stay valid until the importer is closed.
• WEIGHTS and JOINTS are imported as a single custom attribute with multiple sets, mimicking behaviour of AssimpImporter and preparing for Skinned Mesh attributes magnum#441
• Percent-encoded URIs are now decoded, TinyGltfImporter ignored percent encoding.

New features

• Support for KHR_texture_basisu and MSFT_texture_dds (in addition to the already existing GOOGLE_texture_basis)
• asset.version is checked against the supported glTF version (2.0)
• a few extra warnings about invalid mesh attribute names

[mosra]

Oooo, wonderful! 🎉 Looking forward to diving into the code.

Wait, the tinygltf plugin compiles in just 5 seconds for you? Here I always have to wait about half a minute for a compile + link, longer than all other plugins combined... The binary size reduction is awesome.

KHR_texture_basisu

Wow, also? 🤩 I suppose there's some bits missing in KtxImporter to delegate to BasisImporter, it doesn't work just yet, right?

[pezcode]

I suppose there's some bits missing in KtxImporter to delegate to BasisImporter, it doesn't work just yet, right?

Yeah, that needs some extra wiring in KtxImporter.

As for CI failures: the Emscripten one I'm not sure about, seems like a recent issue with sourceforge? The AddressSanitizer error is interesting, I'll have to see if that's a genuine bug but it has to be fixed upstream either way. It doesn't like overlapping strcpy ranges in coded added in jkuhlmann/cgltf#165

[mosra]

I went through hopefully everything, what's left is looking at the coverage and checking if there are some untested areas.

• The Emscripten CI failure is worked around in a7a8b43, so if you rebase then it should go through.
• The ASan failure about overlapping strcpy() is valid I guess (would happen with e.g. \\ab already, copying ab over \a), and same as with memcpy() it's not allowed. Not sure if there's any standard strmove() equivalent to memmove() tho, I suppose you'd need to get the string size first and then use the mem*() APIs instead?

[pezcode]

I addressed everything except moving validation to the plugin side, I'll work on that tomorrow.

I'll send PRs for the two cgltf changes related to string decoding, but those might take a few days to get merged. Should I push the changes locally for now so tests pass and you can see test coverage?

[mosra]

Should I push the changes locally for now so tests pass and you can see test coverage?

Yes please, good idea.

The Emscripten test seems to be failing due to some files not being specified in the corrade_add_test() macro, I think.

[pezcode]

With cgltf_validate gone, we can undo some of the changes made to TinyGltfImporter test files again. Before I send a PR for that, what else would you want backported?

Possible candidates:

• asset.version checks and/or tests (so we can at least minimize the number of test files in CgltfImporter/Test)
• new texture extensions (only KHR_texture_basisu)
• invalid mesh attribute order checks and/or tests
• skinning attribute checks and/or tests
• skinning attribute naming (one custom attribute for all sets of weights/joints) nope
• URI decoding (no change, mentioned in docs)
• requiredExtensions check (no change, mentioned in docs)
• invalid hierarchy detection (cycles, non-root scene nodes, multiple parents)
• multiple buffers in mesh error + test

[codecov]

Codecov Report

Merging #107 (5881d19) into master (48bb558) will increase coverage by 22.81%.
The diff coverage is 100.00%.

@@             Coverage Diff             @@
##           master     #107       +/-   ##
===========================================
+ Coverage   72.42%   95.23%   +22.81%     
===========================================
  Files         162      115       -47     
  Lines       37502    10354    -27148     
===========================================
- Hits        27162     9861    -17301     
+ Misses      10340      493     -9847     

Impacted Files	Coverage Δ
src/MagnumPlugins/CgltfImporter/CgltfImporter.cpp	97.29% <ø> (ø)
src/MagnumPlugins/CgltfImporter/CgltfImporter.h	100.00% <100.00%> (ø)
...MagnumPlugins/CgltfImporter/importStaticPlugin.cpp	100.00% <100.00%> (ø)
...gins/Faad2AudioImporter/Test/Faad2ImporterTest.cpp
src/external/dr/dr_flac.h
...s/PrimitiveImporter/Test/PrimitiveImporterTest.cpp
src/Magnum/OpenDdl/Test/TypeTest.cpp
...slangShaderConverter/Test/GlslangConverterTest.cpp
...gins/DrWavAudioImporter/Test/DrWavImporterTest.cpp
...rImageConverter/Test/OpenExrImageConverterTest.cpp
... and 43 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 48bb558...5881d19. Read the comment docs.

[mosra]

we can undo some of the changes made to TinyGltfImporter test files again

That sounds great, if it's not too much boring work :)

what else would you want backported?

Now that you made CgltfImporter basically a drop-in replacement, I'm thinking it's not worth to invest too much effort into tinygltf anymore, as it doesn't seem to be better / smaller / faster / more flexible than cgltf in any aspect. I know of a few existing projects that depend on it and access tinygltf internals through importerState(), which means I can't deprecate it right away, but I can put it in a maintenance mode at least -- only bug/security/robustness fixes from now on, no new features.

asset.version checks and/or tests (so we can at least minimize the number of test files in CgltfImporter/Test)

Yes, if it reduces the amount of tests it's good :)

new texture extensions

The Khronos Basis extension only, as that one is long overdue. No need for the others, CgltfImporter will get all the new goodies instead.

invalid mesh attribute order checks and/or tests
skinning attribute checks and/or tests

Yes, this counts as a security/robustness fix.

skinning attribute naming (one custom attribute for all sets of weights/joints)

I'd keep this as-is, some projects depend on the current way already, and once the attributes become builtin it would change again. No need to make a breaking change twice I'd say. (For CgltfImporter the way you implemented is fine, as it's closer to the builtin way.)

URI decoding

No need, it'd however be good to mention it among the limitations so people are aware.

Similarly for the missing requiredExtensions check -- I'd not backport that one either, but mention it among limitations. I'm a bit on the fence about it -- soon all found material/node extensions will be exposed through the MaterialData/SceneData, which kinda means the importer does support the unknown extensions, on the other hand those will be just extra layers in the material / extra fields in the scene that the user might not even bother checking. Maybe for CgltfImporter it might make sense to add an off-by-default option to continue parsing even if there are unknown requiredExtensions.

Apart from the above, the scene cycle detection is probably worth backporting as well.

[mosra]

Looked at the coverage, pretty good actually :) The few uncovered lines might point to some error checking redundancy.

Apart from that there's some uncovered lines in two switch cases that deal with types and formats, but I don't think that's worth writing explicit tests for, the code in question is trivial enough to not need that.

[mosra]

Also uncovered, probably the same as the two other cases before. Hmm, doesn't checkAccessor() handle this already? In that case maybe accessorView() and checkAccessor() could be conflated into a single function that returns a null view on failure, instead of having one that returns a bool and then another where you have to check the return value again.

[pezcode]

checkAccessor makes sure accessor ranges are correct, ie. the buffer (view) is large enough, etc. accessorView on the other hand, loads the buffer (if not already loaded). Main reason they're separate is to delay buffer loading to the latest possible point. This is currently used in doMesh, where for each attribute the accessors are checked and their ranges are merged, and at the very end the single buffer is loaded.

[pezcode]

I'm almost done with the coverage improvements, but the checkAccessor() ones depend on updated test files in #109. I'll wait until that's merged, and then adapt the tests and remove superfluous test files. done!

Finish line todo list:

• adapt to TinyGltfImporter test changes
• print glTF magic numbers instead of cgltf enum values (CgltfImporter #107 (comment))
• test checks for accessorView failing (all invocations, if possible)
• avoid strlen for string decoding, depends on Return new length from string decoding functions jkuhlmann/cgltf#167. Don't know if/when this gets merged, we can probably ignore this for now and add it at a later date, not exactly critical 🤠

[pezcode]

This should be ready for (the last? 🙉) review. All outstanding issues should be fixed (except for that cgltf change, but that can be done in a later PR).

[pezcode]

Ah, this is missing test coverage for invalid input to gltf*TypeName(). I'll add a test for that via doAnimation, that should also cover the two missing types in the switch. Going to push that with whatever changes are left after your review.

[mosra]

Okay, last things:

• This one comment about extension warnings
• The coverage fixes you mentioned above
• I looked at the commit log and given that some changes were rather sweeping (adapting to TinyGltf test changes, ditching cgltf_validate()...) I'm thinking it could be all squashed into two commits:
  • one with adding the cgltf dependency (and mentioning jkuhlmann/cgltf@9ba04b0 in the commit message now that the last change was merged as well)
  • and the other with the rest :)

Thank you!

[pezcode]

I'm thinking it could be all squashed into two commits

Just double-checking so I don't do anything stupid here: you want me to squash the entire PR down to two commits locally and then force-push? One commit with cgltf.h and all the other files in another commit?

[mosra]

Yep :)

[pezcode]

I squashed everything down, but I kept a branch in my fork with the full commit history just in case 👀

[mosra]

Ahh, playing with the new plugin feels great, everything exceeds expectations. In my case it's the following (native builds use the scenedata magnum branch, which may add some extra overhead due to all extra compat interfaces):

• Compile time GCC Debug: 19 -> 6 seconds
• Compile time GCC Release: 14 -> 6.5 seconds
• Plugin binary size GCC Debug: 10 MB -> 2.5 MB
• Plugin binary size GCC Release: 696 kB -> 277 kB
• Plugin binary size Emscripten Release: 4.2 MB -> 1.2 MB
• Size of the Emscripten magnum-viewer example (ports branch, without the bundled scene): 914 kB -> 727 kB
  • For comparison the barebones triangle example that uses almost nothing from Magnum is 313 kB

I blame the leftover std::strings and std::vectors in the plugin manager interfaces that get always inlined, without those the binary sizes and compile times could go even less.

Looked at import time using magnum-sceneconverter --info --profile, CgltfImporter never gets slower than TinyGltfImporter but I suppose most of the time was spent in reading the file and copying the data a few extra times, and I only tested on a smallish 7.5 MB Buggy.glb from the glTF samples repo. Once I take care of the extra copies with mosra/magnum#240 (which CgltfImporter finally unblocks, yay!), it has the potential of outperforming everything else.

For memory consumption (valgrind --tool=callgrind) of the same tool, I see CgltfImporter peak memory usage being 16 MB down from 17.2 MB with TinyGltfImporter. Given that there's no other change in Magnum internals that would avoid having the file present in memory twice (which I really have to fix now), I find this quite great -- JSON parser overhead is less than half of the original. With a change that would reuse the memory allocated in openFile() instead of making a copy in doOpenData(), the peak memory consumption would be another 7.5 MB less, so 17.2 -> 8.5 MB 🎉