sofa-framework / sofa

Real-time multi-physics simulation with an emphasis on medical simulation.
https://www.sofa-framework.org
GNU Lesser General Public License v2.1
871 stars 297 forks source link

[SofaCombinatorialMaps] dynamic topology into Sofa #700

Closed untereiner closed 3 years ago

untereiner commented 5 years ago

Hi guys,

This PR is a proof of concept to show an alternative implementation dynamic topologies and meshes using the CGOGN library. If you never heard about, CGOGN is a topological modeling kernel (see https://cgogn.github.io) implementing efficiently combinatorial maps to encode the topology and geometrical informations.

The current implementation is based on implementing new topology into new components.

The PR introduce the following components:

The PR is not supposed to be merged in this state and we welcome any feedback on how to improve and generalize this work.

EDIT: I (damien) changed the description of the PR trying to summary the discussion.

EDIT 2: Steps to compile:


This PR:

Reviewers will merge only if all these checks are true.

damienmarchal commented 5 years ago

Wow, thank Lionel for the PR. This is a fat one :)

untereiner commented 5 years ago

😄 I do not know how to proceed for you to test it. Maybe I can add test scenes to start somewhere.

hugtalbot commented 5 years ago

Hi @untereiner could you point us some examples scenes to run with your integration work ? Cheers

hugtalbot commented 5 years ago

and about the process, I think this PR is a very good test case for everyone to give it a try. Regarding the feedback we should be able to package it together as a public plugin.

hugtalbot commented 5 years ago

@untereiner could you please include CGOGN as an external library + separate the core changes and the pluginizable changes Any scene to test ? We are all curious to test it !

damienmarchal commented 5 years ago

Hi @untereiner

From my experience you will have more feedback if you provide more information on the PR description to give other some envy to try it.

Given the complexity of the PR here is what people passing by may need:

untereiner commented 5 years ago

Hi guys,

Actually I think this PR should not be merged in this state. It is a proof of concept to show the possibilities. I just updated a scene that you can use to test the new components.

The scene called "TetrahedralCorotationalFEMForceField" has two nodes. The first one uses the components "VolumeTopologyContainer" and "CMTetrahedralCorotationalFEMForceField" that are completely written using cgogn and "MapTetrahedronSetTopologyContainer" a compatibility wrapper that allows to use "classical" sofa components with cgogn.

@damienmarchal concerning your questions:

damienmarchal commented 5 years ago

Thanks for the answers, I moved them to the PR description.

"This work has been in progress since 3 years now. His interest has already been debated. I do not want to debate an infinite time on it. " I searched in the issues and PRs if there was ever any debate on that topic so that I could provide a link in the description but I didn't found one. Maybe you are referring to face to face discussions ? If this was the case maybe writing a summary of these debate (pro/cons) would be very informative and helpful for all of us.

"depends on the meaning of breaking" In general we are considering the following:

Damien

damienmarchal commented 5 years ago

After a quick look here are my notes:

DM. NOTE: I will edit my notes if after more reading I found some note are not relevant.

guparan commented 5 years ago

I'll try to take some time to refactor this.

guparan commented 5 years ago

Here is a rebased + cleaned + updated version without CGoGN: https://github.com/sofa-framework/sofa/compare/master...guparan:cmtopology_rebased_cleaned

To be mixed with this extracted CGoGN: https://github.com/guparan/sofa_cgogn

Please tell me if it works :-)

damienmarchal commented 5 years ago

75 files changes...instead of 1300 :) so much better. Thanks Guillaume.

untereiner commented 5 years ago

Thank you @guparan!

What I see first is:

These modifications should decrease the number of changed files a bit more

guparan commented 5 years ago

the files modules/SofaMiscFem/HexaFEMForceField.* should be renamed with CM prefix

Removed HexaFEMForceField and ElementFEMForceField instead.

untereiner commented 5 years ago

With the help of @guparan with made a few :) changes to this branch. To summarize:

cgogn is for now an external dependency and has to be checkouted manually in the extlib directory in order to compile the branch. We have to discuss a way to do it automatically (maybe an ExternalProject_Add ?). After a quick check, this proposal is compatible with the last commit of the cgogn devel branch.

untereiner commented 5 years ago

Hi everyone.

I just created a new branch (https://github.com/mimesis-inria/sofa/tree/cmtopology_module) where I put everything into a module called "SofaCombinatorialMaps" instead of using different existing sofa modules. To test it, you just have to put the cgogn repository that @guparan extrated earlier into a sofa/modules/SofaCombinatorialMaps/cgogn2 directory and it should compile :crossed_fingers:. I am able to launch examples/CMTetrahedralCorotationalFEMForceField.scn

guparan commented 5 years ago

This PR now contains the "module" version of cmtopology. Reviews are welcome :+1:

guparan commented 5 years ago

I did some cleaning on the PR but still need some help to make it compile. If someone could check it out that would be great :1st_place_medal:

hugtalbot commented 5 years ago

Well @untereiner the CI is not passing due to an issue of cmake version:

5.4_options_release/build/external_directories/fetched/CGoGN/CGoGN-prefix/src/CGoGN-stamp/CGoGN-configure
CMake Error at CMakeLists.txt:1 (cmake_minimum_required):
  CMake 3.7.2 or higher is required.  You are running version 3.5.1

we will investigate this next week

hugtalbot commented 5 years ago

Hi @untereiner @guparan

The commit https://github.com/sofa-framework/sofa/pull/700/commits/15a74a5966296d0611e617eaa9f2af4848251f36 seems to lead to a compilation failure

When removing it, ubuntu and MacOS are compiling. Windows crashes: error C2679: binary '>>': no operator found which takes a right-hand operand of type 'const sofa::core::cm_topology::TopologyChange *' (or there is no acceptable conversion) Centos failure was due to assimp: ninja: error: '/lib64/libassimp.so', needed by 'lib/libColladaSceneLoader.so', missing and no known rule to make it

damienmarchal commented 5 years ago

I was looking at he "stalled" PR,

And was wondering about this one, it contains a lot of valuable work and it seems to be connected to the cleanig & refactoring work on topologies started by @epernod.

@guparan , @epernod , @untereiner, @StephaneCotin what are your suggestion on how to move forward and have that work "not lost" ?

untereiner commented 5 years ago

There is only a windows compilation problem if I remember correctly. If someone can solve this issue I think It will be mergeable asap.