UfbxImporter plugin

[bqqbarbhg]

Integration for bqqbarbhg/ufbx as an importer plugin. There is existing FBX support in the Assimp importer but ufbx should have better coverage of advanced FBX features and some that are esoteric in the file format, but quite common in practice, like animation tangents. The library is also embeddable and quite robust against bad/malicious inputs in a way that it should be safe to be bundled to end users and not crash/hang in the face of any inputs.

I'm creating a very early draft PR so I can already ask for some feedback for the direction to take this in.

One major decision is whether to embed or to have ufbx as a dependency. Embedding ufbx.h and ufbx.c is roughly a megabyte uncompressed, which is about 100kB more than the stb/ directory. I'm also planning on creating a CMake "distribution" repository for ufbx that could be used as a dependency so that's an alternative option. I'd suggest going for embedding as that's the spirit of the library and makes life easier for users.

It seems to be common here to include a checklist so I'll draft one below, many of these are quite advanced and might not be worth implementing, at least in the first PR. Feel free to comment on which ones you might find useful:

• Scene graph
  • Nodes
  • Lights
  • Materials
  • Cameras
  • Instancing
  • LODs (rare)
• Geometry
  • Meshes
  • Triangulation
  • Multiple UV sets
  • Multiple color sets
  • Polygon groups (rare)
  • Subdivision surfaces (rare)
  • NURBS surfaces (rare)
  • NURBS curves (rare)
  • Line curves (rare)
• Materials
  • FBX legacy
  • PBR
  • Embedded textures
• Deformers
  • Skinning
  • Blend shapes
  • Geometry caches (rare)
• Animation
  • Node transformations
  • Multiple takes
  • Light properties (rare)
  • Material properties (rare)
  • Arbitrary properties (rare)
  • Additive/blend layers (rare)
  • Constraints (rare)

[codecov]

Codecov Report

Base: 96.81% // Head: 97.17% // Increases project coverage by +0.35% 🎉

Coverage data is based on head (0a7a60d) compared to base (b595419).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #133      +/-   ##
==========================================
+ Coverage   96.81%   97.17%   +0.35%     
==========================================
  Files         130      134       +4     
  Lines       13953    16190    +2237     
==========================================
+ Hits        13509    15732    +2223     
- Misses        444      458      +14     

Impacted Files	Coverage Δ
src/MagnumPlugins/UfbxImporter/UfbxImporter.cpp	100.00% <100.00%> (ø)
src/MagnumPlugins/UfbxImporter/UfbxImporter.h	100.00% <100.00%> (ø)
src/MagnumPlugins/UfbxImporter/UfbxMaterials.h	100.00% <100.00%> (ø)
.../MagnumPlugins/UfbxImporter/importStaticPlugin.cpp	100.00% <100.00%> (ø)
src/MagnumPlugins/GltfImporter/GltfImporter.h	100.00% <0.00%> (ø)
...rc/MagnumPlugins/OpenGexImporter/OpenGexImporter.h	100.00% <0.00%> (ø)
.../StbResizeImageConverter/StbResizeImageConverter.h	100.00% <0.00%> (ø)
...numPlugins/PngImageConverter/PngImageConverter.cpp	98.14% <0.00%> (+0.03%)	⬆️
src/MagnumPlugins/GltfImporter/GltfImporter.cpp	99.50% <0.00%> (+0.09%)	⬆️
src/MagnumPlugins/PngImporter/PngImporter.cpp	95.37% <0.00%> (+0.37%)	⬆️
... and 1 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

[mosra]

Hello, welcome and thanks for the early draft!

One major decision is whether to embed or to have ufbx as a dependency.

My general rule is "embedding json.hpp was a mistake" :) That library (together with tinygltf) is around a megabyte as well, but since it's an auto-generated header, every upgrade consisted of several thousand lines of changes, inflating repo size significantly.

I don't expect your library to have the same kind of churn so embedding should be fine.

Regarding features -- thanks for the checklist! -- to have the initial PR in a manageable scope, I'd pick just the basic node hierarchy, meshes and materials so "usual" models can be imported and rendered. "Usual" as in what's commonly present in FBX files, if it's PBR materials then PBR, if it's legacy materials then legacy. And then, once this is merged, followup PRs with things like animations -- @Squareys will know better what they need and what not, on my side I'm fine with reasonable Assimp feature parity (minus bugs, haha). In other words, so there's no reason to use Assimp for FBX anymore.

