[Framework] Apply template method design pattern to BaseMapping

[bakpaul]

Changed:

• apply
• applyJ
• applyJT (x2)
• applyDJT
• computeAccFromMapping
• getJ (x2)
• createMappedMatrix
• disable
• getJs
• updateK
• getK
• buildGeometricStiffnessMatrix

[ci-depends-on https://github.com/sofa-framework/PluginExample/pull/17]
[ci-depends-on https://github.com/SofaDefrost/ModelOrderReduction/pull/159]
[ci-depends-on https://github.com/sofa-framework/BeamAdapter/pull/189]

ℹ️ Need to be discussed

Some methods (such as apply) have been overloaded in child class, now we've got this stange pattern :
Mother::apply -> Mother::doApply(pure virtual) --> Child::apply
Just because we used an oveload

---

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).

[bakpaul]

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

[sofabot]

[ci-depends-on] detected during build #2.

To unlock the merge button, you must

• Merge or close Fix compilation regarding SOFA PR 5466 PluginExample#17
  For this build, the following CMake flags will be set
  -DPLUGINEXAMPLE_GIT_REPOSITORY='https://github.com/sofa-framework/PluginExample'
  -DPLUGINEXAMPLE_GIT_TAG='a9e6b0df8b240a0542b9e4a9c03fc7c025c0d478'
• Merge or close Fix compilation regarding SOFA PR 5466 SofaDefrost/ModelOrderReduction#159
  For this build, the following CMake flags will be set
  -DMODELORDERREDUCTION_GIT_REPOSITORY='https://github.com/SofaDefrost/ModelOrderReduction'
  -DMODELORDERREDUCTION_GIT_TAG='509d4d9b8cbc06f6502681a6e3ba476da69041fa'
• Merge or close Fix compilation regarding SOFA PR 5466 BeamAdapter#189
  For this build, the following CMake flags will be set
  -DBEAMADAPTER_GIT_REPOSITORY='https://github.com/sofa-framework/BeamAdapter'
  -DBEAMADAPTER_GIT_TAG='022584f92b64fb7361352017fc5dbbc9ae90e3e6'

[alxbilger]

Suggested change

	void doApply (const MechanicalParams* mparams, MultiVecCoordId outPos, ConstMultiVecCoordId inPos ) override;
	void doApply (const MechanicalParams* mparams, MultiVecCoordId outPos, ConstMultiVecCoordId inPos ) final;

?

[damienmarchal]

What a massive work.

I'm surprised by the move from the pure virtual BaseMapping's method to virtual final. To me the purpose of declaring a method virtual with final keyword prevent further re-implementation... and thus is equivalent to a non virtual method.

About the overload of apply()...
The current code is more or less:

BaseMapping
{
public:
     virtual void apply (const MechanicalParams* mparams = mechanicalparams::defaultInstance()) = 0;
};

Mapping : public BaseMapping
{
public:
     void apply (const MechanicalParams* mparams = mechanicalparams::defaultInstance()) override;
     virtual void apply (const MechanicalParams* mparams, OutDataVecCoord& out, const InDataVecCoord& in) = 0;
};

AMapping : public Mapping
{
public: 
     void apply (const MechanicalParams* mparams = mechanicalparams::defaultInstance()) override;
     void apply (const MechanicalParams* mparams, OutDataVecCoord& out, const InDataVecCoord& in) override;
};

After applicaition of the template method pattern I should looks close to:

BaseMapping
{
public:
     // this method is public and non virtual, because we don't want subclass to "tune" (or screw) its internal logical. 
     void apply (const MechanicalParams* mparams = mechanicalparams::defaultInstance()) { 
                // ... common code here....  
                
                // calls the delegate only when pre-requierement/context is fullfill 
                doApply(...); 
                
                // ... other common code here 
     }

// there is a choice between private and protected. 
// the choice depend if the method can be used in isolation in the child classes or if it is not supposed to. 
private:  
     // child class must implement something 
     virtual void doApply(const MechanicalParams* mparams) void = 0; 
};

Mapping  : public BaseMapping
{
private:
     void doApply(const MechanicalParams* mparams) override {
              // the actual implementation providing the service to the "parent class"
              // on this case, apply is actually dispatching to a new level of delegates
              
              // ... common code should be here ...

              doApplyOnCoordinates()
              
              // ... common code should be here ...
     }
     
     // to de-ambiguate the second apply() method introduced in Mapping.h, give it a second name.  
     void doApplyOnCoordinates (const MechanicalParams* mparams, OutDataVecCoord& out, const InDataVecCoord& in) = 0;
};

AMapping : public Mapping
{
private: 
     void doApplyOnCoordinates (const MechanicalParams* mparams, OutDataVecCoord& out, const InDataVecCoord& in) final {
     /// implements things here. 
     }
};

The chaining of delegates with different names makes very explicit what is expected from each method. Because each use of TMP (template method pattern) can be seen as a contract between the base class and its child on the specific, and sole, purpose of the delegated method as well as its context of use.

[bakpaul]

It is not equivalent, otherwise the keyword 'final' wouldn't exist... Actually, doing this we make sure no one shadows the method in a child class. You are right that without the virtual keyword, polymorphism will not apply, but it allows some bad coding practice that could lead to very hard to track runtime errors. In my opinion we should keep the final to make sure no one ever overrides it, this will avoid some tricky situations and changes nothing really in the API.

Your suggestion seems good to me, nevertheless, we wanted to keep this first salve of PRs only at the first level of inheritance. Methods introduced at the second level will be treated in a second stage (maybe a new sprint ?). We will keep your comment in mind then !