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

[Mass] Fix point topological changes for UniformMass #2853

Merged

Conversation

hugtalbot
Copy link
Contributor

@hugtalbot hugtalbot commented Apr 8, 2022

Based on #2870

The topological callback was actually not working for the UniformMass.
Now POINTSADDED and POINTSREMOVED are handled

in addition:

  • add override to avoid massive warnings
  • add box in scene to allow seeing the mesh

Thanks to @Oparras for rising this in #2668
A regression could be added on this scene


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).

@hugtalbot hugtalbot added pr: fix Fix a bug pr: status to review To notify reviewers to review this pull-request labels Apr 8, 2022
@hugtalbot hugtalbot requested a review from epernod April 8, 2022 16:39
@fredroy fredroy added pr: status wip Development in the pull-request is still in progress and removed pr: status to review To notify reviewers to review this pull-request labels Apr 13, 2022
@epernod
Copy link
Contributor

epernod commented Apr 13, 2022

Thank you for this PR, it highlights a more serious bug when adding elements in TopologySubsetData. A PR to fix the TopologySubsetData is coming, then we will update this PR to adapt UniformMass with the good callbacks.

Right now the callback on PointRemoved event proposed in this PR will crash as the TopologySubsetData already has an internal mechanism to update its size. So you perform the deletion twice.

@epernod
Copy link
Contributor

epernod commented Apr 14, 2022

Update the FIX but now this PR depends on change done in PR #2870

@hugtalbot
Copy link
Contributor Author

any update on the #2870 @epernod ?

@epernod
Copy link
Contributor

epernod commented Apr 27, 2022

any update on the #2870 @epernod ?

@hugtalbot now that #2869 has been merged, you can review the #2870, then this one will be the following...

@epernod epernod added pr: status to review To notify reviewers to review this pull-request and removed pr: status wip Development in the pull-request is still in progress labels Apr 29, 2022
@epernod
Copy link
Contributor

epernod commented May 2, 2022

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

@epernod epernod force-pushed the fix_topochange_uniformass_20220408 branch from ece45e7 to 9d4e313 Compare May 11, 2022 07:41
@epernod epernod force-pushed the fix_topochange_uniformass_20220408 branch from 9d4e313 to 5ce37c0 Compare May 13, 2022 18:00
@epernod
Copy link
Contributor

epernod commented May 15, 2022

@hugtalbot this one is ready for me. Could you double check and merge?

@hugtalbot hugtalbot 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 16, 2022
@hugtalbot hugtalbot merged commit 8b09e29 into sofa-framework:master May 16, 2022
@guparan guparan added this to the v22.06 milestone Jun 23, 2022
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.

4 participants