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

[SofaGeneralMeshCollision] Direct SAP is written as a narrow phase #2030

Merged
merged 9 commits into from
Apr 28, 2021

Conversation

alxbilger
Copy link
Contributor

SAP is usually a method to discriminate pairs of objects that are not in collision. The output is pairs of objects to be further investigated in the narrow phase.
However, in SOFA, the DirectSAP component is written as a component doing the broad phase and the narrow phase. Almost all the work is actually done in the narrow phase. That is why, we can consider this component is mainly a narrow phase component.
All the broad phase code has been removed. It is still a broad phase, but the code is located in its base class: BruteForceBroadPhase.
The narrow phase code has been extracted and put in a new component: DirectSAPNarrowPhase. DirectSAP inherits from DirectSAPNarrowPhase.
In the narrow phase, the result of the broad phase is used to skip the boxes which have been totally ignored in the broad phase. Initially, the DirectSAP component did not perform any discrimination in its broad phase. Now it performs a BruteForceBroadPhase. It is not yet clear if it is always a good choice regarding the performances. In the case of the caduceus scene, the performances are slightly better. I guess if the broad phase takes a lot of time (many objects), it is better to avoid it. DirectSAP would not be the best choice. I would use a more performant broad phase, or no broad phase, when used with DirectSAPNarrowPhase.

From now, the user can decide to design its scene with two different ways resulting to the same behavior:

<DirectSAP/>

or

<BruteForceBroadPhase/>
<DirectSAPNarrowPhase/>

In the second option, it will be possible to change the combination of broad phase/narrow phase.
Note: I kept the component DirectSAP, but I don't think its name is meaningful.

This is the following of the work started in #2010


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

…aceDetection

RayTraceDetection inherits from BruteForceBroadPhase. It makes more sense because the broad phase of RayTraceDetection was actually a brute force broad phase.
The narrow phase code from DirectSAP has been extracted and moved into DirectSAPNarrowPhase. DirectSAP now uses DirectSAPNarrowPhase.
@alxbilger alxbilger added pr: new feature Implement a new feature pr: status to review To notify reviewers to review this pull-request labels Apr 20, 2021
@alxbilger
Copy link
Contributor Author

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

@hugtalbot
Copy link
Contributor

by the way @alxbilger it would be great if you feel it's necessary to update the associated pages (DirectSAP, IncrSAP, BruteForce)

@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 Apr 28, 2021
@fredroy fredroy merged commit e6ab382 into sofa-framework:master Apr 28, 2021
@guparan guparan added this to the v21.06 milestone Jun 28, 2021
@alxbilger alxbilger deleted the optim_dsap branch June 28, 2022 10:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
pr: new feature Implement a new feature 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