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

Normal and nearest points uniformization #329

Merged
merged 31 commits into from
Aug 1, 2022

Conversation

lmontaut
Copy link
Contributor

@lmontaut lmontaut commented Jul 29, 2022

This PR addresses #307.

In short:
The library is made uniform w.r.t the definition of the witness points (in hppfcl they are called nearest_points, we might want to change that to avoid confusion) and the normal.

In details:
Depending on which collision pair between two objects o1 and o2, there were concurrent definitions of their normal and their witness points. The reason is that GJK and EPA were rewritten in hppfcl but followed a different convention compared to the original fcl library when it comes to the definition of the normal.
Thus, collision pairs involving specialized functions (not using GJK+EPA) were behaving differently than GJK+EPA.

In addition, the specialized functions returned collocated witness points whereas GJK+EPA returned the true witness points.

To address this, this PR does the following:

  • adds documentation to define the normal and the nearest points. In short, it simply expands on the documentation already present in hppfcl: the normal always points from o1 to o2. It follows that the distance d(o1, o2) between the shapes is always a signed distance (even when using collide, contrary to current behavior): it is negative when collision, positive otherwise. The normal n and the witness points p1 and p2 are related in that the normal always points in the direction of the separation vector: separation_vector = sign(d(o1, o2)) * (p2 - p1) and n = separation_vector.normalized(). The separation vector can be used to bring the shapes into touching contact (share at least a contact point bu zero intersection volume). Indeed, if we translate o1byseparation_vector`, regardless of collision or not, we bring the shapes into touching contact.
  • adds a test to make sure all collision pairs' normal and nearest points verify the definition stated above. This checks tests the relationship between the normal, the separation vector, the witness points, the signed distance etc. Once this is passed, it translates o1 according to the separation vector and makes sure that the shapes are in collision if they were not or not in collision if they were.
  • Updates the behavior of GJK+EPA: now the normal always points from o1 to o2. Previously GJK+EPA's normal was pointing from o2 to o1 when the shapes were colliding.
  • Updates the behavior of all the specialized functions accordingly.

This PR is long, apologies for that. I think that the most important is to understand and agree on the definition of the normal given above. I sticked to the existing documentation, i'm happy to update it if everyone agrees on a different definition of the normal. The other important thing is to understand the test I added, test/normal_and_nearest_points.cpp, which guarantees all collision pairs follow the same definition.

All of this corresponds to the first 9 commits of the PR, the rest is simply updating each specialized function.

Copy link
Member

@jcarpent jcarpent left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR seems good to me.
I suggest creating a new branch called hppfcl3x, as the modifications imply a break in the API.

python/collision.cc Show resolved Hide resolved
src/narrowphase/details.h Outdated Show resolved Hide resolved
@jcarpent jcarpent changed the base branch from devel to hppfcl3x July 29, 2022 13:00
@lmontaut
Copy link
Contributor Author

lmontaut commented Jul 29, 2022

The CI is failing because I didn't serialize the std::array of nearest_points properly.

@nim65s
Copy link
Contributor

nim65s commented Jul 30, 2022

The CI is also failing because #327. Can you rebase this on devel ?

lmontaut added 22 commits July 30, 2022 15:16
This is a test to see if documentation can appear in a commit.
@lmontaut lmontaut force-pushed the normal_and_nearest_points branch from 4d53b5d to 169b700 Compare July 30, 2022 13:17
@lmontaut
Copy link
Contributor Author

The CI is also failing because #327. Can you rebase this on devel ?

Done. Thanks @nim65s ! Is it also possible to rebase hppfcl3x on devel? I am not sure I have the rights to do that.

@nim65s
Copy link
Contributor

nim65s commented Jul 30, 2022

hppfcl3x is behind devel, so you can fast-forward by just pushing there. I think you should have the right for this. Can you check this ?

@lmontaut lmontaut changed the base branch from hppfcl3x to devel July 30, 2022 15:29
@lmontaut lmontaut changed the base branch from devel to hppfcl3x July 30, 2022 15:30
@lmontaut
Copy link
Contributor Author

Yes, I do have the right, thanks!

@jcarpent jcarpent merged commit 650f8e1 into coal-library:hppfcl3x Aug 1, 2022
@florent-lamiraux
Copy link
Contributor

@lmontaut : Thank you for this valuable work. I did not look into all the modifications. Can you confirm that after calling fcl::ComputeCollision::operator(), if there is a collision, the method CollisionResult::getContact still returns a point that is in the intersection between the two objects ?

@lmontaut
Copy link
Contributor Author

lmontaut commented Aug 1, 2022

Hi @florent-lamiraux, yes CollisionResult::getContact still returns a point in the intersection, no matter if collide or ComputeCollision is used. In fact, for most pairs, getContact returns the same point as before. The changes I made only change the contact point's position for halfspace-... and plane-....

A bit more details: the changes I made is to add the nearest_points attribute in the Contactstruct (used inside CollisionResult) such that the contact point (CollisionResult::getContact(i).pos) is 0.5*(nearest_points[0] + nearest_points[1]. The nearest_points are defined such that their difference gives the separation vector (vector of smallest norm such that o1 translated by that vector bring o1 and o2 to touching contact).

@lmontaut lmontaut deleted the normal_and_nearest_points branch August 1, 2022 08:42
@florent-lamiraux
Copy link
Contributor

Great. Thank you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants