sofa icon indicating copy to clipboard operation
sofa copied to clipboard

[SofaSimpleFem] Refactor TetrahedronFEMForceField for better readability

Open alxbilger opened this issue 3 years ago • 8 comments

This is a tentative to refactor TetrahedronFEMForceField for better readability.

All the FEM methods have been dispatched into dedicated files.

The methods are now classes sharing the same interface (polymorphism), called by the component.

Advantages:

  • TetrahedronFEMForceField is smaller
  • TetrahedronFEMForceField had many similar codes (often identical). This is now avoided as all common code are in the base classes.
  • It is easier to read the differences between the different FEM methods.
  • We could imagine having a factory of FEM methods for TetrahedronFEMForceField. Then, the component could be extended in plugins by adding more methods into the factory.

To do:

  • [ ] Clean
  • [ ] Move some class members from TetrahedronFEMForceField to TetrahedronFEMForceFieldImpl
  • [ ] Restore initial performances
  • [ ] Fix broken features such as drawing rotations
  • [ ] If everyone agrees, remove the assembled methods. It adds complexity while the resulted matrix is not used.
  • [ ] Adapt (if necessary) the derived classes (multithreading, CUDA etc)

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 avatar Dec 17 '21 12:12 alxbilger

arg... good input but it is not going to help my work on merging back this class with TetrahedralCorotationalFEMForceField... I started the other way, by moving the "generic" methods into a TetrahedronFEMUtils

epernod avatar Dec 17 '21 12:12 epernod

There is a lot of code in the .h. Would it be possible to have the same refactoring while keeping the headers clean.

damienmarchal avatar Dec 27 '21 17:12 damienmarchal

@damienmarchal @epernod This PR is a WIP. I did not finish it on purpose. The goal was to show a new organisation of the code and discuss about it. Thanks for having reviewed it. It means we can discuss about it on Wednesday ;)

alxbilger avatar Jan 03 '22 08:01 alxbilger

Discussion on the improvement of FE codes in SOFA, Caribou plugin etc. Clarifying the FEM codes is really good, using utility classes (static functions) can help the reading, understanding of the physical model. The loss of performance in addDForce must be investigated → keep wip

hugtalbot avatar Jan 12 '22 10:01 hugtalbot

Some bench to see the perf loss, on examples/Components/forcefield/TetrahedronFEMForcefield (with only one beam with small method): master: TetrahedronFEMForceFieldScene>/1024 4504 ms PR: TetrahedronFEMForceFieldScene>/1024 5735 ms +~25% slower... 😥

fredroy avatar Jan 12 '22 17:01 fredroy

Some bench to see the perf loss, on examples/Components/forcefield/TetrahedronFEMForcefield (with only one beam with small method): master: TetrahedronFEMForceFieldScene>/1024 4504 ms PR: TetrahedronFEMForceFieldScene>/1024 5735 ms +~25% slower... 😥

@fredroy I don't remember such a high difference. I don't think I worked on small deformation. Have you tried large def?

alxbilger avatar Jan 13 '22 07:01 alxbilger

Some bench to see the perf loss, on examples/Components/forcefield/TetrahedronFEMForcefield (with only one beam with small method): master: TetrahedronFEMForceFieldScene>/1024 4504 ms PR: TetrahedronFEMForceFieldScene>/1024 5735 ms +~25% slower... 😥

@fredroy I don't remember such a high difference. I don't think I worked on small deformation. Have you tried large def?

For big deformation: master: TetrahedronFEMForceFieldScene>/1024 6717 ms PR: TetrahedronFEMForceFieldScene>/1024 8100 ms so ~20% slower

Additional note, these benches are done using clang(13) on linux (and in a VM)

fredroy avatar Jan 13 '22 08:01 fredroy

Update (as bench on scenes is unreliable)

  • Linux/clang: 15.5sec (master) vs 16.5sec (pr)
  • Windows/cl: 17.5sec (master) vs 18.2sec (pr)

Much less difference after all

fredroy avatar Jan 13 '22 10:01 fredroy