Skip to content
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

Merged

Conversation

bakpaul
Copy link
Contributor

@bakpaul bakpaul commented May 16, 2024

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

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

@bakpaul bakpaul added pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request labels May 16, 2024
@bakpaul
Copy link
Contributor Author

bakpaul commented May 16, 2024

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

@epernod
Copy link
Contributor

epernod commented May 16, 2024

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
Copy link
Contributor

epernod commented May 16, 2024

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

@bakpaul
Copy link
Contributor Author

bakpaul commented May 16, 2024

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
Copy link
Contributor Author

bakpaul commented May 16, 2024

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 ?

@hugtalbot hugtalbot added this to the v24.06 milestone May 22, 2024
@bakpaul
Copy link
Contributor Author

bakpaul commented May 24, 2024

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

@bakpaul
Copy link
Contributor Author

bakpaul commented May 24, 2024

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

@fredroy fredroy force-pushed the add_topology_subset_indices_test branch from fa02d86 to b633076 Compare May 27, 2024 06:03
@bakpaul bakpaul added pr: backport todo This PR will be backported into the release preceeding its milestone. and removed pr: backport todo This PR will be backported into the release preceeding its milestone. labels May 27, 2024
@epernod epernod force-pushed the add_topology_subset_indices_test branch from b633076 to 9e5fa12 Compare May 31, 2024 07:01
@epernod epernod modified the milestones: v24.06, v24.12 May 31, 2024
@epernod epernod added pr: status ready Approved a pull-request, ready to be squashed and removed pr: status to review To notify reviewers to review this pull-request labels May 31, 2024
@epernod epernod merged commit 62c2a29 into sofa-framework:master May 31, 2024
15 checks passed
bakpaul added a commit to bakpaul/sofa that referenced this pull request Jun 5, 2024
* 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
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fix Fix a bug pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants