Update assimp
- Please check if the PR fulfills these requirements
- [x] The commit message follows our guidelines
- [ ] Tests for the changes have been added (for bug fixes / features)
- [ ] Docs have been added / updated (for bug fixes / features)
- What kind of change does this PR introduce? (Bug fix, feature, docs update, ...) Update assimp external TODO:
- [ ] tests if everything is loaded correctly
- [ ] specifically animation
- [ ] choose a commit as reference, right now it's master, but it's a bad idea since it can changes anytime, and introduce unexpected behavior
This update solves compilation problems under windows, and also when you have Release Application with Debug Lib (or the other way round).
-
What is the current behavior? (You can also link to an open issue here) Compilation fails on windows (with assimp, in CI), compilation fails if your application is compiled in Debug with Radium lib in Release.
-
What is the new behavior (if this is a feature change)? Everything compiles fine.
-
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?) No change is expected, but maybe some post file loading process will be needed.
Codecov Report
Merging #647 (a5a77d9) into master (4dcd0ef) will not change coverage. The diff coverage is
n/a.
@@ Coverage Diff @@
## master #647 +/- ##
=======================================
Coverage 16.68% 16.68%
=======================================
Files 308 308
Lines 14572 14572
=======================================
Hits 2431 2431
Misses 12141 12141
Continue to review full report at Codecov.
Legend - Click here to learn more
Δ = absolute <relative> (impact),ø = not affected,? = missing dataPowered by Codecov. Last update 4dcd0ef...a5a77d9. Read the comment docs.
Can you split this PR in two: update assimp on one side, and fix testing on windows on the other side ?
@MathiasPaulin did you have a specific commit in mind for this one ?
We have a specific tag for all our externals, I would prefer we give the sha of the current master instead of referring to the branch directly.
@MathiasPaulin did you have a specific commit in mind for this one ? Not really. Any one that allows to build and use assimp reliably is welcome. I need to test the current proposal.
I think we could squash merge at the end, so I don't need to rewrite history.
This assimp update breaks animation loading in Radium. Some scene does not load due to incomplete scene detection (all the scene is below an animated node), others loads but with wrong animation time, speed and matrices ... Those issues must be investigated and solved before upgrading assimp.
After investigations, there are problems with this assimp update :
- from the assimp side. Issue https://github.com/assimp/assimp/issues/3584 refer to collada import problem on animation but even fixing this as indicated in the issue does not solve loading problems. There are issues about loading meshes attached to bones in the assimp collada loader (meshes not loaded, wrong face number, ...).
- from the Radium side. Some problems arise that concerns the animation time and duration.
The first thing to do is to fix the assimp side problems (hard to understand where they come from), then the Radium-side one before accepting this PR.
The first thing to do is to fix the assimp side problems (hard to understand where they come from), then the Radium-side one before accepting this PR.
Assimp issues related to this PR : https://github.com/assimp/assimp/issues/3584 https://github.com/assimp/assimp/issues/3576 https://github.com/assimp/assimp/issues/3477 https://github.com/assimp/assimp/issues/3468
See https://github.com/assimp/assimp/pull/3619, is fixed on the assimp-master,
Thanks @kimkulling for updating us on this. We will come back to you if we find other problems.
With assimp master from 05-25-2021 : Radium compiles fine but there is huge problems when loading animations :
- from dae files : no binding from skeleton to meshes,
- from fbx files : wrong matrices and time settings.
The first thing to do is to verify our transformations from assimp data to Radium data is correct, if so, understand what is incorrect on the assimp side and open a issue on assimp about that. But note that as the issues assimp/assimp#3576 assimp/assimp#3477 assimp/assimp#3468 that might solve part of the problem are still opened, perhaps our problem will be fixed soon.