[SofaDenseSolver] Fix compilation with SOFA_FLOATING_POINT_TYPE as float

[FabienPean]

issue sofa-framework/sofa#2 Fix compilation when option SOFA_FLOATING_POINT_TYPE in CMake selected as float

The problem arising when trying to compile with float was due to missing declaration from newmat external library. The library has been modified for sofa to use a #define SOFA_FLOAT in include.h However the SOFA_FLOAT was never defined for the extlib since the current procedure is to set CMake variable SOFA_FLOAT and SOFA_DOUBLE to 1. This was done anyway AFTER processing the cmake of the extlibs.

Therefore, this pr moves the option for floating point type in sofa in the root CMakeLists and adds the #define for the newmat library to actually create the float type functions.

---

This PR:

• [x] builds with SUCCESS for all platforms on the CI.
• [x] does not generate new warnings nor unit test failures.
• [x] does not break existing scenes.
• [x] does not break API compatibility.
• [x] has been reviewed and agreed to be transitional.
• [x] is more than 1 week old (or has fast-merge label).
• [x] reports important changes in Changelog.

Reviewers will merge only if all these checks are true.

[sofabot]

Thank you for your pull request! Someone will soon check it and start the builds.

[damienmarchal]

[ci-build]

[hugtalbot]

Thank you @FabienPean for this nice PR!

[guparan]

Dashboard details are here: https://www.sofa-framework.org/dash/index.php?branch=pr%2Ffix%2Fissue%232

[guparan]

Hi @FabienPean, Any idea why RigidLinearDeformationMappings_test is failing on Windows ? See windows7_VS-2015_amd64_origin_options tests report.

EDIT: This is certainly nothing related to your PR. It is also failing for sofa-framework/sofa#261.

EDIT 2: The problem is known, see issue sofa-framework/Flexible#4.

[FabienPean]

I removed the sofa define from newmat/include.h and corrected the CMakeFiles. Now it activates float only when "float" is explicitly selected in root CMakeFiles

[FabienPean]

Ok I did not expect this problem. I understand now better what Matthieu meant.

When I removed SOFA_DOUBLE from newmat, the target_compile_definitions are only visible within newmat project with the PRIVATE attribute, which was causing problem when the newmat.h was used included in SofaDenseSolver, not having the compile definitions set. It works if the compile definition of newmat are PUBLIC, but I do not know the side effects it may cause.

[damienmarchal]

Thanks Fabian for the quick update of the PR... [ci-build]

[guparan]

Did you try compiling with SOFA_FLOATING_POINT_TYPE = float? It is still failing on my side (Windows + VS-2015).

The Dashboard is OK but compiling with SOFA_FLOATING_POINT_TYPE = both.

[FabienPean]

I did not manage to compile it fully. SofaDenseSolver is now ok but not the remaining of Sofa. I started to fix some errors, however it seems that this feature was not actively maintained recently. With latest commit it should compile but it gives weird runtime visualization. (Windows VS2015 also)

If the feature is a must-have, maybe the CI should test with float-only and double-only instead of "both" ?

[guparan]

Thank you for your fixes @FabienPean !

Two remarks about consistency:

• Sofa_test default template is SReal. I would go for Sofa_tests<> inheritance. Unless there is a specific reason for inheriting from Sofa_tests<SReal> ?
• #ifndef SOFA_FLOAT block usually comes before #ifndef SOFA_DOUBLE block.

I can push these tiny corrections if you're ok 😉

[FabienPean]

Hi Guillaume, sure feel free ! :)

[damienmarchal]

The 'leaking' snake looks great :)

[hugtalbot]

is it ready to merge ? @guparan do you plan to push your "tiny corrections" ?

[guparan]

Here they are !

[damienmarchal]

It is unclear to me if it is ready. Isn't the screenshot from @FabienPean showing something is broken ?

[guparan]

At least the compilation is fixed so I'd say this PR does its job. I had the same visualization glitches on my side though. Maybe a quick review to ensure we did not create the problem - @fredroy ?

[damienmarchal]

Rebuild from a recent master... [ci-build]

[FabienPean]

My bad it seems I was quite asleep when I tried to fix the DilateEngine.cpp

[damienmarchal]

So it seems there is some troubles here in compiling this PR. Anyone can check ?

[FabienPean]

The problem comes when using 'both' it seems. For one part of the build failure, TriangleOctree / TriangleOctreeRoot must be instantiated with both precision instead of currently using the default type SReal only.

[fredroy]

I built it on VS2015, and got the same behavior as on the pictures. It appears to be a bug with SparseGridRamification (in caduceus.scn) If you replace it with SparseGridTopology, everything appears fine (and behaves fine)

[damienmarchal]

Many thanks @FabienPean for this PR as it seems quite complex to fix everything. [ci-build]

[FabienPean]

Ok so, hopefully the end of this PR. I reverted some commit and double/both should be back to normal. In the case of float only the DilateEngine is not available and its test deactivated. So basically it should compile fine, but some components would be limited. I quickly tried to fix it but I encountered some difficulties. I would prefer open a new issue regarding float compliance of remaining components.

To sum up, it should be back to the original goal: fix compilation, not necessarily the components with type limitation

[damienmarchal]

The CI disagree with you :)

[FabienPean]

I am on Windows and did not try on Linux, so my hopes still hold as long as that yellow dot does not become a cross for the windows VS build :D

The error on the linux build seem trivial fortunately. However it stopped at the first error... Would it be possible for the build system not to stop at this first error and try compiling the remaining ? Since I cannot test on every platform/os that would be valuable to have all potential error at once.

[matthieu-nesme]

Regarding the visualization problems when compiling with float only: keep in mind that double precision can be necessary in specific situations, so be careful when replacing some hard-coded double with SReal. Some double must remain double and I guess some SReal should become double also in the actual code. On platforms where double precision does not exist, tricky numerical solutions have to be employed.

[guparan]

I rebased your branch to clean it a bit (removing all merges and reverts). Let's see how it builds... https://www.sofa-framework.org/dash/index.php?branch=pr%2Ffix%2Fissue%232

[guparan]

[ci-build]

[guparan]

Are we good to go with this PR? @matthieu-nesme, do you see a specific line where a SReal should remain a double?

[matthieu-nesme]

There is no easy answer to that, numerical limits are really complex. But obviously, some double are mandatory somewhere in the components that map the snake model ^^

[damienmarchal]

[ci-build]