RegularGridTopology dimensions

[epernod]

Small PR on GridTopology and RegularGridTopology link to issue: #163

• Allow to set RegularGridTopology in 1,2 or 3 dimension. But still check if the grid resolution is valid.
• Fix example of Tetrahedron FEM that was crashing if no tetrahedron is found in the scene.
• Add an example scene.
• Remove RegularGridTopology dependencies from MechanicalObject.

CHANGELOG:

• GridTopology
  • clean the code & factor the constructor
  • implement "flat" grids in 1 or 2 dimension by using setting grid resolution to "1" in the corresponding axis
• RegularGridTopology clean the constructor's code & remove NDEBUG code.
• Adds RegularGridTopology_test
• TetrahedronFEMForceField clean code
• MechanicalObject removes code that was specific to the grid.
• Adds new examples scenes called: examples/Components/topology/Hexa2TetraTopologicalMapping.scn examples/Components/topology/RegularGridTopology_dimension.scn

  ---

  This PR:

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

Reviewers will merge only if all these checks are true.

[epernod]

@damienmarchal I have added a RegularGridTopology_test. In fact I don't think GridTopology is used like this as a component. It is more a mother class for Regular/Cylindrical and sphereGridTopology (which each has tests now)

[damienmarchal]

Thanks Eric for the fixes and the additional test.

Your answer about GridTopology rise other questions:

• If the component is not supposed to be used without being inherited... shouldn't we remove it from the factory ?
• If a class is not a component does not mean we shouldn't test it. Actually my feeling is that base class (as they are shared) are very good entities to test :)

EDIT: Maybe these questions should be for a new Issue and not to handle in this PR.

[epernod]

In fact most of the methods I'm testing are directly inside the mother class. So I'm not sure a gridTopology_test is needed. But to remove GridTopology from the factory is maybe possible. We need to investigate if someone is using it as a component.

[damienmarchal]

Hi Erik,

There is one test crash in TetrahedronFEMForceField_test. Is this related to your changes ?

DM

[epernod]

Should not but I will check. Yes in fact my fix in TetrahedronFEMFF reveals that the test was doing nothing: This the log on the master branch:

[ RUN      ] TetrahedronDiffusionFEMForceField_test/0.extension
[ERROR]   [TetrahedronFEMForceField(fem)]  object must have a mesh topology. The component is inactivated.  To remove this error message please add a topology component to your scene.

Right now it crashes... will see if I understand enough the test to fix it.

[guparan]

Hi @epernod Is this PR also linked to #162 ?

[damienmarchal]

Hi @epernod

Can I add more tests on this PR ? Because I think we should validate a bit more "negative/bad" case to enforce the behavior of component when used improperly.

[epernod]

help yourself.

[damienmarchal]

@matthieu-nesme
Do you think we should remove the dimension ? Or is it ok to have this PR switched to the ready state ?

[matthieu-nesme]

To me the notion of dimension is not well defined, keeping it as a function w/o Data would be better. unsigned getMaterialDimension() const ?

[untereiner]

I would suggest unsigned getDimension() const because the object should be considered as a topological space of a given dimension and not as a material since it does not have material properties yet.

[epernod]

I agree, "Material" does not mean something in this base component. This parameter should only be informative, not a Data, there is no mechanism to change the grid resolution if you change this dimension.

[epernod]

Finally do we have an agreement here? @matthieu-nesme @untereiner @damienmarchal Or do we remove that field?

[damienmarchal]

getDimmension() sounds ok to me. Not needed to have it as a Data. It can be recomputed each time getDimmension is called, there is no need to store it.

[damienmarchal]

@epernod, @matthieu-nesme do you agreed so that we can merge soon ?

[epernod]

It is ok for me. We are just speaking about an informative int or an enum. I can do the change (if any) as soon as their is a final decision.

[damienmarchal]

@hugtalbot, you set this PR to ready but didn't dismissed @matthieu-nesme's "request for change", it looks a bit ambiguous to me.

Let's take it easy, I will dismissed the request for change and do the merge. If this is not what was intented don't hesitate to revert or submit a small fix to remove the Grid_dimmension as matthieu suggested.

[damienmarchal]

[ci-build]

[matthieu-nesme]

I still do not get the interest of storing m_gridDim. What is the limitation of a simple function? So you never have to change the value of this variable to keep it consistent.

If it was in a data you want to plug in a Data/Engine graph, it would indeed be necessary to store it, but it is not the case here.

[damienmarchal]

Everyone agreed there is no interest in storing m_gridDim.
9 days ago erik asked: " Or do we remove that field?" followed by "I can do the change (if any) as soon as their is a final decision."

He is waiting for an answer since 9 days.
So here is the answer, "yes do the change and remove this field" :)

[epernod]

yes we can change the test to removebadDim as now the dim is not computed at the same time.

No, _n[i] = 1 works. n = [1; 5; 5] give a plan grid of 25 cells. -> Dim = 2

[matthieu-nesme]

@epernod If I am not wrong, if you have n=[2,2,2] your code will give dim=0 where it should give dim=3. Only a test could check that ;)

[epernod]

Indeed... my bad. This Pr already add tests for the gridTopology but the Tests needed a test. Thanks @matthieu-nesme for the review. It should be good now.

[damienmarchal]

[ci-build]

EDIT: Thank you all to make things moving and progressing.

[damienmarchal]

Hi Erik,

It seems that this PR introduced a new examples RegularGrid_dimmension that is failing because it use a component <EdgeSet/> that does not seems to be part of Sofa.

Can you provide a bit of details please.