Default Rayleigh parameters in ODE solvers non-null

[hugtalbot]

Hi,

I am wondering if there is a reason why Data related to Rayleigh damping and mass in ODE solvers (e.g. see EulerImplicitSolver) are not zero by default. Couldn't it be very confusing for new users ?

Thanks for your feedback

---

Suggested labels:

[fjourdes]

It should be zero by default in my opinion. There is also a little confusion because there are currently two ways to specify the rayleigh damping parameters. If you use EulerImplicitSolver rayleigh damping is specified at the solver level. However both the ForceField and Mass API in SofaCore also implements rayleigh damping... provided you use kfactorIncludingRayleighDamping instead of kfactor when assembling the stiffness and the mass. This changed was introduced quite a long time ago, because in the Compliant plugin, the solvers for time integration do not implement global Rayleigh damping, but a more "local" one, delegated to each of the Mass and Forcefield components. So short answer is : yes should be zero. But it could break a lot of scenes that extensively rely on the default damping values to actually work... which mostly happens when you do not pay attention about the unit system you want to use in your scene ( e.g meters and kilograms versus millimetres and grams )

I would also like to have the opinion of someone knowledgeable in that particular area. Rayleigh stiffness factor, contrary to Rayleigh mass factor, is not unit less. It is s^-1 as far as I can remember. Would it be better to specify it as unit less factor instead (and then have it internally divided by the current time step value), so that the value has a sort of consistent meaning regardless of the time step chosen ?

[hugtalbot]

Hi Francois, thank you for the feedback. Hopefully for the common good, this non-zero default value will disappear! I did not know the historical explanation of the ForceField/Mass API.

On my opinion, no matter how many scenes will be broken: hidden numerical damping is worse than anything else.

Finally about making them clearly time dependent, I don't really see the point but whatever, this can be done in a second stage.

[fjourdes]

For the last point about the units, it is just that if you are developping a scene where you indeed add rayleigh stiffness, I was wondering if that would be beneficial to have a unit less value in case you want to compare different executions with slightly different time step size, without having to alter significantly the rayleigh stiffness value. Which you would have to do if you want to keep roughly the same dynamics.

Maybe it is just me, but I was wondering why specifically this constant is not dimensionless like the other damping ratio quantities... which can be a bit disturbing if you play out a bit with the time step size.

[fjourdes]

As a final note, even though I do not use it ( I still use global rayleigh damping when I need some) I believe that having rayleigh damping set at the component level is a better option.

[EulalieCoevoet]

Hi, I think it would be nice to have a warning about these changes, when running our scenes.

[maxime-tournier]

@fjourdes Most definitely agreed! Rayleigh damping in the solver is mostly a (convenient) hack for ensuring stability, it has little to do with actual mechanical material properties.

The proper API for damping is through the B matrix in forcefields, so any forcefield is free to implement addMBK and friends using Rayleigh damping if they see fit with no extra cost, but it makes no sense to mandate a global Rayleigh damping.

I too strongly believe it should be set to zero by default, otherwise bogus/instable behaviours can easily go unnoticed.

[hugtalbot]

@damienmarchal how could we warn users about the fact that Data's default value changed ?

[damienmarchal]

@EulalieCoevoet is right, changing parameter without warning is bad @hugtalbot it is very easy, there is two options. One consist in adding some test in the init() function of the component to detect if the data is set or not by the user and provide the adequate message. The other is to add a dedicated rules in the SceneChecker at the following of:

void SceneCheckAPIChange::installDefaultChangeSets()
{
    addHookInChangeSet("17.06", [](Base* o){
        if(o->getClassName() == "RestShapeSpringsForceField" && o->findData("external_rest_shape")->isSet())
            msg_warning(o) << "RestShapeSpringsForceField have changed since 17.06. The parameter 'external_rest_shape' is now a Link. To fix your scene you need to add and '@' in front of the provided path. See PR#315" ;
    }) ;

    addHookInChangeSet("17.06", [](Base* o){
        if(o->getClassName() == "BoxStiffSpringForceField" )
            msg_warning(o) << "BoxStiffSpringForceField have changed since 17.06. To use the old behavior you need to set parameter 'forceOldBehavior=true'" ;
    }) ;

@fredroy Eulalie also report that there was some changes in the way shaders are pass to components and that old working behavior stopped working without warnings so adding warning in the same way would be nice (@EulalieCoevoet).