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鈥檒l occasionally send you account related emails.

Already on GitHub? Sign in to your account

AssimpImporter: import skin data #99

Closed
wants to merge 21 commits into from

Conversation

pezcode
Copy link
Contributor

@pezcode pezcode commented Jul 3, 2021

This PR adds support for importing skins as well as joint weight mesh attributes with AssimpImporter.

doSkin3D imports skins (joint node ids + corresponding inverse bind matrices) for each mesh with skin data. Assimp doesn't expose any of the original (possibly multi-mesh) skins and instead copies joint data into each mesh, duplicating bones if necessary. The new mergeSkins option is meant to allow users to merge all skins in the scene into one, removing duplicate joints and adjusting mesh attributes.

doMesh imports joint ids and weights as custom mesh attributes (built-in support for those is WIP in mosra/magnum#441) named "JOINTS" and "WEIGHTS". Multiple sets (4 weights per set) of these are imported as layers of the same custom mesh attribute id. This allows more than 4 weights per vertex but requires changing the new maxJointWeights option (default: 4).

This is very much WIP, but I pushed my current progress so you can intervene early if anything is completely off-track 馃槃


Commissioned by Wonderland.

@pezcode
Copy link
Contributor Author

pezcode commented Jul 3, 2021

Todo:

  • mergeSkins is currently not de-duplicating bones
  • investigate AI_CONFIG_IMPORT_NO_SKELETON_MESHES shenanigans
  • Finish tests, a good part is already tested, but not everything
    • skin matrices, something off about rotations there
    • joint ids and weights from a common test file
    • mergeSkins
    • maxJointWeights, need some useful test data

@@ -287,9 +296,6 @@ verbosity levels in each instance.
- Custom mesh attributes (such as `object_id` in Stanford PLY files) are
not imported.
- Texture coordinate layers with other than two components are skipped
- For some file formats (such as COLLADA), Assimp may create a dummy
"skeleton visualizer" mesh if the file has no mesh data. For others (such
as glTF) not.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is fixed by setting AI_CONFIG_IMPORT_NO_SKELETON_MESHES, but there's another comment down below in the Scene import section ("[...] there's sometimes just a single root node") which I'm not sure about. The node collapsing doesn't happen anymore with this setting (the test got adapted accordingly), but I wonder if this is the only circumstance where this happens.

Copy link
Owner

Choose a reason for hiding this comment

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

/* Skinned mesh imported into Blender and then exported */

// TODO attribution:
// https://github.com/KhronosGroup/glTF-Tutorials/blob/master/gltfTutorial/gltfTutorial_019_SimpleSkin.md
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Where would I best put attribution here? The tutorials are licensed under CC-BY 4.0 so anything allowed as long as we credit the author and add a link to the license. I'm afraid recreating a skinned mesh with Blender is a bit out of my depth.

Copy link
Owner

Choose a reason for hiding this comment

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

Is that where the skin.gltf is from? Looks rather different, did you re-export with Blender? The test files from TinyGltfImporter could not be reused?

In any case, attribution should go here, something like

diff --git a/src/MagnumPlugins/AssimpImporter/AssimpImporter.h b/src/MagnumPlugins/AssimpImporter/AssimpImporter.h
index 0da7c4c3..2d03fa5c 100644
--- a/src/MagnumPlugins/AssimpImporter/AssimpImporter.h
+++ b/src/MagnumPlugins/AssimpImporter/AssimpImporter.h
@@ -128,7 +128,11 @@ This plugin provides `3dsImporter`, `Ac3dImporter`, `BlenderImporter`,
     licensed under @m_class{m-label m-success} **BSD 3-clause**
     ([license text](http://assimp.org/index.php/license),
     [choosealicense.com](https://choosealicense.com/licenses/bsd-3-clause/)).
-    It requires attribution for public use.
+    It requires attribution for public use. The tests use a file from the
+    [A Simple Skin](https://github.com/KhronosGroup/glTF-Tutorials/blob/master/gltfTutorial/gltfTutorial_019_SimpleSkin.md)
+    glTF tutorial, licensed under @m_class{m-label m-success} **CC-BY 4.0**
+    ([license text](https://github.com/KhronosGroup/glTF-Tutorials/blob/master/LICENSE),
+    [choosealicense.com](https://choosealicense.com/licenses/cc-by-4.0/)).
 
 @section Trade-AssimpImporter-usage Usage

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, it's re-exported from Blender, mainly because I needed an extra un-skinned mesh in the scene. The original TinyGltfImporter skin test files can't be used with Assimp because they have no meshes. Assimp only exposes bones through meshes, so no meshes = no bones. Main reason I imported and re-exported that tutorial file is that it has accompanying joint weights, making it fairly simple to test both skins and vertex attributes with the same file.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@mosra turns out the original file is from the glTF 2 sample repo, licensed under CC0. Do I still mention it in that case? It's not used verbatim, if that makes any difference.

Copy link
Owner

Choose a reason for hiding this comment

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

I don't think that's needed, no.

@pezcode
Copy link
Contributor Author

pezcode commented Jul 3, 2021

There is a known test failure with one glTF file, but the CIs with old Assimp versions skip that. They still report failure with 0 failed tests, is that because of the warning about some tests not containing any checks?

/* Without this setting, Assimp adds bogus skeleton visualization meshes
to files that don't have any meshes. It claims to only do this when
there is animation data, but at least for Collada it always does it. */
importer->SetPropertyBool(AI_CONFIG_IMPORT_NO_SKELETON_MESHES, true);
Copy link
Owner

Choose a reason for hiding this comment

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

Note to self: this should probably go into the "Potential compatibility breakages" section in the changelog.

src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp Outdated Show resolved Hide resolved
src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp Outdated Show resolved Hide resolved
}
Containers::Pointer<ObjectData3D> parent = importer->object3D(0);
CORRADE_COMPARE(parent->children(), {1});
CORRADE_COMPARE(parent->instanceType(), ObjectInstanceType3D::Empty);
Copy link
Owner

Choose a reason for hiding this comment

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

Ummm ... I don't understand why is there such a weird interaction with IMPORT_NO_SKELETON_MESHES?

IIRC the original intent of this test was to verify that collapsing the whole file into a single node works, i.e. testing corner cases of this block of code (and the COLLADA/skeleton-related comment there is probably outdated now?).

I think in this case the expected behavior was that the importer skips the useless extra added root node and returns only its child, but I have no idea why it's no longer being done -- it should be.

The "only a single node" case in the code happens for PLY files for example (or some of the glTF files), but seems like the test doesn't explicitly verify that. I'll add a test case to check that code path.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The comment in the test suggested it was Assimp doing the collapsing internally so I simply adapted the test. But if it tests some importer code then it might need some more scrutiny. I'm not entirely sure what's happening either, but I can do some more debugging.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Mildly related question: what are bone nodes?

/** @todo support for bone nodes */

Copy link
Owner

@mosra mosra Jul 4, 2021

Choose a reason for hiding this comment

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

That's an outdated comment -- nodes associated with a skin. Basically what's provided by SkinData now, I suppose this TODO expected the reference would go the other way around. Remove :)

Assimp doing the collapsing internally so I simply adapted the test.

IIRC Assimp does the collapsing internally if you enable PreTransformVertices, but still adds a silly empty root node with an identity transformation that the importer plugin then attempts to remove.

@mosra
Copy link
Owner

mosra commented Jul 4, 2021

They still report failure with 0 failed tests, is that because of the warning about some tests not containing any checks?

Yes, that's why :)

@mosra mosra added this to the 2021.0a milestone Jul 4, 2021
@mosra mosra added this to TODO in Asset management via automation Jul 4, 2021
@pezcode
Copy link
Contributor Author

pezcode commented Jul 4, 2021

Thanks for taking the time to look at this on a Sunday, very much appreciated 鈽猴笍

@pezcode
Copy link
Contributor Author

pezcode commented Jul 6, 2021

Functionality is all there, I just need to finish a few tests...

Meanwhile, enjoy some animated, skinned Mixamo characters imported from FBX files into Wonderland Engine 馃懐

2021-07-06_00-34-36

@pezcode
Copy link
Contributor Author

pezcode commented Jul 7, 2021

Bigger struggle than anticipated, but everything should be tested now, including:

  • multiple sets of joint weight attributes
  • the maxJointWeights option
  • the mergeSkins option

For the last one I had to do some openState shenanigans because I couldn't for the life of me find a small file with joints shared between skins, or how to make such a file with Blender.

I took another look at Assimp code to understand the AI_CONFIG_IMPORT_NO_SKELETON_MESHES interaction with node collapsing but I'm no smarter than I was before. Is this blocking? I don't think turning AI_CONFIG_IMPORT_NO_SKELETON_MESHES back off again is great, but I also don't understand the implications for other file types too well.

@mosra
Copy link
Owner

mosra commented Jul 7, 2021

Is this blocking?

I'll look into it during the merge.

@mosra mosra marked this pull request as ready for review July 7, 2021 08:32
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.

Your code looks fine. I still need to check it out, build and run locally... and look into that scene collapsing behavior.

The Windows builds fail because of this, which I define for the Windows Assimp builds. So... I guess I should enable export functionality for these binaries? Another option would be to rewrite that test as a glTF file instead of building it up using Assimp's own APIs, I could attempt that.

Containers::Optional<SkinData3D> AssimpImporter::doSkin3D(const UnsignedInt id) {
/* Import either a single mesh skin or all of them together.
Since Assimp gives us no way to enumerate the original skins
and assumes that one mesh = one skins, we give the users
Copy link
Owner

Choose a reason for hiding this comment

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

Suggested change
and assumes that one mesh = one skins, we give the users
and assumes that one mesh = one skin, we give the users

This is the only remaining issue I found by looking at the code :)

@mosra
Copy link
Owner

mosra commented Jul 7, 2021

Wow, goddamit. So the node collapsing worked before exactly because the AI_CONFIG_IMPORT_NO_SKELETON_MESHES was not set, which added one extra mesh to the imported file, which then managed to circumvent this: assimp/assimp#3820

After patching scene.dae to contain a mesh, the test case started passing again, and was testing the else case in doOpenData() (as I hoped/expected). Dumpster fire.

@pezcode
Copy link
Contributor Author

pezcode commented Jul 7, 2021

Another option would be to rewrite that test as a glTF file instead of building it up using Assimp's own APIs, I could attempt that.

That's probably the better option because you don't have to deal with Assimp itself in the tests. I painfully edited an existing Collada file for the multiple weight attributes test, but constructing glTF buffers actually seems easier (and might uncover yet-unknown glTF Assimp bugs... I'm really suspicious I haven't hit any in the basic skin() test 馃憖).

@mosra
Copy link
Owner

mosra commented Jul 7, 2021

I managed to fix the Windows CI issue by using the underlying C++ APIs instead of the C wrappers, not sure why there's a difference. (Of course it's never simple, so I'm now testing a workaround workaround that negates the workaround for version 3.2... and funnily enough both "copy" calls leak memory!).

One last question, about the "ignore dummy joint weights" commit -- there doesn't seem to be any difference in the test output whether the code is there or not, and looking at https://github.com/assimp/assimp/blob/1d33131e902ff3f6b571ee3964c666698a99eb0f/code/AssetLib/glTF2/glTF2Importer.cpp#L1099 it seems this happens only when the glTF looks like this

    "skins": [
        {
            "joints": []
        }
    ],

which is invalid according to the spec. So I wouldn't even bother adding a special case, and if Assimp crashes on such a file it's ... not our problem :)

@pezcode
Copy link
Contributor Author

pezcode commented Jul 7, 2021

One last question, about the "ignore dummy joint weights" commit -- there doesn't seem to be any difference in the test output whether the code is there or not, and looking at https://github.com/assimp/assimp/blob/1d33131e902ff3f6b571ee3964c666698a99eb0f/code/AssetLib/glTF2/glTF2Importer.cpp#L1099 it seems this happens only when the glTF looks like this

    "skins": [
        {
            "joints": []
        }
    ],

which is invalid according to the spec. So I wouldn't even bother adding a special case, and if Assimp crashes on such a file it's ... not our problem :)

I didn't look into it all that deeply, but I ran across it when testing this file exported from Blender:
https://discourse.threejs.org/t/vertex-groups-on-gltf-exported-model/4397

What happens is that the dummy weights lead to overflow in jointCount (no. of weights per vertex) because the code assumes that no two bones affect the same vertex, which isn't 100% correct if we're using the glTF spec as reference:

No joint may have more than one non-zero weight for a given vertex

I suppose the correct (and slow) way to fix this would be to store everything in a map but that completely garbles the joint order (which Assimp sorts by weight, if it has to reduce the bone count in the postprocess step). Ignoring all zero-weights is another option but that has a funny aftertaste to me.

@pezcode
Copy link
Contributor Author

pezcode commented Jul 7, 2021

If I'm reading Assimp's code correctly, the dummy weights are added to all joints in the skin that aren't influencing any vertex in the mesh, which is entirely possible. Should I try to create a small test with a custom file for this?

@mosra
Copy link
Owner

mosra commented Jul 7, 2021

Ah, you're right, I misread the code.

I tried to craft a file by reducing skins.gltf by hand and then adding a few extra nodes not referenced from the mesh but got too impatient, this crap library a massive time sink every time I touch it 馃敟

If you're able to do that, that'd be great. Another thing is the leak inside aiCopyScene(), I don't know what to do about that, probably best would be to hand-craft another file there as well :/

Assimp only imports the last(!!) set of glTF vertex weights
@pezcode
Copy link
Contributor Author

pezcode commented Jul 8, 2021

Sweet Jesus! I think I found the dumbest Assimp bug yet.

I was checking if it correctly imports multiple sets of joint weights (what I originally wanted to do with the file linked earlier, but then forgot because it caused the bug with dummy weights), and not to much suprise, it doesn't.

Now comes the best part: it doesn't even import the first set of weights... The parsing code is so abysmal, they look for JOINT (note the missing trailing S), find it (because JOINTS_0 exists), then try to extract the index by checking against JOINT_i which fails so each set gets index i = 0, overwriting each previous set. What you get is always the last set of joint weights. 馃檮

I added a check that finds non-normalized weights in glTF files and prints a warning (+ accompanying test), but there isn't much else we can do.


Now for better news: adding the other test was fairly simple, so the dummy weight removal has a proper test that fails if you remove the checks. For some reason Github doesn't show the commits in the main PR view, but they're there in the Commits tab. Together with #100 this should hopefully be the victory lap for this PR 馃檹

@mosra
Copy link
Owner

mosra commented Jul 8, 2021

Thanks! Will get back to this hopefully this evening, now I have to work on other things ;)

@mosra
Copy link
Owner

mosra commented Jul 8, 2021

Almost done putting everything together. Thanks to #100 it doesn't leak anymore, but the newly added test for dummy weights had an error in the .bin size (520, should be 620), which triggered an OOB read inside Assimp:

=================================================================
==1125177==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x616000028d88 at pc 0x5581dfff6298 bp 0x7ffd5b2951d0 sp 0x7ffd5b294980
READ of size 512 at 0x616000028d88 thread T0
    #0 0x5581dfff6297 in __interceptor_memcpy.part.0 (/home/mosra/Code/magnum-plugins/build-clang-addresssanitizer/Debug/bin/AssimpImporterTest+0xc0297)
    #1 0x7fd2d67aa39b  (/usr/lib/libassimp.so.5+0x71a39b)
    #2 0x7fd2d67ab3e4  (/usr/lib/libassimp.so.5+0x71b3e4)
    #3 0x7fd2d67abc51  (/usr/lib/libassimp.so.5+0x71bc51)
    #4 0x7fd2d6398eae in Assimp::BaseImporter::ReadFile(Assimp::Importer*, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, Assimp::IOSystem*) (/usr/lib/libassimp.so.5+0x308eae)
    #5 0x7fd2d63ab26c in Assimp::Importer::ReadFile(char const*, unsigned int) (/usr/lib/libassimp.so.5+0x31b26c)

SUMMARY: AddressSanitizer: heap-buffer-overflow (/home/mosra/Code/magnum-plugins/build-clang-addresssanitizer/Debug/bin/AssimpImporterTest+0xc0297) in __interceptor_memcpy.part.0
Shadow bytes around the buggy address:
  0x0c2c7fffd160: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2c7fffd170: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2c7fffd180: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2c7fffd190: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
  0x0c2c7fffd1a0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00
=>0x0c2c7fffd1b0: 00[fa]fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2c7fffd1c0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2c7fffd1d0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2c7fffd1e0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2c7fffd1f0: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
  0x0c2c7fffd200: fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa fa
Shadow byte legend (one shadow byte represents 8 application bytes):
  Addressable:           00
  Partially addressable: 01 02 03 04 05 06 07 
  Heap left redzone:       fa
  Freed heap region:       fd
  Stack left redzone:      f1
  Stack mid redzone:       f2
  Stack right redzone:     f3
  Stack after return:      f5
  Stack use after scope:   f8
  Global redzone:          f9
  Global init order:       f6
  Poisoned by user:        f7
  Container overflow:      fc
  Array cookie:            ac
  Intra object redzone:    bb
  ASan internal:           fe
  Left alloca redzone:     ca
  Right alloca redzone:    cb
  Shadow gap:              cc
==1125177==ABORTING

TOP NOTCH error recovery right there. Very secure. Much hardening. I'll just pretend I didn't see anything. 馃敟

@mosra
Copy link
Owner

mosra commented Jul 8, 2021

I squashed the major part into a single commit, with the fun and exciting new bug/misfeature discoveries kept as a separate commits, and merged as faa16d4...d62d58c.

Thanks a lot for having the perserverance to work on this 馃槄

@mosra mosra closed this Jul 8, 2021
Asset management automation moved this from TODO to Done Jul 8, 2021
@pezcode
Copy link
Contributor Author

pezcode commented Jul 8, 2021

Thanks for cleaning up the debris, I didn't realize there was so much work left to merge 馃檹

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