Radium-Engine icon indicating copy to clipboard operation
Radium-Engine copied to clipboard

Update assimp

Open dlyr opened this issue 5 years ago • 10 comments

  • 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.

dlyr avatar Jan 11 '21 07:01 dlyr

Codecov Report

Merging #647 (a5a77d9) into master (4dcd0ef) will not change coverage. The diff coverage is n/a.

Impacted file tree graph

@@           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 data Powered by Codecov. Last update 4dcd0ef...a5a77d9. Read the comment docs.

codecov[bot] avatar Jan 11 '21 07:01 codecov[bot]

Can you split this PR in two: update assimp on one side, and fix testing on windows on the other side ?

nmellado avatar Jan 11 '21 10:01 nmellado

@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.

nmellado avatar Jan 11 '21 11:01 nmellado

@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.

MathiasPaulin avatar Jan 11 '21 12:01 MathiasPaulin

I think we could squash merge at the end, so I don't need to rewrite history.

dlyr avatar Jan 11 '21 14:01 dlyr

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.

MathiasPaulin avatar Jan 19 '21 08:01 MathiasPaulin

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

MathiasPaulin avatar Jan 19 '21 21:01 MathiasPaulin

See https://github.com/assimp/assimp/pull/3619, is fixed on the assimp-master,

kimkulling avatar Feb 08 '21 22:02 kimkulling

Thanks @kimkulling for updating us on this. We will come back to you if we find other problems.

nmellado avatar Feb 09 '21 06:02 nmellado

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.

MathiasPaulin avatar May 25 '21 08:05 MathiasPaulin