AssimpImporter: clean up STL types + update tests to 5.2.5 #130

pezcode · 2022-09-10T21:27:57Z

5.2.5 just got released and finally fixes the issues with glTF and multiple sets of vertex joint weights. I just wanted to update the tests but then got caught in a cleanup session 👀

Some of these changes may not be desired so I tried to keep them as granular as possible, so they can be reverted/dropped individually.

Mostly, this removes the majority of std types that have equivalents in Corrade::Containers, except for unordered_map and a few cases of std::string required by the assimp or importer API.

Tests got adapted to:

5.2.3 not outright failing to import glTFs with multiple skinning attribute sets anymore
XFAIL 5.2.4 temporarily breaking skinning attributes for all file types
5.2.5 correctly importing all(!) skinning attributes, for all file types

mosra

Ohh, this is a nice surprise cleanup. Thank you!

src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp

mosra · 2022-09-13T10:28:52Z

src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp

    bool mergeSkins = false;
    /* For each mesh, map from mesh-relative bones to merged bone list */
-    std::vector<std::vector<UnsignedInt>> boneMap;
-    std::vector<const aiBone*> mergedBones;
+    Containers::Array<Containers::Array<UnsignedInt>> boneMap;


Because the boneMap is filled sequentially (if I see correctly), I was wondering if we could avoid the nested allocations by having just one array and an offset array to index into that? If it's not too much extra testing effort.

Good idea 👍 I'll check if the current tests cover this well enough, otherwise I'll extend them a bit

I extended the tests a bit and I feel like I've hit a bug either with assimp or the bone merging logic in our code. Not too much time to investigate before I leave on vacation for ~2 weeks, but I'll pick this back up afterwards.

In that case, can you just push/rebase the remaining changes except this one? This particular optimization can wait two weeks, but I now have 5.2.5 on my system (and I expect Homebrew will soon as well) and it'd be nice to have this merged to have tests passing again.

Thank you!

I pushed everything except this change. https://github.com/pezcode/magnum-plugins/tree/assimp-skin-merge-better-test has the change + extended tests that fail, if you care to look at it.

Awesome, thanks :) Looked at the commit, but didn't see anything suspicious. Strange library, strange bugs.

Anyway, enjoy your vacation! 🌴

src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp

mosra · 2022-09-17T20:26:16Z

Oh, and the stupid ARM test crash is fixed in c0337ef. I was neglecting it because I expected some alignment issues deep inside stb_image_resize, but turns out it wasn't that at all. If you rebase, the build will pass and a Codecov report gets triggered properly.

codecov · 2022-09-18T03:38:55Z

Codecov Report

Merging #130 (4b08c3f) into master (477cace) will decrease coverage by 0.00%.
The diff coverage is 98.82%.

@@            Coverage Diff             @@
##           master     #130      +/-   ##
==========================================
- Coverage   96.76%   96.76%   -0.01%     
==========================================
  Files         130      130              
  Lines       13665    13687      +22     
==========================================
+ Hits        13223    13244      +21     
- Misses        442      443       +1

Impacted Files	Coverage Δ
...rc/MagnumPlugins/AssimpImporter/AssimpImporter.cpp	`90.43% <98.82%> (+0.10%)`	⬆️

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

This whole ordeal was apparently already solved elsewhere

Since we always report all custom mesh attributes when a file is open regardless of whether any mesh uses them, it makes no sense to store them per scene

To make up for Array not overallocating when growing (unlike vector) and one instance of unordered_map without memory reservation

It's completely broken, reporting wrong attribute count or just wrong attribute data. For some reason it works with glTF files but that might just be a weird interaction with the other brokenness there. It got fixed right away in 5.2.5, and the importer can't really detect or handle it anyway, so no change there.

…weights from glTF Adapt the check and warning to only run on affected versions, and limit the XFAIL as well

…dling And always report them, regardless of whether a file is opened

Useless attempt to micro-optimize memory when assimp just eats it by the bucket anyway

@mosra

@mosra made some nice changes to make this possible

…erstand

pezcode · 2022-09-19T22:33:52Z

Some Linux CIs are failing because of old GCC not liking my array initialization syntax. I feel like I've fixed this before, but before I waste CI minutes on this, do you know what's wrong there?

