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

[SofaCore] Revert #2047 for deterministic contacts #2067

Merged
merged 1 commit into from
May 7, 2021

Conversation

alxbilger
Copy link
Contributor

In this PR, map_ptr_stable_compare is preferred over unordered_map, as it allows reproducible contact response. However, unordered_map is still a lot faster.

#2047 leads to a crash in examples/Components/collision/RayTraceCollision.scn. I suspect the cause of this crash existed before #2047, and that #2047 revealed the crash by processing the contacts in a non-deterministic order. With the non-deterministic order, I can see that the crash does not happen always at the same current time step. It even happened that there was no crash.

Currently, I am not able to fix the crash. But I will continue investigating. As a workaround I restore the stable map to have deterministic contacts. IMO, it hides the cause of the crash.

In the end, I think the stable map should be proposed to the user (as a parameter?) by default. Let's see if we can accelerate it (perhaps with Insimo version).


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

For now, map_ptr_stable_compare is preferred over unordered_map, as it allows reproducible contact response. However, unordered_map is still a lot faster.
@alxbilger alxbilger added pr: fast merge Minor change that can be merged without waiting for the 7 review days pr: status to review To notify reviewers to review this pull-request labels May 7, 2021
@fredroy
Copy link
Contributor

fredroy commented May 7, 2021

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

@fredroy
Copy link
Contributor

fredroy commented May 7, 2021

Should be apply it right now, and discuss it in a devmeeting ? or would you want to wait to discuss before merging ?
(IMO we should apply it now)

@alxbilger
Copy link
Contributor Author

We can apply it now. It will satisfy the CI.
In the meantime, I continue working on the issue.

@fredroy fredroy 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 7, 2021
@fredroy fredroy merged commit 8c00a9b into sofa-framework:master May 7, 2021
@guparan guparan added this to the v21.06 milestone Jun 28, 2021
@alxbilger alxbilger deleted the deterministic_contacts branch June 28, 2022 10:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: fast merge Minor change that can be merged without waiting for the 7 review days pr: status ready Approved a pull-request, ready to be squashed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants