UfbxImporter: Support for animation and skinning

[bqqbarbhg]

Hi, I'm back at the grind!

This PR currently adds support for animations, I'll add skinning in the same PR if that's fine by you as it's quite related.

Some background information on FBX animation to explain what's going on:

• FBX stores animations per-component as 1.5D Bézier curves (or linear/constant)
  • The plugin needs to merge keyframe times from multiple curves and resample the Béziers due to the time tangent
• Rotations are represented with Euler angles* with configurable rotation order
  • Even linear rotations have to be resampled if we want to be accurate, currently configuration option resampleRotation
• Rotation/scaling may affect translation with pivots/offsets (!)
  • Currently detected as "complex" translation/rotation and keyframe times are sampled from all properties that might affect the result, these should be rare enough that this handling is enough.
• Animation curves may have step-like transitions between constant keyframes
  • Handled using configuration option constantInterpolationDuration for step duration/epsilon
• Animations may be composed of multiple layers
  • Currently flattened by default, configuration option animationLayers to retain layers but all layer metadata (such as blend modes/weights) is lost
• In practice most FBX files contain resampled animation (due to constraints or different curve types in the program)
  • Configuration option minimumSampleRate prevents additional resampling when the keyframes are dense enough

* (one of the rotation orders is called SPHERIC which might imply some sort of quaternion interpolation, but I haven't been able to produce a file using that)

Classic disclaimer about everything being very much work-in-progress but I wanted to open the PR in case you have differing ideas to the solutions I described above. Also, don't worry about the embedded ufbx diverging here, I'm going to merge that to the main repository after feeling out the changes (or remove it in case it turns out to be a bad idea, though I've been wanting to add those defines for ages).

Edit: Oh and the Visibility support is there as kind of a test for a non-transformation property, it does exist in some files and is sometimes used but I don't mind removing that either.

[codecov]

Codecov Report

Base: 96.97% // Head: 96.78% // Decreases project coverage by -0.18% ⚠️

Coverage data is based on head (9c8ade0) compared to base (94aefed).
Patch coverage: 86.46% of modified lines in pull request are covered.

❗ Current head 9c8ade0 differs from pull request most recent head c620fbe. Consider uploading reports for the commit c620fbe to get more accurate results

Additional details and impacted files

@@            Coverage Diff             @@
##           master     #136      +/-   ##
==========================================
- Coverage   96.97%   96.78%   -0.19%     
==========================================
  Files         133      133              
  Lines       14682    14950     +268     
==========================================
+ Hits        14238    14470     +232     
- Misses        444      480      +36     

Impacted Files	Coverage Δ
src/MagnumPlugins/UfbxImporter/UfbxImporter.h	100.00% <ø> (ø)
src/MagnumPlugins/UfbxImporter/UfbxImporter.cpp	96.26% <86.46%> (-3.74%)	⬇️

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.

[bqqbarbhg]

Managed to implement skinning: demo mp4

It was very straightforward with the new API for vertex joint/weight data. I did run into a couple of issues though:

I didn't find a way to report begin/end time for AnimationData which means many animations that should loop cleanly don't as they contain errant keyframes outside of the "playable area". This is extra problematic with files which have multiple consecutive takes as different animation stacks. If you want to avoid the complexity of arbitrary begin/end times one option would be to internally crop and offset the animations to fit between the begin and end, adding keyframes to the boundaries to render correctly.

Skinning was broken in magnum-player in some files (including the demo above) due to the skinned mesh having a transform. I got it working by hacking the player to zero out transforms for skinned meshes for now. ufbx reports joint skin matrices from bone world space to local mesh space. Thus including the transform of the mesh causes the meshes to be double translated and go way off. Not sure yet how other importers handle it, so can't say if this is an issue with the viewer or UfbxImporter at least intuitively to me it seems like including both mesh and skin bone transformations would always run into the issue.

[mosra]

Hi, thank you! Again extremely sorry for a delayed review...

I made some comments but still want to go through the animation keyframe code more thoroughly to fully understand what's going on.

the Visibility support is there as kind of a test for a non-transformation property

That's good to have, and it nicely matches the Visibility field in the scene. I just need to improve the APIs for custom animation targets so they're not that annoying to use, can be named etc.

(Going off on a tangent -- I was thinking about how to make the Visibility field builtin in a way that it's "visible" (so, true) by default, and realized that having the inverse, i.e. Hidden might make more sense from that perspective. But that's probably wildly inconsistent with how it's named everywhere else, right? I also saw glTF has a KHR_nodes_disable extension proposal, which was originally named KHR_visibility_nodes, so I suppose they're facing the same issue, heh.)

I didn't find a way to report begin/end time for AnimationData

I had a feeling that "it should definitely be there" so I checked and apparently it's only in Animation::Player, but not in the tracks themselves. Strange. It definitely makes sense to have that in the tracks directly, I'll add it (that's I guess a third TODO for me, in addition to the two below).

Skinning was broken in magnum-player in some files

I never said it was working correctly there :P A bug like that is completely possible, I was in a time pressure when implementing skinning support in the player and so tested with just a few glTF files and they all looked alright.

But then at some point after I loaded this skinned FBX model with Assimp and was greeted by the following nightmare trigger:

I shook it off as yet another Assimp weirdness and didn't investigate further, but now that's probably the same bug you were experiencing? :D Can you send me the patch you did to make it working? I'll need to go through the glTF spec again to see what exactly I was misunderstanding there, and why none of the models I tried exposed that behavior (fourth TODO for me).

[mosra]

Hmm, I should probably fix this now. I have a TODO from when I was doing skinning support elsewhere and was hitting similar pain points -- the AnimationData APIs pre-date the MeshData / SceneData by several years and are unnecessarily verbose and hard to use in comparison.

[mosra]

Ugh, and this I have to fix as well, to have similar utilities as with sceneFieldCustom() etc.

[bqqbarbhg]

Thanks for the review and no worries about the delay!

and realized that having the inverse, i.e. Hidden might make more sense from that perspective. But that's probably wildly inconsistent with how it's named everywhere else, right?

I'm not sure how it's done in other file formats (and actually looking at KHR_nodes_disable I haven't looked at whether FBX visibility is recursive). After a bit of thought I quite like Hidden, it implies that it has been hidden with intent, Visible is a bit more ambiguous (but on the other hand Visible has less negation confusion going on, naming is hard :D)

Can you send me the patch you did to make it working?

I just did a super nasty hack to make skinning look correct in ScenePlayer.cpp PhongDrawable::draw() where I override the transform matrices with the camera matrix if skinned:

-    if(_jointMatrices) _shader
-        .setJointMatrices(_jointMatrices)
-        .setPerVertexJointCount(_perVertexJointCount, _secondaryPerVertexJointCount);
+    if(_jointMatrices) {
+        _shader
+            .setJointMatrices(_jointMatrices)
+            .setPerVertexJointCount(_perVertexJointCount, _secondaryPerVertexJointCount)
+            .setTransformationMatrix(camera.cameraMatrix())
+            .setNormalMatrix(camera.cameraMatrix().normalMatrix());
+    }

[mosra]

Thanks for the patch! Hmm, I need to look into this more, then. Still not sure what exactly should I be doing.

Just to be sure -- if you load the model I linked above with ufbx, does it behave the same broken way with the unpatched magnum-player as on the screenshot?

[bqqbarbhg]

Uhh, looks like it's a bit of both, I still seem to have some DLL issues and can't get AssimpImporter to work but here's UfbxImporter:

ufbx patched:

ufbx unpatched:

[mosra]

ufbx unpatched:

Oh hey that's the same issue I had with the glTF Cesium Man, it also had the actual objects 90° rotated from where the mesh got transformed. (And I "fixed" it by patching the file to add another rotation to everything, heh.) This is starting to make sense to me, just not fully yet.

The Assimp behavior is then probably mainly just an Assimp bug, this additional rotation is a minuscule issue compared to what it does with the legs and eyes and aaargh.

[bqqbarbhg]

Oh hey that's the same issue I had with the glTF Cesium Man, it also had the actual objects 90° rotated from where the mesh got transformed. (And I "fixed" it by patching the file to add another rotation to everything, heh.) This is starting to make sense to me, just not fully yet.

Very possible if the node containing the mesh itself is transformed (not the skeleton), which would cause the same issue.

The Assimp behavior is then probably mainly just an Assimp bug, this additional rotation is a minuscule issue compared to what it does with the legs and eyes and aaargh.

Yea I'm curious how it got to look so off :D Now that I think about it, as the issue with skinning is that for correct skinning the matrices should plug together like (using Sebastian Sylvan's neat matrix naming scheme, albeit with the standard non-D3D "right-to-left" multiplication order):

//            (bone xform)  (inverse bind)   (mesh vertex)
worldVertex = boneToWorld * geometryToBone * geometryVertex

But what the player is doing was something like this, where we end up transforming a point already in world space by the nodeToWorld matrix:

//            (node xform)  (bone xform)  (inverse bind)   (mesh vertex)
worldVertex = nodeToWorld * boneToWorld * geometryToBone * geometryVertex

Anyways long way to say that this bug can in the worst case only transform the whole skinned mesh (as it did with your glTF and the ufbx unpatched cases) as the erroneous matrix is composed last.

---

In other news got the test coverage back to 100%, still need to go over with a cleanup pass and crunch through my dataset when I got time, so aside from some latent bugs and stylistic issues should be ready for reviewing! I can incorporate some fixes still if you wanted to refactor AnimationData before merging this PR.

[mosra]

Hi, extremely sorry for a massive delay again. I finally managed to resolve the TODOs on my side which were blocking this PR:

• since mosra/magnum@c04b375 there's Trade::animationTrackTargetCustom() for creating custom track target IDs, plus there were some minor renames
• since mosra/magnum@694e60b there are new animationTrackTargetName() and animationTrackTargetForName() APIs on the importer allowing you to provide a string name for the custom visibility field
• the AnimationData constructors were reworked in mosra/magnum@94e9961 to allow construction from type-erased views, so it's not needed to branch on a type anymore; for a practical example the other plugins were adapted in e69bfa7
• and I looked into the magnum-player transformation bug, finally understood why was it wrong and pushed a fix in mosra/magnum-extras@0851a1a (with you as an author)

And finally,

I didn't find a way to report begin/end time for AnimationData

there actually is a way, the duration parameter in this constructor. Somehow I didn't see it myself at first either, sorry. Hopefully that's enough to make it work and there isn't some other omission somewhere else in Magnum.

The PR looks fine to me in its current state, so what's left is just adapting to the Magnum-side changes listed above, and possibly upstreaming the ufbx changes in case you don't have them in your repo yet. The only remaining thing I don't exactly know where to put (and how to test) would be the custom track duration.

I completely understand if you don't have time for this PR anymore -- I'm happy to take over in that case. Let me know, thank you and my apologies!

[bqqbarbhg]

No worries at all about the delay, I figured that you wanted to have this done right instead of hacked together with how the animation representation was at the time!

I can adapt to the Magnum changes and clean up this PR in probably a bit over a week or so, and I'll probably include some new changes from ufbx as well (currently writing documentation and changing the public API whenever I find myself having to explain why something is weird).

there actually is a way, the duration parameter in this constructor

Nice, and it seems to even be a range so I can have the start time there as well, perfect!

Also seems like my GitHub notifications are bugged as I didn't get any info about your reply (and looks like I've got a couple of issues in repos I wasn't aware of) so I might not reply at my usual speed until I get that sorted out :D

[mosra]

Suggested change

	Containers::StridedArrayView1D<const void> stridedValues{valueBytes, times.size(), animTrack.valueElementSize};
	Containers::StridedArrayView1D<const void> stridedValues{valueBytes, times.size(), std::ptrdiff_t(animTrack.valueElementSize)};

There's a single signed-unsigned conversion warning on GCC, this should fix it.

[mosra]

Such operation would belong in some processing tool, I don't think the importer plugin should deal with that. It's also easier to test that way, and reuse it outside of FBX files.

[bqqbarbhg]

There's probably a nicer way to do this but this is the best I could come up with

[bqqbarbhg]

Converting from type-erased StridedArrayView2D<char> to Containers::StridedArrayView1D<const void> that is

[mosra]

It'd be animTrack.values.transposed<0, 1>()[0] (where the [0] doesn't get the first element bytes anymore, but rather first bytes of all elements), and this is a StridedArrayView1D<char> which should be implicitly convertible to the void variant.

But yes, this part is a bit shitty, unfortunately given how unconventional the API is I don't really have from where to steal (and I refuse to look at std::mdspan for inspiration).

What could probably solve this better is a AnimationTrackData constructor that takes a StridedArrayView2D<const char>. Other *Data APIs have this and it's useful, not sure why I didn't add that here as well in the original round of updates. Will do so now.

[bqqbarbhg]

Shouldn't it be StridedArrayView2D<const char>? 1D would be individually spaced out bytes without any idea about element size or am I confused? And yes the API is really novel and I like it :D

[mosra]

Yes, 2D, updated the message, sorry.

I'm answering 15 threads at once and can't keep my thoughts straight 🙈

[mosra]

I changed my mind after seeing how many constructors there already is -- don't really want to basically double their count again 😅 So transposed<0, 1>()[0] it is.

[bqqbarbhg]

Seems to work flawlessly, nice!

[bqqbarbhg]

Ouch, can't repro the LeakSanitizer check locally on Linux, so gonna do a few cursed commits..

[mosra]

can't repro the LeakSanitizer check crash

I was checking it in the CI and have no idea what could it be. That thing usually reports what went wrong, but this time it seems to just crash itself. It's building on an ancient system, so it's possibly you hit some nasty bug in LSan itself.

Feel free to ignore that problem, I can look into that myself after. CircleCI has a nice option to rerun with SSH which is much better for iteration times -- not sure if you can do that from your end, but I can.

[bqqbarbhg]

Alright great, I'll stop my desperate attempts at bisecting then :D

[mosra]

Managed to repro the LSan crash -- it happens only if running through ctest, not directly... which means I can't easily use --only / --except to bisect which test case triggers this :/

This seems to be related and the suggested ASAN_OPTIONS=intercept_tls_get_addr=0 works: google/sanitizers#1322 The last comment there says a fix was pushed to LLVM repos last week, which isn't really useful given that I want to build this on Ubuntu 18.04 😆

Maybe you can also reproduce this locally now -- just need to run through ctest -R Ufbx -V instead of executing the test directly. Oh also, the CI has CMake 3.4, it's possible this is no longer a problem with newer CMakes.

I'm investigating further through SSH...

[mosra]

I have no idea what's going on. This is the change that makes the test pass without LSan crashing:

diff --git a/src/MagnumPlugins/UfbxImporter/Test/UfbxImporterTest.cpp b/src/MagnumPlugins/UfbxImporter/Test/UfbxImporterTest.cpp
index 3ca685b4..7476a51c 100644
--- a/src/MagnumPlugins/UfbxImporter/Test/UfbxImporterTest.cpp
+++ b/src/MagnumPlugins/UfbxImporter/Test/UfbxImporterTest.cpp
@@ -307,7 +307,9 @@ UfbxImporterTest::UfbxImporterTest() {
               &UfbxImporterTest::textureTransform,
               &UfbxImporterTest::textureWrapModes,
 
+#if 0
               &UfbxImporterTest::imageEmbedded,
+#endif
               &UfbxImporterTest::imageExternal,
               &UfbxImporterTest::imageExternalFromData,
               &UfbxImporterTest::imageFileCallback,

As far as I see there's nothing in this PR that would even remotely touch image-related code, and a build that I made few hours ago off master isn't crashing here. So I suspect it's happening randomly and I was just lucky to not hit that LSan bug until now?

TLS is used for thread-safe access to plugin managers and debug output redirection, but seriously I've never had a problem with that interacting with LSan until now. I guess I'd have to read the whole thread on the sanitizers repo to get an idea what's happening.

If we don't figure this out soon, to avoid wasting too much time the solution here would be to do a special case on the CI and run UfbxImporter tests separately with ASAN_OPTIONS=intercept_tls_get_addr=0. It used to be done in a similar way for certain dumpster fire libraries (see 613b3f3 and a120b9c), the only difference here would be that it's not your library what's a leaky/crashy dumpster fire but vendor's :D

[bqqbarbhg]

I assume morph targets are not supported on Magnum yet? I at least couldn't find any way of representing them from UfbxImporter.

[mosra]

Yes, not supported, the interfaces are not ready for that yet. It's on my near-term TODO list tho.

[mosra]

Hey, sorry for the accidental silence here -- seeing the "documentation draft" commit I assumed you'd be pushing some additional commits after but you didn't, and looking at it it doesn't really seem "drafty" at all :) Which means this PR is (mostly) ready, right?

I definitely don't want to delay it any further by any additional requests, I already caused enough of that. As I see it, there's this left:

• Smashing the sanitizer CI until it stops being weird (which would be my task)
• That transposed<0, 1>()[0] suggestion above, or just leave it as-is if it doesn't do the right thing on the first try, it's fine that way as well
• Syncing the ufbx copy here with upstream (which I assume is Add changes from MagnumPlugins integration ufbx/ufbx#63, right?)

Again, absolutely no pressure here, just want to make sure you weren't waiting for some action from my side.

[bqqbarbhg]

Ah yes no worries, I still wanted to do a quick pass over everything. I was busy wrapping up some other things at work, gonna clean this up for merge now!

[bqqbarbhg]

Ah and just to clarify, I've now gone over it so feel free to review/cleanup/merge, only the lingering CI issue remains :D

[mosra]

I squashed the commit series a bit and merged as e40a9ca...70f5644. The only bit I didn't include was f8571a5, I have a bunch of these "weird uncovered lines" in various other places as well and simply refuse to work around gcov / lcov bugs :D They'll be gone eventually, either after a lcov update or after a compiler update.

I did just very minor whitespace shuffling on top -- your code was mostly perfect to begin with. The FBX model I attempted to load in Assimp isn't cursed with ufbx anymore, and everything is great ✨

Cheers, and thanks a lot for your work!