Re CIs, you may want to enable MAGNUM_WITH_UFBXIMPORTER in *.sh and *.bat files inside package/ci to have the code actually built and tested there. Not sure what's up with that CircleCI error, somehow it's always a problem for new contributions :/ You could try doing what they suggest on that page, it also might be that you'll need to log into there with your GitHub account if you haven't done that yet. OTOH don't tell CircleCI to start building your fork of this repo, that will break in a different way (sigh).

Assuming you don't intend to become a regular Magnum contributor, no need to spend too much time with code style. I'll appreciate if you try to match how other code looks like, but I'm fine with doing a formatting pass in the end myself anyway. Same for Git workflow, this initial PR will most probably get squashed to a single commit (which is why I want to have its scope small), the individual commits will matter only the followup PRs, so no need to spend too much time on a clean history yet either.

If you have any questions about how various APIs are meant to be used, feel free to ask here or reach out to me on Gitter (simply log in with your GitHub account).

Looking forward to this!

[bqqbarbhg]

Thanks @mosra for the welcome and thorough comment!

Understandable with the json.hpp situation, ufbx has got a bit annoyingly large over time which makes the embedding decision less obvious than I'd like. Just a lot of special cases and hacks that accumulate (and some are relatively large, like 3ds Max serializing shader graphs as FBX textures), but that's the price you have to pay for making something that "just works".

But like you mentioned ufbx is not amalgamated from separate source files, but actually developed in the single one, so updates should not result in large diffs. So good to know you agree with the embedding angle as well.

I'll check if I can get the CI working, and no worries about the CircleCI issue, 3rd party services always have their own problems.

I'll try to keep the code style I'll try to adapt still, feels wrong to contribute into a repo with my own preferences (especially when you have clear documentation about all that!). I guess I should prefer using Corrade over STL where possible as well?

[mosra]

CircleCI seems to be working now, yay! There's about 25 individual jobs and to reduce credit usage I have some dependencies set up between them, so if some fail like now, it won't run the rest. So just be aware that there's a lot :) Also, if it fails to build on certain obscure platforms (such as MSVC 2015, UWP or iOS) and the fix isn't obvious / trivial, we can decide to just ignore given platform until someone actually starts needing it.

Just a lot of special cases and hacks that accumulate

Worst case, if it would eventually become too large to be bundled, I can always drop the embedded sources in favor of an opt-in CMake subproject, "find the files and compile them" kind of a CMake subproject, looking for an external installation and such. I'm already doing that with e.g. Basis Universal or ImGui (where neither is really available in package managers) and while it makes building from sources slightly harder, it's still acceptable.

I guess I should prefer using Corrade over STL where possible as well?

In a lot of cases you'll be kinda forced to, because e.g. the MeshData class needs an Array, but there's still many cases where I don't have a STL replacement ready, such as various hashmaps. STL container mapping table is here but -- again -- I don't expect you to learn a whole new library just to write this one plugin, so just do it as you think is best and if there's a different, more Magnum-like way, I'll point that out in the review.

It might also be useful for you to check how the same thing is done in other plugins -- GltfImporter is the most complex one, StanfordImporter and StlImporter are the lighter-weight ones, importing just a single mesh. All these deal with meshes represented directly as an almost-ready-to-copy binary though, not sure how well that maps to FBX.

[bqqbarbhg]

Nice! Just had to look a bit into how the whole CTest system is set up as I've never used it but now it should hopefully pass in CI with a dummy ufbx test.

Also, if it fails to build on certain obscure platforms (such as MSVC 2015, UWP or iOS)

I'm just happy that I get indirectly more testing for ufbx with this setup so I'm happy to fix any errors that ufbx may cause!

I don't expect you to learn a whole new library just to write this one plugin, so just do it as you think is best and if there's a different, more Magnum-like way, I'll point that out in the review.

I'm not the biggest fan of STL myself so I'll probably reach out for the Magnum equivalents sooner than later.

[mosra]

