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
Deprecating SOFA_FLOAT ? #495
Comments
Even shorter ;-) int ImplicitSurfaceMappingClass = core::RegisterObject("Compute an iso-surface from a set of particles")
.add< ImplicitSurfaceMapping< Vec3Types, Vec3Types > >()
.add< ImplicitSurfaceMapping< Vec3Types, ExtVec3fTypes > >();
template class SOFA_SOFAIMPLICITFIELD_API ImplicitSurfaceMapping< Vec3Types, Vec3Types >;
template class SOFA_SOFAIMPLICITFIELD_API ImplicitSurfaceMapping< Vec3Types, ExtVec3fTypes >; (the point being to keep |
To me, the real question is do we want to keep float+double at the same time? A simple typedef for SReal would be so easy... Otherwise the graal would rather be to keep only float everywhere, and having just a few double where it is really necessary for numerical precision. |
@matthieu-nesme that's precisely what I'm hinting to: having a single typedef for |
So if I understand you maxime the idea is to have: And at the same time dropping the SOFA_FLOAT/SOFA_DOUBLE if def that are there to, as mathieu said, mix in the same scenes component in Vec3f & Vec3d. |
@damienmarchal I would drop When |
It is easy enough to let the code as now, and compiling only in double, or to remove the extra template instantiations to keep only the one based on SReal as suggested by Max with a possibility to let the other ones that are really required. I repeat the ultimate goal would be to work only with single float, they are more efficient (using less bandwidth, serialization will handle more computations at the same time...) and they are numerically enough in 99.999%. cases We are using double because we never tried to tackle the numerical challenges. |
That is exactly the case right now! Vec3fTypes/Vec3dTypes/Vec3Types - Rigid3fTypes/Rigid3dTypes/Rigid3Types |
But do we actually? Did we ever use this feature in the past, apart from mapping external vectors (as I said, for Cuda, OpenGL and the likes)? |
Who is we? I think the problem was: our code (e.g. CGSolver) is not numerical robust in single precision so we need to use double precision implementations, but some part of the code is (was) single precision only (e.g. Cuda forcefields). Once again the real point is why using double precision when single float should be enough? |
I just did a quick test in a simple scene (horizontal beam with downward force at one end), and the Newton static solver does not converge with the TetrahedronFEMForceField in single precision, using a CGSolver in double precision. The problem may be with the addDforce function used by CG, but it would need some more investigating. So basically, we can't simply switch to single precision without checking everything that could be sensitive to numerical errors (but I agree that it would be better if we could use mostly single precision). |
Thanks all, Matthieu pointed that "The feature was used ; removing it has already been discussed many times and refused" but having something discussed or used (a long time ago) shouldn't be an argument not to "refresh" the discussion (in the last 10 years things have changed in term of hardware as well as we have now more insight on the intrusiveness of the selected approach). Having the featured re-discussed again is probably an indication that something is somehow problematic :) As I said before there is fact two different issues...one is about mixing in the same scene object/solver with different floating point representation while the other is about having SReal mapped to float or double by default. The current approach, despite it does not match individual preferences, makes a relative consensus: To me the real problem is not there, it is in how we have implemented the mixing in the same scene object/solver with different floating point representation. Our current implement is based on instantiating templates in the factory for each types so that when the types matches the objects can work together without conversion cost. So we are trading run-time speed for a development cost. But the use case is very rare that we may wonder if other approach shouldn't be preferable (eg: a conversion layer instead) ? |
Maybe we should add in the object factory a way to detect the use of Vec3f and warn user that it needs to update its scene to use Vec3 instead. So we can start simplifying the code base ? |
Full agreement ! |
The first PR to remove SOFA_FLOAT on the sofa code base is on its way. This will not change the world but the resulting code look so much better |
Hi all, Defrost is happy to announce that we have dropped the SOFA_FLOAT from our SoftRobots & SoftRobots.Inverse plugins. About PR #853, If you think this is not the way to go...please tell it now because otherwise it will happen. |
The PR #853 is now compiling on some system...and there is already a lot of scene working. What do you think we should do for the user layer:
|
Done. |
From my perspective maintaining three code base (SOFA_FLOAT, SOFA_DOUBLE, SOFA_FLOAT+SOFA_DOUBLE) and insuring everything is ok on the three code path, costs a lot (amount of code to write, maintain, read (all those #ifdef all around) and compilation time.
So I wonder if it would be acceptable to drop the SOFA_FLOAT part of sofa.
PS: I actually also have the same question for the PS3 #ifdef.
Example of code "readability" improvement:
Would become
Suggested labels:
The text was updated successfully, but these errors were encountered: