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.

Sign up for GitHub

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

Jump to bottom

Setting default parameters for the narrowphase #531

Merged
merged 16 commits into from
Feb 9, 2024
Merged

Setting default parameters for the narrowphase #531

merged 16 commits into from
Feb 9, 2024

Conversation

lmontaut
Copy link
Contributor

@lmontaut lmontaut commented Feb 8, 2024 •
edited
Loading

This PR is related to #380 where users are sometimes experiencing assertion errors in Debug mode.
This PR is related to one of these assertion triggers:

  • When the user sets a tolerance for GJK and/or EPA that is too low, the support function may work with support directions that are almost zero, which is not suitable for GJK and EPA. In hppfcl, we have hard-coded assertion checks, and these checks make the assumption that GJK's tolerance is not below 1e-6. This PR introduces default values for GJK and EPA and places a warning in Debug mode only for users going below this tolerance.
    Side note: when working with the default GJK and EPA (default 1e-6 tolerance, default convergence criterion), it is precise up to the micro-meter, which is an extremely low tolerance. In practice, working with 1e-4 or even 1e-3 should be more than enough.

Sorry for the long coming text.
I will use this PR to evoke the 2 main other assertions that sometimes trigger:

  • When shapes of a collision pair are extremely far apart or have extremely different dimensions (i.e., a molecule vs. a plane), the projection step in GJK may be limited due to floating point error, and assertions will be triggered. We might want to implement a "pre-conditioning" system in the future. When working with "molecule vs. plane" dimensions, it would warn the user that hppfcl is not made to work with such different dimensions. When working with "molecule vs. molecule" dimensions, it would simply scale the problem "to the meter." This pre-conditioner would simply check the dimension of the AABB of each shape.
  • When shapes of a collision pair are in "touching contact", i.e they are colliding but have a 0 penetration depth. In such a case, EPA tries to run, fails, and an assert triggers. Although this situation in "rare" in theory, it actually occurs quite often when using hppfcl for physics simulation. I am actively working on fixing this issue, so hopefully this assert will soon disappear.

@lmontaut
GJK/EPA: setting default parameters
de1a40e
@lmontaut
GJK: fix incompatible assertion check in support function
73adf8a 
Checking the max coef is incompatible with the first convergence
criterion of GJK which checks the norm of the current iterate (see first
convergence check in `GJK::evaluate` in `gjk.cpp`).
This convergence check is needed, so it's better to adapt the assertion
than the GJK check itself.
@lmontaut
GJK: fix missing "-" in assertion
f056652 
Also bumped the tolerance of the assertion because asking 1e-16 is way
too small.
@lmontaut
BVH: adding non-zero precision to assertion
ce2a371
@lmontaut
Copy link
Contributor Author

lmontaut commented Feb 8, 2024

This PR also fixes additional assertion errors in GJK and BVH thanks to tests provided by #382

@lmontaut lmontaut requested review from jorisv and jcarpent February 8, 2024 19:13
jcarpent
jcarpent previously approved these changes Feb 8, 2024
View reviewed changes
@jcarpent jcarpent enabled auto-merge February 8, 2024 20:01
jcarpent
jcarpent previously approved these changes Feb 8, 2024
View reviewed changes
@lmontaut
test/gjk_asserts: isolating the problematic cases to reduce test run
836eec1 
time

I only keep the cases that were previously triggering asserts.
Otherwise this test timeout on the CI.

All of the other cases don't trigger any assert.
jorisv
jorisv requested changes Feb 9, 2024
View reviewed changes
include/hpp/fcl/narrowphase/narrowphase_defaults.h Outdated Show resolved Hide resolved
src/collision_node.cpp Outdated Show resolved Hide resolved
src/narrowphase/gjk.cpp Outdated Show resolved Hide resolved
test/collision_node_asserts.cpp Outdated Show resolved Hide resolved
test/gjk_asserts.cpp Outdated Show resolved Hide resolved
test/collision_node_asserts.cpp Outdated Show resolved Hide resolved
test/gjk_asserts.cpp Outdated Show resolved Hide resolved
test/gjk_asserts.cpp Outdated Show resolved Hide resolved
test/gjk_asserts.cpp Outdated Show resolved Hide resolved
test/gjk_asserts.cpp Show resolved Hide resolved
@lmontaut lmontaut force-pushed the louis/topic/narrowphase-defaults branch from 2176822 to 29273e7 Compare February 9, 2024 09:55
jorisv
jorisv approved these changes Feb 9, 2024
View reviewed changes
CHANGELOG.md Outdated Show resolved Hide resolved
@jcarpent jcarpent merged commit 6462318 into coal-library:devel Feb 9, 2024
34 checks passed
@lmontaut lmontaut deleted the louis/topic/narrowphase-defaults branch February 9, 2024 16:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jorisv jorisv jorisv approved these changes

@jcarpent jcarpent jcarpent left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@lmontaut @jorisv @jcarpent