Just FYI so it doesn't fall through the cracks -- regarding mesh vertex weights / joint ID attributes, at the moment there's no predefined MeshAttribute for them and both Assimp and GltfImporter import them as custom attributes, which involves some extra work.

I'm in the process of plugging that hole and should have builtin support ready hopefully until end of next week, so to avoid wasting your time best would be if you just ignore / skip those for now, and add them only once the builtin support exists. I'll comment here once that's done.

[bqqbarbhg]

Good to know! Sounds like good timing on my part as currently I'm ignoring skins anyways, just ping me when it's ready to go!

[bqqbarbhg]

First draft for now, completely untested as trying to run it started unloading the .dll for me. Lots of warnings and style issues, I wouldn't spend much time reviewing this (other than personal curiosity) as the code will probably get a couple of passes done still.

In theory this would support scene graphs, meshes, materials, textures, images, lights, cameras, but like mentioned above it's all untested as of now.

[bqqbarbhg]

Managed to get it running with statically built plugins, still not quite sure what the .dll issue is. With some fixes managed to get up to a "Hello cube!" now.

[bqqbarbhg]

After some fixes most of the implemented features should be working(ish) now. At least node hierarchy, materials, textures and images seem to work alright on small test scenes.

[bqqbarbhg]

Went through the comments and improved the documentation. In ufbx news I got rid of the sentinels if using C++11/C23 (or newer) and added the ufbx_metadata.warnings[] I mentioned in the review comment above. I had one more question above but other than that it's getting pretty clear.

I wouldn't merge this as-is quite yet even if it would be at an acceptable state as the embedded ufbx is based on a feature branch and has some features that are not tested up to the standard of the rest of ufbx. I'll run a fuzzing pass overnight, do a bit of additional testing and check that there's nothing fishy left without coverage. After that I think it's high time that I merge the far-strayed fileformat-obj PR and bump the version to v0.2.0, which would be more worthy of embedding.

[mosra]

Thank you!

as the embedded ufbx is based on a feature branch

That would have been my last request, to embed something that is an "official" commit and not a branch, but you already thought of that -- amazing. I'll wait for that, then :)

[bqqbarbhg]

Finally got ufbx updated, took a bit longer than expected as I hit a stack overflow on CI and ended up writing a script that walks the call graph and uses GCC's -fstack-usage to determine the maximum stack size statically so that won't happen again :D

Feel free to squash/merge/edit the PR whenever you have the time!

[mosra]

Squashed & merged as 9e7b5d8 and 3db48ec, with a few very minor changes after, consisting mostly of whitespace changes and doc polishing. Rendered docs here. Due to the embedded ufbx sources you're now also the second most significant contributor, haha:

Thank you very much for all the work you did here. I enjoyed reviewing this code a lot, and hope to see more from you! 🙇

[mosra]

Oh, one more thing -- how complete is ufbx's OBJ support regarding PBR materials? I was just randomly messing around to see what all is imported now, and played with the Autodesk Telescope model which has PBR properties in the material, such as:

...

newmtl gold_metal
	Kd 0.9810 0.7217 0.1090
	Ks 1.0000 1.0000 1.0000
	Tr 0.0000
	d 1.0000
	Tf 1.0000 1.0000 1.0000
	Pr 0.2000
	Pm 1.0000
	Pc 0.0000
	Pcr 0.0000
	Ni 1.5000
	Ke 0.0000 0.0000 0.0000
	illum 2

...

newmtl wire_177088027
	Ns 32
	d 1
	Tr 0
	Tf 1 1 1
	illum 2
	Ka 0.6941 0.3451 0.1059
	Kd 0.6941 0.3451 0.1059
	Ks 0.3500 0.3500 0.3500

But when I look at those with magnum-sceneconverter --info-materials -I UfbxImporter AutodeskTelescope.obj, I get just this:

What convinced me to look further was that PbrClearCoat was in the type set, but no ClearCoat layer was actually present. So I commented out this line and got the following:

Which is in-line with what the *.mtl has, but then (as the comment in the code says) it included default-derived PBR properties also for the non-PBR wire_177088027 material which it shouldn't, so just flipping that bit for all OBJs isn't the right thing to do it seems. But maybe I could infer that from the collected material types? For the wire material it's just Phong, so that would work.

