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

Adds gjk tolerance to the CollisionRequest #283

Conversation

SeanCurtis-TRI
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI commented Apr 16, 2018

Sets the tolerance in the request and propagate it through to the solvers.

Only a single tolerance is exposed through the CollisionRequest interface because there is only a single tolerance value for libccd. So, for the indep the single tolerance is applied to both gjk and epa algorithms.

This PR includes:

  1. Addition of new field to CollisionRequest.
  2. Setting of the tolerance to the GJKSolver_*.
  3. Updated documentation on CollisionRequest struct.
  4. Unit test to show that setting different tolerances in the CollisionRequest produces different (better?) answers.

NOTE: This does not provide the same functionality for continuous collision request.

Fixes #280


This change is Reviewable

@SeanCurtis-TRI
Copy link
Contributor Author

+@sherm1 for review, please.


Review status: 0 of 4 files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Apr 17, 2018

Reviewed -- :lgtm: with a few BTWs.

It's worth noting that FCL's master branch doesn't currently build with float precision (not because of this PR). Are there users who want float? Should file an issue requesting confirmation that FCL should support float and soliciting a volunteer to fix it if so.

The CI failures are the usual Mac suspects; no new problems.


Reviewed 4 of 4 files at r1.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


include/fcl/narrowphase/collision_request.h, line 50 at r1 (raw file):

struct CollisionResult;

/// @brief Parameters for performing collision request

BTW period missing here and in several briefs below.


include/fcl/narrowphase/collision_request.h, line 68 at r1 (raw file):

  // are.

  /// @brief The maximum number of cost sources will return

BTW will -> we'll ? or CollisionRequest will return ?


test/test_fcl_collision.cpp, line 843 at r1 (raw file):

// answers (i.e., the tolerances are getting through). The answer based on a
// smaller tolerance may or may not be a better answer; that is the subject of
// other unit tests.

BTW Per f2f, I think you should include a comment here saying that you would have preferred to test for better answers but had trouble getting libccd to behave reasonably.


test/test_fcl_collision.cpp, line 886 at r1 (raw file):

    const Real low_tolerance = std::pow(Eigen::NumTraits<S>::epsilon(), 0.25);
    // All of the bits.
    const Real high_tolerance = Eigen::NumTraits<S>::epsilon();

BTW please consider "loose_" and "tight_tolerance" here -- "low" and "high" can be ambiguous since low is actually the larger number.

BTW "high" seems too tight for any non-trivial algorithm to achieve. I'd suggest ε^(7/8) (≈ 10⁻¹⁴ in double) as the most you could really ask for.


Comments from Reviewable

@SeanCurtis-TRI SeanCurtis-TRI force-pushed the PR_collision_request_tolerance branch from 83ce4a5 to c97fd7f Compare April 17, 2018 15:22
@SeanCurtis-TRI
Copy link
Contributor Author

Comments addressed. PTAL.


Review status: 2 of 4 files reviewed at latest revision, 4 unresolved discussions.


include/fcl/narrowphase/collision_request.h, line 50 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW period missing here and in several briefs below.

Done

For the record, I intentionally omitted periods because these aren't sentences. They are noun phrases. However, I did a bit of homework and decided these can be considered "sentence fragments" if they end with punctuation. While sentence fragments were considered a sin by my third-grade teacher, they are not incorrect in an absolute sense. So, I've added periods.


include/fcl/narrowphase/collision_request.h, line 68 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW will -> we'll ? or CollisionRequest will return ?

Done

I'd left this alone as I didn't actually know the semantics. But I'll go out on a limb for the sake of grammatical correctness.


test/test_fcl_collision.cpp, line 843 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW Per f2f, I think you should include a comment here saying that you would have preferred to test for better answers but had trouble getting libccd to behave reasonably.

Done


test/test_fcl_collision.cpp, line 886 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW please consider "loose_" and "tight_tolerance" here -- "low" and "high" can be ambiguous since low is actually the larger number.

BTW "high" seems too tight for any non-trivial algorithm to achieve. I'd suggest ε^(7/8) (≈ 10⁻¹⁴ in double) as the most you could really ask for.

Done


Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Apr 17, 2018

Reviewed 2 of 2 files at r2.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks failed.


include/fcl/narrowphase/collision_request.h, line 50 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Done

For the record, I intentionally omitted periods because these aren't sentences. They are noun phrases. However, I did a bit of homework and decided these can be considered "sentence fragments" if they end with punctuation. While sentence fragments were considered a sin by my third-grade teacher, they are not incorrect in an absolute sense. So, I've added periods.

No change to this one?


test/test_fcl_collision.cpp, line 899 at r2 (raw file):

    request.gjk_tolerance = loose_tolerance;
    fcl::CollisionResult<S> result_low;

BTW should be result_loose/tight to match previous change.


Comments from Reviewable

Sets the tolerance in the request and propagate it through to the solvers.
NOTE: This does *not* provide the same functionality for continuous
collision request.
@SeanCurtis-TRI SeanCurtis-TRI force-pushed the PR_collision_request_tolerance branch from c97fd7f to fccaeb8 Compare April 17, 2018 16:14
@SeanCurtis-TRI
Copy link
Contributor Author

Review status: 2 of 4 files reviewed at latest revision, 2 unresolved discussions, some commit checks failed.


include/fcl/narrowphase/collision_request.h, line 50 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

No change to this one?

Done

Just checking to see if you were paying attention...errr...yeah.


test/test_fcl_collision.cpp, line 899 at r2 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW should be result_loose/tight to match previous change.

Done


Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Apr 17, 2018

All good!


Reviewed 2 of 2 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@sherm1 sherm1 merged commit 7d53019 into flexible-collision-library:master Apr 17, 2018
@SeanCurtis-TRI SeanCurtis-TRI deleted the PR_collision_request_tolerance branch April 17, 2018 19:07
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.

2 participants