sofa-framework / sofa

Real-time multi-physics simulation with an emphasis on medical simulation.
https://www.sofa-framework.org
GNU Lesser General Public License v2.1
871 stars 297 forks source link

[SofaKernel] FIX Mat::transpose() and Mat::invert() #317

Closed marques-bruno closed 6 years ago

marques-bruno commented 6 years ago

when passing "this" as argument, forcing object copy to avoid unexpected results Fixes #280


This PR:

Reviewers will merge only if all these checks are true.

marques-bruno commented 6 years ago

Not a big fan of the reinterpret_cast in the transpose() method, but it's the only way to check wether or not a copy is needed...

damienmarchal commented 6 years ago

[ci-build]

hugtalbot commented 6 years ago

if you agree with this fix @fjourdes and @maxime-tournier , we will merge this as soon as it builds

marques-bruno commented 6 years ago

@maxime-tournier I believe the last commits takes your input into consideration (if I didn't miss anything) Looking forward your feedback

guparan commented 6 years ago

Any idea why building acd79fe caused more than 20 new unit test failures?

damienmarchal commented 6 years ago

[ci-build]

damienmarchal commented 6 years ago

[ci-build]

marques-bruno commented 6 years ago

PR ready?

hugtalbot commented 6 years ago

Hey @lagarkane There's still 22 new failures of the tests. Is that normal? Looks your commit : 092b19f did it

guparan commented 6 years ago

The 20 new unit tests failures are still there, certainly caused by 092b19fd8e922d155a459de09f002f1815ce9a16.

marques-bruno commented 6 years ago

I take a look

hugtalbot commented 6 years ago

[ci-build]

marques-bruno commented 6 years ago

Hum... I messed up somehow in my previous commit. Did a pull -r and everyting went to hell for some reason... I have 689 files with changes in the PR, which is quite aweful for a small modification in 2 files... How could I fix this? :/ My apologies

marques-bruno commented 6 years ago

Hi dear reviewers, I'm waiting for the ci-build, but I believe that the problems are now solved, and the fix now comes with a few unit tests =) @matthieu-nesme I also restored the static_assert that I previously removed so the code is not only better. Any other suggestion?

hugtalbot commented 6 years ago

@matthieu-nesme is it ok for you? we would like to merge the PR today if possible

guparan commented 6 years ago

Hi @lagarkane, is this finally ready? :-)

marques-bruno commented 6 years ago

I believe it is now! @matthieu-nesme , everything ok?

damienmarchal commented 6 years ago

[ci-build] (to sync with the latest merge of GIL.