I was not sure whether this is something that needs to be fixed in ufbx itself or if it's something specific to this particular plugin implementation -- let me know if I should open an issue on the ufbx repo directly instead. Thanks!

[bqqbarbhg]

Oh nice catch! I didn't think to add the PBR feature I added for the plugin to the obj material, which is actually the first material model that can be conditionally PBR or not. Fortunately I had the groundwork for it and it ended up being a two line fix: ufbx/ufbx#50 I'll bump the version and merge when CI passes, I assume you can just update the version without further PR for that?

Regarding follow up I'm going to draft the animation PR soonish, and I've really enjoyed the the reviews and comments in general as well!

Edit: Turns out moving the repository to an organization broke my Raspberry Pi Actions runner and the GitHub access tokens aren't cooperative, might take a short while that I get it working.

[mosra]

I assume you can just update the version without further PR for that?

Yes -- just did that in 2429547. Thanks a lot for the ultra-fast fix :)

I'm going to draft the animation PR soonish

Amazing! For that the skinning-related groundwork I mentioned above got merged recently, you might want to look at mosra/magnum@f447e99, 19af6b8 and cf9c300 to see how it's exposed in MeshData and what was needed to do in AssimpImporter / GltfImporter for this. Latest commits of magnum-player also support skinning (tested on a bunch of glTF files), so hopefully working with FBX as well.

I suppose in ufbx you're closer to the Assimp way where it's a variable amount of joints / weights per vertex, instead of having multiple separate 4-component attributes. Repeating for clarity -- don't bother with the fallback compatibility attributes, implement just the builtin ones.

[bqqbarbhg]

Sounds good, the new representation looks clean!

I suppose in ufbx you're closer to the Assimp way where it's a variable amount of joints / weights per vertex, instead of having multiple separate 4-component attributes.

Yes pretty much, FBX itself stores vertex index/weight pairs in "skin clusters" aka bones but ufbx builds a convenience structure with N bones per vertex (sorted by weight, so you can just take first four for example and be done with it). Should be straightforward to implement!

[bqqbarbhg]

Bit of an off-topic follow up but I'm posting it here as it's 100% inspired by your data-based material comparison, which was super nice to use (especially the clever line number thing I saw in some tweet of yours). I'm using an external files for real-world test cases like the Autodesk Telescope, so I created a small DSL so now I can verify the loaded materials in my dataset tests:

version 1

material black_rough
    shader_type wavefront_mtl

    fbx.diffuse_factor implicit 1.0
    fbx.diffuse_color 0.0100 0.0100 0.0100
    fbx.specular_factor implicit 1.0
    fbx.specular_color 1.0 1.0 1.0
    fbx.transparency_factor 0.0
    fbx.emission_color 0.0 0.0 0.0
    fbx.normal_map.texture maps\orangepeel_very_low_normal.png

    pbr.base_factor implicit 1.0
    pbr.base_color 0.0100 0.0100 0.0100
    pbr.roughness 0.4
    pbr.metalness 0.0
    pbr.coat_factor 0.0
    pbr.specular_ior 1.5200
    pbr.emission_color 0.0 0.0 0.0
    pbr.normal_map.texture maps\orangepeel_very_low_normal.png

    features.pbr 0
    features.metalness 1
    features.diffuse 1
    features.specular 1
    features.emission 1
    features.transmission 1
    features.coat 1

material black_metal
    ...

Which is a bit wrong for demonstration and would result in the following errors:

autodesk-telescope.ufbx.mat:16: Material 'black_rough' pbr.roughness has wrong value: got (0.300), expected (0.400)
autodesk-telescope.ufbx.mat:23: Material 'black_rough' features.pbr mismatch: got 1, expected 0

This actually revealed a bug that MTL file d property would result in wrong ufbx_material.fbx.transparency_factor values as FBX stores them as inverted (ie. 1.0 being fully transparent), so going to update ufbx with a fix for that soonish. Anyways apologies for the off-topic rambling but just wanted to thank you for the idea for diffing materials :D

[mosra]

Hey, that's nice, glad to see I'm an influencer in this regard :D

The idea for diffing materials was an accident for the most part, it started with being unhappy about excessive repetition in material tests, and then it took me almost a year to finally implement something that wasn't some half-useless ad-hoc thing.