constexpr Containers::StringView objectNames[]{
    "Mesh_1"_s, "Mesh_2"_s, "Plane"_s
};
constexpr Containers::StringView jointNames[][2]{
    {"Node_1"_s, "Node_2"_s},
    {"Node_3"_s, "Node_4"_s}
};

AssimpImporterTest.cpp:1382:5: error: array must be initialized with a brace-enclosed initializer

mosra · 2022-09-19T22:36:33Z

Try with just const and not constexpr? That's what usually makes it go crazy.

Or maybe it doesn't like the implicit size? No idea.

mosra · 2022-09-20T14:03:36Z

I squashed in some of the fixup commits and merged the commit series as 477cace...549486d. Now the only remaining test failure for me locally is glslang which I have to look at, Assimp is sparkling clean ✨

Thank you!

mosra added this to the 2022.0a milestone Sep 13, 2022

mosra reviewed Sep 13, 2022

View reviewed changes

mosra reviewed Sep 17, 2022

View reviewed changes

src/MagnumPlugins/AssimpImporter/AssimpImporter.cpp Outdated Show resolved Hide resolved

pezcode added 19 commits September 20, 2022 00:10

AssimpImporter: replace std::string with String[View] where possible

9193d26

AssimpImporter: use String[View]Converter<aiString>

1403aa5

AssimpImporter: remove duplicate code

dae09cb

This whole ordeal was apparently already solved elsewhere

AssimpImporter: don't store custom mesh attributes in containers

d76c544

Since we always report all custom mesh attributes when a file is open regardless of whether any mesh uses them, it makes no sense to store them per scene

AssimpImporter: add vanished newline

dde5b6d

AssimpImporter: replace std::vector with Array

734e09e

AssimpImporter: optimistically reserve memory for containers

734d2e8

To make up for Array not overallocating when growing (unlike vector) and one instance of unordered_map without memory reservation

AssimpImporter: replace std::pair and std::tuple with Pair and Triple

edc8526

AssimpImporter: version 5.2.5 finally imports multiple sets of joint …

4eb92b9

…weights from glTF Adapt the check and warning to only run on affected versions, and limit the XFAIL as well

AssimpImporter: remove remaining arrays for custom mesh attribute han…

e03ed17

…dling And always report them, regardless of whether a file is opened

AssimpImporter: avoid String copies for unordered_map lookup

51565c4

AssimpImporter: these read-only pointers can be const

6061681

AssimpImporter: don't shrink internal arrays

ffd4739

Useless attempt to micro-optimize memory when assimp just eats it by the bucket anyway

AssimpImporter: make fixed-sized array creation more explicit

5a15c65

AssimpImporter: don't need this cast anymore

39ac1f2

@mosra made some nice changes to make this possible

AssimpImporter: also replace std::string in tests where possible

b505531

AssimpImporter: deduplicate some test code

622f149

AssimpImporter: make the skinMerge() test a bit less confusing to und…

d5385bc

…erstand

pezcode force-pushed the assimp-cleanup-5_2_5 branch 2 times, most recently from 00a7ab1 to d5385bc Compare September 19, 2022 22:14

AssimpImporter: help GCC 4 out a bit

4b08c3f

mosra added the changelog mention added label Sep 20, 2022

mosra closed this Sep 20, 2022

AssimpImporter: clean up STL types + update tests to 5.2.5 #130

AssimpImporter: clean up STL types + update tests to 5.2.5 #130

Uh oh!

Conversation

pezcode commented Sep 10, 2022

Uh oh!

mosra left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mosra Sep 13, 2022

Choose a reason for hiding this comment

Uh oh!

pezcode Sep 17, 2022

Choose a reason for hiding this comment

Uh oh!

pezcode Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

mosra Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pezcode Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

mosra Sep 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

mosra commented Sep 17, 2022

Uh oh!

codecov bot commented Sep 18, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

pezcode commented Sep 19, 2022

Uh oh!

mosra commented Sep 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mosra commented Sep 20, 2022

Uh oh!

Uh oh!

mosra Sep 19, 2022 •

edited

Loading

codecov bot commented Sep 18, 2022 •

edited

Loading

mosra commented Sep 19, 2022 •

edited

Loading