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
883 stars 300 forks source link

[Core] Add topology subset indices test #4738

Closed bakpaul closed 2 weeks ago

bakpaul commented 1 month ago

During my Way of the Cross of fusing StiffSpring and its parent, I saw that the topological change of removing points didn't work as planned for topologySubsetIndices when there is multiple occurrence of the same element in the data.

I've fixed that and added tests.

One question is remaining though : here I kept the original mechanism using a swap of the deleted element and the last one. This is efficient in term of memory but it has the side effect of changing the indices order in the data. --> My question is, is that what we want ? Do we prefer memory/time efficiency over order coherency for this data ? Is it logical to get a random order of the vector out of a simple topological change ? This answer will change a bit the way I'll finish the refactoring in https://github.com/sofa-framework/sofa/pull/4649


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

bakpaul commented 1 month ago

[ci-build][with-all-tests]

epernod commented 1 month ago

as this component is in fact a kind of map. The swap might not be needed... but without diving back into this I'm not 100% sure... normally there is enough regression tests on topology modification to detect if removing the swap will break the mechanism.

epernod commented 1 month ago

btw why have you redundant values? This is not logical for an indice map.

bakpaul commented 1 month ago

Well it isn't actually a map in the code but a vector. This TopologySubsetIndices data is used in SpringForcefield to keep track of the topological changes and contains the indices on which each spring is attached. Given the fact that multiple springs can be attached to one point, then this vector may contains multiple occurrence of the same index (and it is the case for some unit tests currently). And more importantly its order is very important because it should be kept consistent with the order of the list of attached indices of the second object...

bakpaul commented 1 month ago

I though that maybe this feature of supporting multiple occurrence was meaningfull for all TopologySubsetData, but it might be a special case for topologySubsetIndices, su I could overlead the method in its declaration if you prefer ?

bakpaul commented 3 weeks ago

[ci-build][with-all-tests]

bakpaul commented 3 weeks ago

[ci-build][with-all-tests][force-full-build]