[SofaKernel] issofa_topology: Improvement, BugFix and Cleaning on Topology

[guparan]

New features

• UncoupledConstraintCorrection : add option to disable handling of topological changes for compliance vector
• Container : add accessor to the topology type
• SubsetMapping : add resizeToModel flag in SubsetMapping to enable resizing of the destination MechanicalState to the size of indices
• BaseContext : add new method getActiveMeshTopology to get relevant topology, taking BaseMaping::sameTopology() into account
• PointSetTopology : add some unit tests

Improvements

• each time a topological change sequence is detected in BarycentricMapperTriangleSetTopology the mapping is initialized
• PointSetTopologyContainer maintains a list of point indices, accessible through the data points
• Topology : Factorize the last method specific to each topological elements in data containers

Cleans

• MechanicalObject size and link to topology is now exposed as Data and Link, using the new getActiveMeshTopology() method. Set the link manually and/or set useTopology="false" to override

Bugfixes

• apply data container updates before mapping updates
• use existing parameter handleTopologyChange in SubsetMapping
• slightly cleanup topology change handling method for triangle mappers
• check of upper topology based on the emptiness of the upper container
• crash in MeshTopology::init() for non manifold meshes
• re-enable Propagate of topological changes inbetween triangle creation and removal in TriangleSetTopologyModifier::addRemoveTriangles(), as it is required in cases involving chains of topology mapping and topology data containers depending on each other

This is the re-opening of previous issofa_topology pull-request.

---

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] has been reviewed and agreed to be transitional.
• [x] is more than 1 week old.

Reviewers will merge only if all these checks are true.

[guparan]

[ci-build]

[damienmarchal]

Hi all,

This PR is getting old, would it be possible to:

• Fix the conflict
• Update the reviewer's status if it is now ok

Thanks.

EDIT: @matthieu-nesme Are you still requesting changes on this PR ?

[matthieu-nesme]

@damienmarchal I already added comments w/o answer, but I commented them again

[JeremieA]

Answered (and please remember that all this was already reviewed and discussed during STC, the remaining issue was supposed to be only to fix sofaCUDA as far as I understood)

[damienmarchal]

@matthieu-nesme and @JeremieA many thanks for the quick updates.

@guparan I see you made a commit (15 May) about SofaCUDA. Does it means this pending issue is solved and we can merge or is there still something to do ?

[damienmarchal]

According to the CI there is one test in Flexible that is failing in this PR.

[matthieu-nesme]

Indeed and it is related to the size of a mstate "Size of output dofs is wrong: 4 expected: 1"

[hugtalbot]

what and how would you recommand to fix this error @matthieu-nesme ?

[matthieu-nesme]

I presume it has nothing to do with Flexible, but it rather seems related to the mstate size.

Maybe the test could be fixed, but what is important to notice is that this PR can have unexpected behaviors with existing code.

[damienmarchal]

I really would like to have this merged. So how could we progress ?

• can we remove the feature that makes the tests fail ?
• do we fix the test and hope others person code will no be broken silently ?
• can we implement kind of backward compatibility tricks ?

[matthieu-nesme]

No way!

Did you have a chance to check what is wrong? It can be nothing as it can be fundamental, it has to be checked for sure.

[damienmarchal]

Thanks for the clear answer,

I'm afraid that if no one spend time on fixing this issue, the whole PR will wait for an un-defined amount of time. On my side I'm fully booked.

[guparan]

I'm gonna find some time to investigate this test failure. To be continued...

[damienmarchal]

A gentlereminder :) @guparan

[hugtalbot]

Apparently a remaining test is failing : TetrahedronVolumeMappingTest/0.test_perTetra Are you on it @epernod ?

[epernod]

I'll try, as soon as I can compile Sofa and all the plugins ( you know what I mean @hugtalbot ;) )

[JeremieA]

Hello,

Before I explain the bug, let me give a bit of context (it is a bit long, but I hope it is useful ;)).

The point of this change was to make it transparent which topology is used by which state, and allow for correct initialization and most importantly topological changes propagation. One issue with the initial implementation, associating a state to the first topology found in its parents and then ignoring it if it is not in the same context (hopefully in all codes using state->m_topology, but who checked?), is that this make it impossible to share the same topology (and topological changes) with multiple states in different (children) nodes. One such example is when you use an IdentityMapping (or a Rigid <-> Vec3 mapping or any other mapping that produce the same number of outputs as its inputs), previously to get correct topology association and changes propagation you would need to replicate the topology (and the topological mappings) with a topology both before and after the mapping (but beware if they end up with different sizes, or updated in the wrong order...).

With our changes, there is a clear(er) method BaseContext::getActiveMeshTopology() that you can call from any context and will give you the topology that is relevant for this context, i.e. either the topology at this exact node, or from a parent node but only if there is no mapping in between (or if there are only mappings whose sameTopology() method returns true). Using this method, state->l_topology is initialized (and you can inspect its result in the GUI), and if it is not NULL it will be used by all computations for the state without extra hidden checks.

Now back to this bug, the last issue here was that the tested mapping (TetrahedronVolumeMapping) was not preserving the topology, but its output state was somehow linked to its input topology. The solution is not to go back to the old (broken) behavior, but instead to understand what is the actual root cause here. Theoretically, if the logic described above was behaving correctly, the output state would not be able to see the input topology because of the use of getActiveMeshTopology() and the fact that the mapping sameTopology() method would return false (thanks to its "safe" default implementation). The reason why this incorrect link was being created was because the mapping was created in the wrong Node. Normally, Sofa requires mappings to be in the same node as its output state. This is important in order to know for example which mapping is associated to which state (at least prior to the introduction of reflective Links, and still required because Mapping->State links are only one-way). This is used for example by visitors to know if a state is to be considered as real DOFs (because of the absence of a mapping in its node). But the MappingTest initialization code was not respecting this requirement, instead putting the mapping in the root node along with the input state (and the topology). Only the output state was in the child node. In this case, getActiveMeshTopology() from the output state does not see any mapping in the child, allowing to go to the parent to find a topology. It assumes that the Mapping in the root node is there to write to the state/topology of the root node, so it is not considered as something that would block the topology to remain active. Hence the wrong link being created, and the bug. The fix (beaaaf5) is a simple one-line change in MappingTest, creating the mapping in childNode instead of root, which should have been the case to respect Sofa's conventions (but which are unfortunately not checked/asserted by anyone).

Hopefully all is well now. It's difficult to know for sure because not-disabled tests are still failing on my computer (but they are also failing on the master...)

[guparan]

[ci-build][with-scene-tests]

[damienmarchal]

[ci-build] I was passing by, noticed a PR ready...clicked on the merge button :)

[JeremieA]

cool, but it looks like the merge button did not do its job :/

[damienmarchal]

Oups I wanted to make a rebuild against a recent master to be sure and forgot. Now done. :)

GG to everyone that make the PR & that helps in having it reviewed & merge If someone notice something, please report asap on the issues board.