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

Capsule vs Capsule Collision Make sure the penetration depth is not zero #382

Conversation

robertocapuano
Copy link
Contributor

Sometimes assertion fails in NarrowPhaseInfoBatch::addContactPoint():
assert(penDepth > decimal(0.0));
This happens for Capsule/Capsule collision check.

Probable cause is float approssimation in
bool CapsuleVsCapsuleAlgorithm::testCollision(NarrowPhaseInfoBatch& narrowPhaseInfoBatch, uint32 batchStartIndex, uint32 batchNbItems, MemoryAllocator& /*memoryAllocator*/) ;

I inserted a double check on penetrationDepth.

if (penetrationDepth > 0)

The logic is the same of SphereVsSphereAlgorithm::testCollision().

@AthosArantes
Copy link

Is the sqrt() really needed? Why not compare the difference of closestPointsDistanceSquare and sumRadiusSquare with an epsilon instead?

@robertocapuano
Copy link
Contributor Author

I tried with
if ( fabsf(closestPointsDistanceSquare - sumRadius * sumRadius )<eps )

with different values for eps:

  • MACHINE_EPSILON
  • 1e-7

but after few seconds of simulation I always get
assert(penDepth > decimal(0.0));
in NarrowPhaseInfoBatch::addContactPoint

@AthosArantes
Copy link

After reading a second time, what about something like
std::max(sumRadius - closestPointsDistance, MACHINE_EPSILON)
in

decimal penetrationDepth = sumRadius - closestPointsDistance;

@robertocapuano
Copy link
Contributor Author

It works well, I tried it for a few minutes without errors.

@robertocapuano
Copy link
Contributor Author

Reverted the previous change and pushed the fix on this branch.

@DanielChappuis
Copy link
Owner

Thanks a lot for your pull request.
I have merged your first commit into the develop branch and refactored it a bit myself before your last two commits.
Are you able to try the develop branch with your test and see if the issue is now fixed?

@robertocapuano
Copy link
Contributor Author

Just tried it for a few minutes: it works. Thank you.

@DanielChappuis
Copy link
Owner

Just tried it for a few minutes: it works. Thank you.

Thanks a lot for your feedback. That's very helpful.

@DanielChappuis DanielChappuis added this to the v0.10.1 milestone Jun 6, 2024
@DanielChappuis DanielChappuis self-assigned this Jun 6, 2024
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