[Lagrangian.Model] Remove use of LateSpecialization pattern

[alxbilger]

The LateSpecialization pattern, introduced in https://github.com/sofa-framework/sofa/commit/1b92a802eb69693af65f2094f02f9a734d333804, was used to avoid code duplication between float and double specialization of Rigids. However, now that there is only one specialization, it makes the code more difficult to read. Therefore, I suggest to go for a standard specialization.

Note that the diff may be difficult to read, but this PR only moves code (with adaptation).

---

By submitting this pull request, I acknowledge that
I have read, understand, and agree SOFA Developer Certificate of Origin (DCO).

---

Reviewers will merge this pull-request only if

• it builds with SUCCESS for all platforms on the CI.
• it does not generate new warnings.
• it does not generate new unit test failures.
• it does not generate new scene test failures.
• it does not break API compatibility.
• it is more than 1 week old (or has fast-merge label).

[alxbilger]

@damienmarchal do you have a ressource for the Late Specialization pattern? I understand the concept, but I cannot find formalism of this pattern.

[alxbilger]

[ci-build][with-all-tests]

[damienmarchal]

Hi @alxbilger

Thanks for the PR and requesting a review.

The existing code makes the assumption we only want Vec3 and Rigid3, the PR builds on that assumption to make the implementation cleaner and clearer. The changes dones are relevant and can probably merged as-is.

Nevetheless the PR makes the underlying assumption that we only want Vec3Type and Rigid3Type is questionnable in several ways and is also connected to several of Sofa's code architecture issues that are worht discussing.

1. Do we want to support 1, 2,4, 6d constraint ? If this is the case then the current non specialized implementation need to be check it actually compiles and do the expected thing. If this is not the case we should consider fixing that. But if handling 1,2,4, 6d is not possible nor a desirable property the we should consider making code extension through in-heritance and subclassing instead of templatization.

2. Asymetrical implementation regarding the floating point precision: The current implemented allows users to add extra instanciation for Vec3 (f,d) but do not allow such a thing for Rigid3f. This asymetry in implementation is weird to me as I tend to think that if there is functionnal equivalence regarding the templated data (i.e that one data type is in no way special against the other => Vec3 is i no way "better" compared to Rigid3) then this functionnal equivalence results in a code base that exhibit symmetrical implementation where the Vec3 and Rigid3 are treated the same. This is not the case as one is specialized in .cpp while the other is not specialized and is implemented in the .inl.

3. Shouldn't we remove the "call-super" ? The code and PR makes use of what we call the "call super"design patern in which an specialization or an in-herited class must call its parent/general implementation to behave correctly. I think the PR contains a variation for "template" of this pattern with the unspecializedInit(). In most of the case I noticed that the "call super" was a bad design choice as it delegate the "validity" of the implementation to the child/specialization instead of enforcing the behavioral validity by code structure. Example of the call super "design"

class GeneralImplementation
{
public:
    void doSomethingCrucial(); 
    virtual void init()
    {
        doSomethingCrucial();
    }
}

class UserImplementation : GeneralImplementation
{
public:
    virtual void init()
    {
         GeneralImplementation::init();     
    }
}

And how it can be change to prevent mis-implementation:

class GeneralImplementation
{
public:
    void doSomethingCrucial();
    void init()
    {
        doSomethingCrucial();
        doInit();    
    }
    
protected:
    virtual void doInit() = 0;      
}

class UserImplementation : GeneralImplementation
{
public:
    final void doInit() override
    {
         /// my specific things
    }
}

I don't know if this could/may/must be applied to BilateralInteractionConstraint...but at first sight it looks like a good sanbox.

4. Mixing reset/init I see that reset() is actually calling init(). To me this leads to a lot of fuzzyness in which it is not clear what are the purpose of these function. Maybe it is possible to rework that part to make clear what are the specific role of the API entry points. This probably requires to clarify what reset is supposed to do (and this is probably not strictly equivalent to init()).

5. Extra side notes: I would recommand deprecating all handleEvent implementation by printing a deprecation message saying that you need to use a separated controller. Because...well this is just a non-sense to have hardcoded keycode and UX concerns.

[damienmarchal]

• I made to PR trying to have symetry in the implementation: https://github.com/alxbilger/sofa/pull/2
• I also tested the code with Vec2 and Vec1 and it does not compile (so the use of template there is highly questionnable)

It could also be a good use case for c++ 20 concepts;

More comments (to go totally out of the scope of the PR):

• the methods "addContact" are probably not properly name... as this component has nothing to do with contact maybe addConstraint (?)
• the "merge" data field is only in in Vec3 which means either the data field should be only on that specialization or that it should be used in Rigid3Type (or at least printing an "unsupproted behavior" when set)
• the "keepOrientDiff" data field is only for Rigid, it shouldn't exists at all in the Vec3 version (it can be considered an favor of having inheritance in addition to specialization).
• the "numericalTolerance" data field is only used for rigid specialization (so same comment as for keepOrientDiff" and it seems it is only for rotational part.
• why BilateralInteractionConstraint<Rigid>::getVelocityViolation() is empty ?
• in BilateralInteractionConstraint<Rigid3Types>::getConstraintResolution() there is getValue() in for loops()
• accessors are not used in BilateralInteractionConstraint<Rigid3Types>::buildConstraintMatrix()

[alxbilger]

@damienmarchal you went far beyond my first intention to improve a bit the readability. Your suggestions seems fine, and I agree with your comments. Although I am not in favor to address them all in this PR.

If I understood correctly, with this new design, we would be able to compile all template specializations with both single and double precision, and without duplicating code specialization. Am I right?

[damienmarchal]

Yes, I kind of hijacked the PR to open general discussion, but as my first line was saying:

"The existing code makes the assumption we only want Vec3 and Rigid3, the PR builds on that assumption to make the implementation cleaner and clearer. The changes dones are relevant and can probably merged as-is."

All what follow is probably for future PR.

[damienmarchal]

Beware @alxbilger, in https://github.com/alxbilger/sofa/pull/2 I made a mistake by removing the code for handleEvent(). Maybe the initial code need to be restore in to stay focus on just initial topic of the PR

[fredroy]

• there are some different kind of { ... } against the guidelines but it was there before so it is not really the point of the PR
• I would put the content of the specialized(?) inl files directly into the associated cpp.

[damienmarchal]

Hello @alxbilger, @fredroy, @hugtalbot

Here is a quick test around the idea we discussed during the today's sofa dev meeting: https://godbolt.org/z/9f1z4nbfq