-
Notifications
You must be signed in to change notification settings - Fork 305
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
[Core] Add topology subset indices test #4738
[Core] Add topology subset indices test #4738
Conversation
[ci-build][with-all-tests] |
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... |
btw why have you redundant values? This is not logical for an indice map. |
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... |
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 ? |
[ci-build][with-all-tests] |
[ci-build][with-all-tests][force-full-build] |
fa02d86
to
b633076
Compare
b633076
to
9e5fa12
Compare
* WIP * Added unit test on TopologySubsetIndices. Need to fix the tests * Fix removing process * Fix windows compilation * Change swap mechanism with an erase one, less efficient in time but keep list order
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 #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