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: Simplify skinMerge() test #100



Copy link

@Squareys Squareys commented Jul 7, 2021

This PR is against next!

Hi @mosra & @pezcode !

Thank you both for the amazing work on #99, here's a small contribution with the hand-crafted gltf file replacing the leaky scene copy.


pezcode and others added 6 commits July 7, 2021 17:47
No idea why the C wrappers are not there but the underlying C++
implementation is. But it's assimp we're talking about here, any sanity
assumption go outta the window.

AND OF COURSE things are never simple with this pile of hacks, so we
still have to use the C wrappers for versions < 4.0. It also leaks
some allocated memory inside that CopyScene, so VERY WONDERFUL, YES.
Signed-off-by: Squareys <>
@Squareys Squareys force-pushed the assimp-skinning-merge-skins-test branch from 49db760 to 70ab59b Compare July 7, 2021 22:58
Copy link

codecov bot commented Jul 7, 2021

Codecov Report

Merging #100 (70ab59b) into next (90eb7f6) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##             next     #100   +/-   ##
  Coverage   94.69%   94.69%           
  Files         101      101           
  Lines        7628     7628           
  Hits         7223     7223           
  Misses        405      405           

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 90eb7f6...70ab59b. Read the comment docs.

@mosra mosra added this to the 2021.0a milestone Jul 8, 2021
@mosra mosra added this to TODO in Asset management via automation Jul 8, 2021
Unfortunately, <assimp/SceneCombiner.h> is available only since 4.0, so
on the ancient 3.2 use the C API (and assume nobody needs the insane corner case of running the test against Assimp 3.2 that has export APIs
disabled). */
Copy link

Choose a reason for hiding this comment

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

The #include block related to this should get cleaned up as well. I'll do it during merge.

Otherwise looks great, thanks a lot! 👍

Copy link

mosra commented Jul 8, 2021

I went ahead and rewrote also the Assimp-specific matrix operations and squashed the whole thing into 9bf2b2b. Thanks a lot!

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

Successfully merging this pull request may close these issues.

None yet

3 participants