search

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
934 stars 312 forks source link

Minor numerical implementation difference between float/double on BilateralInteractionConstraint #45

Closed damienmarchal closed 8 years ago

damienmarchal commented 8 years ago

I,

I'm factoring the code that is in double/float template specialization using the design pattern used in the Image plugin (an external class Specialization which is friend to the class to specialize).

The code in:

BilateralInteractionConstraint<Rigid3fTypes>::getConstraintResolution(...)
BilateralInteractionConstraint<Rigid3dTypes>::getConstraintResolution(...)

is different. Float is using 

  temp->tolerance = 0.0001;

Double is using:

  temp->tolerance = 0.01;

Can someone check if this is expected or if this a kind of "someone change the value in one place and forgot the other".

DM.

hugtalbot commented 8 years ago

For me, it makes no sense to have two different values based on the template in BilateralInteractionConstraint. But it would be nice to have some feedback from real expert on Constraints.

JeremieA commented 8 years ago

Hello,

A simple blame shows the 2 commits where these values where last set: https://github.com/sofa-framework/sofa/commit/bb99399cba47a95301025531577c2e3ac3f728ab https://github.com/sofa-framework/sofa/commit/8b294f75884a0216ce26f5457b8689da57dd081a

They were done for a PhD work, tuning for specific cases but without giving real reasons in the log. So this is not something that was really meant to be preserved like that, indeed it does not make sense that the value is much higher for double that floats. But the actual refactoring that is needed is to expose this tolerance as a Data instead of hiding it within the code itself, so that it can be tuning within the simulations that require it instead of requiring a patch within Sofa. Ideally the value in the Data should preserve the current behaviour in order not to break existing scenes, possibly with the isRequired flag set such that others will know that they should set the value specifically instead of relying on the previously hidden and inconsistent default.

Jeremie.

hugtalbot commented 8 years ago

Agreed. Thank you Jeremie for your input.

damienmarchal commented 8 years ago

Thank Hugo, thanks Jeremie for the inputs.

I fixed that in the defrost branch. I choose a different approach to one suggested by Jeremie to maintain backward compatibilty. If you want to have a look it is there: https://github.com/sofa-framework/sofa/commit/2ccfd85df740afeb084f7fdab2ded74c13018561

We should merge our weeks difference from defrost to master tomorrow (if everything goes smoothly).

DM.