-
Notifications
You must be signed in to change notification settings - Fork 311
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[SofaBaseMechanics] Clean barycentric mapping #797
[SofaBaseMechanics] Clean barycentric mapping #797
Conversation
…opologyContainer part) 1-variable renaming with sofa prefix 2-added parent class (BarycentricMapperTopologyContainer) to gather duplicated codes 3-added templates
…opologyContainer part) Major refactoring: 1-added template on element type 2-removed apply, applyJ, applyJT, getJ, draw and init functions from child to BarycentricMapperTopologyContainer
Thanks for this massive PR @EulalieCoevoet 👍 Here is the error output for CentOS build:
SofaCUDA seems also to be a problem on Linux build. |
Could we have a bit more details about what you did and why you did it please ? |
Sure, sorry. So I wanted to optimize the initialization of BarycentricMapping, see issue #784 for clear description of the problem. To avoid more duplication of code, and to allow the optimization to be available for a maximum of topology types, I needed the code to be refactored. So:
The PR also include some cleaning, in particular the renaming of variables. That could indeed break not updated code. What I couldn't do, is to also refactor the code of BarycentricMapperMeshTopology... |
@EulalieCoevoet Hi, do you plan to fix the CUDA errors? |
…lasses into files
So, |
- moves the code in .h into .inl - add #include <sofa/core/visualparams.h>
…aTest and SceneCreator.
using SofaTest is bad but we should have working test to have the Barycentricmapping refactoring to work.
The CI still seems angry about the PR |
1- added missing includes related to the refactoring of BarycentricMapping into different files 2- removed commented code
Hi guys, |
The compilation problem appears between 7b183a2 and aeb4f8. |
@EulalieCoevoet I got the problem. It was because you added SOFA_BASE_MECHANICS_API to the mapper. I suspect the problem is that when you specialize the template in a different dll (like the rigid that is in miscmapping) tthe specialization is of course using SOFA_MISC_MAPPING_API while the base class is using SOFA_BASE_MECHANICS_API. |
Thanks @damienmarchal ! |
[ci-build][with-all-tests] |
@EulalieCoevoet |
@epernod lack of time... I guess it wasn't straightforward and I didn't want to spend more time on it. |
Cleaning and refactoring in BarycentricMapping (only for topology container)
description from comment below:
Sure, sorry. So I wanted to optimize the initialization of BarycentricMapping, see issue #784 for clear description of the problem.
To avoid more duplication of code, and to allow the optimization to be available for a maximum of topology types, I needed the code to be refactored.
So:
I created an abstract class (BarycentricMapperTopologyContainer) that gather the shared implementations of BarycentricMapperEdgeSetTopology, BarycentricMapperTriangleSetTopology, BarycentricMapperQuadSetTopology, BarycentricMapperTetrahedronSetTopology, and BarycentricMapperHexahedronSetTopology.
Enabeling to refactor the apply(), applyJ(), applyJT(), getJ(), draw() functions and more importantly the function I want to optimize, init().
The PR also include some cleaning, in particular the renaming of variables. That could indeed break not updated code.
What I couldn't do, is to also refactor the code of BarycentricMapperMeshTopology...
This PR:
Reviewers will merge only if all these checks are true.