-
Notifications
You must be signed in to change notification settings - Fork 311
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
[SofaCore] FIX: enable ExtVecXf mappings with double floating type #827
[SofaCore] FIX: enable ExtVecXf mappings with double floating type #827
Conversation
I agree that maintaining those ifdef are quite cumbersome and there are probably easier ways to manage this. Just a small remark, while arithmetic operations on double are probably faster than float (well, at least on most of our hardware), using float might minimize cache misses and might, with the help of compiler optimisations, produce faster execution times on memory intensive application (like SOFA) . I think we should test this before removing floats from everywhere. If we got different results, maybe we could set a default floating type for the entire Sofa, and allow modules to change this default value for their components. |
IMO the complexity price we are paying right know in term of maintainability, code uglyness, bugs and weird issues it far bigger than the "un-measure" performance gain we expect. An our engineering time should be more useful in fixing all the crash/segfaul or performance issues because of n^2 algorithm (as in) than trying to get a totally minor (if any) performance increase because of switching from double to float. So I recommand to only use Vec3Type in sofa instead of having both Vec3dType / Vec3fType, it will save thousand of man-month to invest more intelligent things. As said by @jnbrunet, using Vec3Type allows to switch from float/double. |
Thanks for your feedback @jnbrunet @damienmarchal . Meanwhile, this PR is ready, and I suggest reviewing it as a fast-merge since it's not much and fixes a compile-time issue for quite a few people. |
e69b3b7
to
7a42001
Compare
// this is needed even if SOFA_DOUBLE only is compiled | ||
extern template class SOFA_CORE_API Mapping< sofa::defaulttype::Vec2dTypes, sofa::defaulttype::ExtVec2fTypes >; | ||
extern template class SOFA_CORE_API Mapping< sofa::defaulttype::Vec3dTypes, sofa::defaulttype::ExtVec3fTypes >; | ||
extern template class SOFA_CORE_API Mapping< sofa::defaulttype::Vec6dTypes, sofa::defaulttype::ExtVec3fTypes >; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If SOFA_FLOATING_POINT_TYPE=double, then SOFA_WITH_DOUBLE is defined so line 341 is activated.
Why this block?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this block should be just before the closing #endif //SOFA_WITH_FLOAT
line 436 and the similar block line 341 should be deleted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, that was a duplicated bloc that I didn't remove after moving it in SOFA_WITH_DOUBLE:
VecXd -> ExtVecXf are required both when compiling in SOFA_WITH_DOUBLE and in SOFA_WITH_DOUBLE + SOFA_WITH_FLOAT. So technically, they could either be in the SOFA_WITH_DOUBLE bloc or in the combination of both, it would be the same.
Stupid logic. can't wait to see it dissapear...
Compiling sofa with SOFA_FLOATING_POINT_TYPE=double (no float) leads to a series of compile errors in Mapping and MultiMapping
I believe the bug has been introduced in PR #738
The reason for this crash is that some Mappings double->float are necessary for some applications (usually for visual models, CUDA, etc.).
Thus ExtVecXfTypes are always compiled, even when #ifndef SOFA_FLOAT
This PR fixes the bug, but also raises an important underlying point:
Having ifndefs everywhere in the code is very intrusive, source of errors, and most importantly, prevents having a full code coverage in the CI in a single build. Floats are slower, and much less precise than Doubles. We do not support XBox / PS3 code, thus we have one less reason to keep this code smell in the core of sofa.
If the only reason we still need floats is for Cuda compatibility and other visual rendering stuff, shouldn't the conversions be done in those components / plugins, instead of having this FLOAT things affecting the whole codebase of SOFA?
I dream of a sofa without all these ifdefs lying around polluting the code.
@courtecuisse @fredroy , you worked the most on CUDA stuff and visual rendering respectively, do you have an opinion on the subject?
This PR:
Reviewers will merge only if all these checks are true.