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

Add CollisionResult::nearest_points #200

Closed
wants to merge 4 commits into from

Conversation

jmirabel
Copy link
Contributor

@jmirabel jmirabel commented Oct 27, 2020

This supersedes #198 .

Work left to be done:

  • agree on the API
  • handle shape-shape collision
  • add some unit test
  • revise the distance lower bound (does not take the security margin).

Class ShapeCollisionTraversal should be removed.

As a bonus, this also solves #199.

@jmirabel
Copy link
Contributor Author

@nim65s does it make sense to turn off travis (except for osx) ?

@nim65s
Copy link
Contributor

nim65s commented Oct 27, 2020

I prefer not to have opinions about travis. If someone wants to use and maintain it, that's perfectly fine for me, but on my side I have enough work and results with one CI stack :)

@jmirabel
Copy link
Contributor Author

After some effort, I ended up with the following.

  • for collision queries, all the returned distances take the security margin into account,
  • the collision result stores valid nearest_points whenever the distance lower bound is inferior to break_distance. When the distance lower bound is higher, their validity depends upon the pair of object type (maybe we should add a boolean that says whether they are valid).
  • the distance lower bound can be negative, in which case it is the opposite of the penetration depth (the security margin is taken into account).

This leads to one strange thing. With the current changes, when the distance lower bound is inferior to the break distance, we don't have ||AB|| = distance_lower_bound but ||AB|| = distance_lower_bound + security_margin(A and B being the nearest point). i.e. the nearest point are not on the shapes inflated bysecurity_margin/2`

Some improvements, kept for later:

  • For BVH, the minimal distance between two bounding volumes could be decrease as the algo finds smaller distance lower bound. This is especially useful when the break_distance is set to a high value.
  • Rename break_distance to something easier to understand from the user perspective.
  • Clean API of CollisionTraversalNode:
    • remove ShapeCollisionTraversalNode,
    • merge the two CollisionTraversalNode::BVDisjoint prototype into one. The squared distance lower bound attribute is not useless.
  • The above API should be enforced by some unit-tests.

@jmirabel
Copy link
Contributor Author

This PR is ready for reviews.

src/collision.cpp Outdated Show resolved Hide resolved
this->result->addContact(Contact(this->model1, this->model2,
Contact::NONE, primitive_id,
.5 * (c1+c2), (c2-c1).normalized (),
-distance));
Copy link
Contributor Author

Choose a reason for hiding this comment

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

TODO: sometimes the contact distance takes the security margin into account. Sometimes it does not. It should be homogenized.

Copy link
Contributor

Choose a reason for hiding this comment

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

Why to not apply the change here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I plan to do it. I wrote it so as not to forget.

Copy link
Contributor

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

Maybe, this should fix another bug

src/collision_func_matrix.cpp Show resolved Hide resolved
@jcarpent
Copy link
Contributor

jcarpent commented Feb 8, 2022

@jmirabel Could you consider adjusting this PR for merging it?

@jmirabel
Copy link
Contributor Author

Not sure what you are asking exactly. I can definitely resolve the git conflicts quickly. Adding unit test, finalizing the code and maybe update the API is another story. I know I won't have time before beginning of March. Then I am not sure yet.

@jmirabel jmirabel self-assigned this Feb 15, 2022
@jmirabel
Copy link
Contributor Author

I assigned myself so that I see this in my list of assigned PRs.

@@ -60,7 +60,13 @@ void collide(CollisionTraversalNodeBase* node,
collisionRecurse(node, 0, 0, front_list, sqrDistLowerBound);
else
collisionNonRecurse(node, front_list, sqrDistLowerBound);
result.updateDistanceLowerBound (sqrt (sqrDistLowerBound));
Copy link
Contributor

Choose a reason for hiding this comment

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

@jmirabel Why do you remove that here?

jcarpent added a commit to jcarpent/hpp-fcl that referenced this pull request Jun 4, 2022
jcarpent added a commit to jcarpent/hpp-fcl that referenced this pull request Jun 4, 2022
Not sure yet if the change are all correct.
That's why, I decided to split with respect to the content of coal-library#200.
@jcarpent
Copy link
Contributor

jcarpent commented Jun 4, 2022

Supersedes by #303.

@jcarpent jcarpent closed this Jun 4, 2022
jcarpent added a commit to jcarpent/hpp-fcl that referenced this pull request Jun 5, 2022
jcarpent added a commit to jcarpent/hpp-fcl that referenced this pull request Jun 5, 2022
Not sure yet if the change are all correct.
That's why, I decided to split with respect to the content of coal-library#200.
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.

3 participants