Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

AssimpImporter: import animations #97

Closed
wants to merge 29 commits into from

Conversation

pezcode
Copy link
Contributor

@pezcode pezcode commented Jun 18, 2021

This PR adds support for importing node animations with AssimpImporter.

doAnimation imports translation vectors, rotation quaternions and scale vectors in separate tracks for each animation channel. Interpolation is assumed to always be linear as Assimp doesn't support any other interpolation types, and it's impossible to get the source data.

I reused the TinyGltfImporter config values to perform the same sanity fixups. While some Assimp importers perform these themselves (e.g. the FBX importer makes sure to get the shortest rotation path), not all of them do.

What's missing:

  • tests - a majority can be reused from TinyGltfImporter
  • check against older Assimp versions as well as latest master
  • there's a bug with uninitialized quaternions when using magnum-player, have to investigate see comments about spline-interpolated glTF
  • mention animation support in the docs

Commissioned by Wonderland.

@pezcode
Copy link
Contributor Author

pezcode commented Jun 18, 2021

To expand a bit on the interpolation issues:

In the case of glTF cubic spline animations, Assimp simply takes the spline knots. While this is not terribly wrong, it's still wrong, but there is no way to get the interpolation function out of Assimp. Same thing applies to step interpolation, although that's probably less relevant.

There's also a nasty bug that only got fixed less than a year ago (so way after the latest tagged release 5.0.1): glTF spline tangents are read as key frame values instead of skipping them, producing unusable garbage and, worst of all, leaving no way to reliably detect that we got garbage.

Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this!

Some ideas that could hopefully unblock the glTF import disaster :)

src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp Outdated Show resolved Hide resolved
src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp Outdated Show resolved Hide resolved
src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp Outdated Show resolved Hide resolved
src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp Outdated Show resolved Hide resolved
@mosra mosra added this to the 2020.0b milestone Jun 19, 2021
@mosra mosra added this to TODO in Asset management via automation Jun 19, 2021
@mosra mosra mentioned this pull request Jun 19, 2021
19 tasks
Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Got just a few minutes today, so only a bunch of random comments :)

Looks great so far! 👍

This file is part of Magnum.

Copyright © 2010, 2011, 2012, 2013, 2014, 2015, 2016, 2017, 2018, 2019,
2020, 2021 Vladimír Vondruš <mosra@centrum.cz>
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add yourself here, to all touched files :)

src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp Outdated Show resolved Hide resolved
src/MagnumPlugins/AssimpImporter/AssimpImporter.h Outdated Show resolved Hide resolved
src/MagnumPlugins/AssimpImporter/AssimpImporter.h Outdated Show resolved Hide resolved
@pezcode
Copy link
Contributor Author

pezcode commented Jun 23, 2021

Got just a few minutes today, so only a bunch of random comments :)

Looks great so far! 👍

Thanks for taking the time to check in even if you're busy, appreciate it. I adressed your recent feedback locally, I'll push it along with all the tests probably tomorrow. Assimp is still abusing me with a funny bug, gotta fix that first.

@pezcode
Copy link
Contributor Author

pezcode commented Jun 24, 2021

Here are the tests 🐌 A few related notes:

  • TinyGltfImporterTest files and cases are re-used where possible. The animation*.gl* needed changes, because Assimp doesn't import animations from glTF files that don't have a scene (there's also a test for that behavior). So I copied them over and modified them in this importer test directory.
  • Re-use doesn't work for the spline-interpolated data (see discussion above), so I simply test that the data makes it there as linearly interpolated animations.
  • Trying to convert the existing animated glTF files into FBX/Collada (.dae) made me lose so much hair I decided to create a simple test scene in Blender, add key-frames and export them directly. This kind of worked but not well enough to be able to add tests of the same fidelity as the glTF ones. Every exporter seems to have a different way to bake and merge samples, so the outputs are wildly different. In animation() (as opposed to animationGltf()), these exported files are loaded and then some sanity checks are performed. It's probably possible to extend this to check certain key-frames but it will be painful, see the @todo comment at the end of it.
  • There is no test for Animation::Extrapolation beyond expecting Constant in tests. There is currently no code in Assimp that sets this except for the .irr importer, and I couldn't even find a sample file using animations, so...

Some general comments on format support:

  • glTF is poisoned. I had to remove the invalid animation test, because out of bounds indices in the .gltf ended up in wild asserts in the underlying json library, so there's no way to even get useful error output. There is also a mind-blowing bug that reads beyond key/value buffers and you end up with extraneous, bogus data and there is no way to detect this. I would almost go as far as putting a big red warning on the importer docs à la "DON'T EVEN TRY TO USE THIS WITH GLTF".
  • FBX seems to work fine, but I couldn't for the life of me figure out how to export files correctly for better tests. Either Blender exported no animations, or it exported all animations targetting all(??) objects. I tested a whole slew of matching FBX/glTF files off Sketchfab with magnum-player and they all seemed to work. The ones that were broken were still broken with TinyGltfImporter.
  • Collada works judging by the test files and a few random scenes off the internet, but I didn't put that much effort into checking

I wanted to test with older versions, but various vcpkg shenanigans threw a wrench in my plans. Not sure how much of this is tested on CI, but I could give this another try with WSL (or as a CMake subdirectory - is that supported for Assimp?). So for now, consider the test extent to be 5.0.1 and master.

Copy link
Owner

@mosra mosra left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The more you look at Assimp, the more bugs it has 🙈

Thanks!

/* There is a *ridiculous* bug in Assimp 5.0.1(?) with glTF animations that makes it
ignore the value sampler size and always uses the key sampler size
(instead of using the minimum of the two). Wouldn't be surprised
if this produces an out-of-bounds access somewhere, too. */
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where's the trash fire emoji when I need it? 🔥

About the OOB accesses, the ASan CI build is using Assimp 3.2, so I guess we won't get notified there. I suspect it'll be failing locally on my machine tho, will pull the branch and check once I have time.

src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp Outdated Show resolved Hide resolved
src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp Outdated Show resolved Hide resolved
@mosra
Copy link
Owner

mosra commented Jun 24, 2021

I would almost go as far as putting a big red warning on the importer docs à la "DON'T EVEN TRY TO USE THIS WITH GLTF".

I already mark the support as red in the file format tables, but perhaps this case would warrant a black "DANGEROUS" label 😆

image

There is no test for Animation::Extrapolation beyond expecting Constant in tests. There is currently no code in Assimp that sets this

Good to know. Can you add this as a comment next to the switch to document the lack of code coverage for these lines?

Collada [...] I didn't put that much effort into checking

That's fine. People who still use Collada for animations are fully responsible for their fate, not us. The format is an overengineered insanity.

I wanted to test with older versions. Not sure how much of this is tested on CI, but I could give this another try with WSL (or as a CMake subdirectory - is that supported for Assimp?)

What's tested is 3.2 on Ubuntu 16.04, whatever is the current version on Homebrew and 5.0.1 on Windows. Assimp supports a CMake subproject (recipe in the docs) but I don't know how far back this works, it's possible that early 4.x might have issues that my Find module isn't aware of. Personally I'd only manually test with 4.0 and let the CI take care of the other versions, or even don't bother with version 4 at all.


About the CI failures:

  • macOS and MinGW CI failures unrelated to your code should get fixed if you rebase on top of current master
  • for the Linux failures, CheckAssimpVersion.cpp is referenced as checkAssimpVersion.cpp in CMake, and thus works only on case-insensitive filesystems (IIRC, with Git on Windows the rename has to be done in two steps, CheckAssimpVersion.cpp -> a.cpp -> commit -> a.cpp -> checkAssimpVersion.cpp, and then squash the commits together)

@pezcode
Copy link
Contributor Author

pezcode commented Jun 25, 2021

CI still fails, compiler doesn't like the tuple assignment, but I'll rework that bit of code in another commit.

Thanks for the detailed feedback with code snippets, makes it a breeze to implement any changes 😃

Exercises left:

  • show std::tuple the door
  • file massaging and basic player output testing in animation() test
  • try to compile with older Assimp, at least 4.0

@pezcode
Copy link
Contributor Author

pezcode commented Jun 26, 2021

I fixed the CI failure on Linux locally (loading .gltf without checking for the Assimp version), but the Mac/Mingw one needs some more debugging. Track order seems to be mixed up there.

Final stretch 🙏

@mosra
Copy link
Owner

mosra commented Jun 26, 2021

Track order seems to be mixed up there.

Sigh, FFS. I think this is the problem: https://github.com/assimp/assimp/blob/d855ec2472e82066aedb6d43efd63355d2eb2ef2/code/AssetLib/glTF2/glTF2Importer.cpp#L1364 There's no guarantee about iteration order for a std::unordered_map, so this stupid thing just gives out something totally different on every platform.

So... I guess the test will need to do some mapping / reordering first (based on target type and target ID, maybe?) and then compare the ordered IDs. Probably not worth the extra maintenance hell to do this directly in the importer code, just in the test.

@@ -482,9 +482,9 @@ void AssimpImporterTest::animation() {

constexpr Vector3 rotationData[KeyCount]{
{0.0f, 0.0f, 0.0f},
{0.0f, 0.0f, Float{Rad(-90.0_degf)}},
{Float{Rad(90.0_degf)}, 0.0f, Float{Rad(-90.0_degf)}},
{Float{Rad(135.0_degf)}, 0.0f, Float{Rad(-90.0_degf)}}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Any idea why GCC 4.8 doesn't like the curly braces here?

error: cannot convert 'Magnum::Rad {aka Magnum::Math::Rad}' to 'Magnum::Float {aka float}' in initialization
{Float{Rad(135.0_degf)}, 0.0f, Float{Rad(-90.0_degf)}}

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As far as I know, early C++11 spec didn't allow using {} for conversion operators, only for constructors. Old Clang (3.8) has the same issue. Similar issue is with initializing references, e.g. int& a{b}doesn't compile there. Newer compilers are more consistent and allow {} for all those cases.

C++17 then allows using {} for enum initialization as well (the main reason was to be able to do e.g. std::byte{56}).

@pezcode
Copy link
Contributor Author

pezcode commented Jun 27, 2021

Look at all these green checkmarks 😱🎉

I did have to skip checking expected rotation values for Collada files, but everything else is properly checked now. Note that this is not an issue with Assimp, but purely with Blender's Collada exporter being complete rubbish.
Turns out I should learn how to use quaternions properly. Everything is properly checked now 👀

Pending any review changes (and adding copyright data), this is complete from my side.

@codecov
Copy link

codecov bot commented Jun 27, 2021

Codecov Report

Merging #97 (ae5b7be) into master (9baaadb) will decrease coverage by 0.00%.
The diff coverage is 94.52%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
- Coverage   94.63%   94.63%   -0.01%     
==========================================
  Files         101      101              
  Lines        7257     7453     +196     
==========================================
+ Hits         6868     7053     +185     
- Misses        389      400      +11     
Impacted Files Coverage Δ
...rc/MagnumPlugins/AssimpImporter/AssimpImporter.cpp 90.72% <94.52%> (+1.33%) ⬆️

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 9baaadb...ae5b7be. Read the comment docs.

@mosra mosra marked this pull request as ready for review June 28, 2021 08:42
@mosra
Copy link
Owner

mosra commented Jun 28, 2021

I squashed the commits slightly, keeping the major parts separate, and merged as 9baaadb...e733222. Apart from the "integration tests" with Littlest Tokyo glTF and Steam Locomotive using magnum-player, which went great, I checked locally with AddressSanitizer and there were no out-of-bounds reads or memory issues, only a silly

==579355==ERROR: AddressSanitizer: alloc-dealloc-mismatch (operator new vs operator delete []) on 0x60300001daa0

inside ~aiScene(), but I don't care that much, really. Maybe it's already fixed in Assimp master, whatever.

Thanks for a great piece of work, and sorry for having to deal with this buggy 💩

@mosra mosra closed this Jun 28, 2021
Asset management automation moved this from TODO to Done Jun 28, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Development

Successfully merging this pull request may close these issues.

None yet

2 participants