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

Adding scalar-dependent tolerances to GJK solvers #279

Conversation

SeanCurtis-TRI
Copy link
Contributor

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

  1. Extends fcl::constants to have scalar-dependent tolerances
    a. Documentation
    b. Tests
  2. Modify default constructors for solvers to use default values.

Fixes #278


This change is Reviewable

@sherm1
Copy link
Member

sherm1 commented Apr 12, 2018

FYI -- CI failure looks real for test_fcl_geometric_shapes.


Review status: 0 of 5 files reviewed at latest revision, all discussions resolved, some commit checks failed.


Comments from Reviewable

@sherm1
Copy link
Member

sherm1 commented Apr 12, 2018

:lgtm: with a few minor BTWs.


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


include/fcl/math/constants.h, line 76 at r1 (raw file):

}  // namespace detail

/// A collection of scalar-dependent constants. This provides the ability to

Awesome documentation -- thanks!


include/fcl/math/constants.h, line 110 at r1 (raw file):

/// following contrast will make it clear.
/// ```
/// S local_eps = 10 * std::numeric_limit<S>::epsilon();

BTW just to be clear to people who might be skimming this comment, add a comment above this line, like:

// DON'T DO THIS!

include/fcl/math/constants.h, line 112 at r1 (raw file):

/// S local_eps = 10 * std::numeric_limit<S>::epsilon();
/// ```
/// The above example shows a common basis for defining a local epsilon. It

BTW "common basis" -> "common but incorrect"


include/fcl/math/constants.h, line 135 at r1 (raw file):

static constexpr S pi() { return S(3.141592653589793238462643383279502884197169399375105820974944592L); }

/// The golden ratio

BTW missing periods on this and previous sentence.


include/fcl/math/constants.h, line 139 at r1 (raw file):

/// Defines the default accuracy for gjk and epa tolerance. It is defined as
/// pow(epsilon, 7/8) -- where epsilon is the machine precision epsilon. This

BTW would be worth saying that this is the machine epsilon for the in-use RealType, and that it is much tighter in double -- approx 2e-14 vs 9e-7 for IEEE floating point double vs float, resp. Consider including those numbers in the comment to make it clear what this means.

Also, below you used ε rather than epsilon. Consider ε^(7/8) in comments rather than the awkward C "pow" syntax.


include/fcl/narrowphase/detail/gjk_solver_libccd-inl.h, line 906 at r1 (raw file):

  max_distance_iterations = 1000;
  collision_tolerance = constants<S>::gjk_default_tolerance();
  distance_tolerance = 1e-6;

BTW why is this one still hardcoded?


test/test_fcl_constant_eps.cpp, line 67 at r1 (raw file):

            << "Failed for " << type_name;
  EXPECT_EQ(std::pow(expected_eps, S(0.875)), constants<S>::eps_78())
            << "Failed for " << type_name;

BTW consider allowing for a small numerical error here?


test/test_fcl_constant_eps.cpp, line 118 at r1 (raw file):

  ::testing::InitGoogleTest(&argc, argv);
  return RUN_ALL_TESTS();
}

BTW missing newline at eof


Comments from Reviewable

@SeanCurtis-TRI
Copy link
Contributor Author

Review status: 3 of 6 files reviewed at latest revision, 7 unresolved discussions.


include/fcl/math/constants.h, line 76 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Awesome documentation -- thanks!

Thanks


include/fcl/math/constants.h, line 110 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW just to be clear to people who might be skimming this comment, add a comment above this line, like:

// DON'T DO THIS!

Done


include/fcl/math/constants.h, line 112 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW "common basis" -> "common but incorrect"

Done


include/fcl/math/constants.h, line 135 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW missing periods on this and previous sentence.

Done


include/fcl/math/constants.h, line 139 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW would be worth saying that this is the machine epsilon for the in-use RealType, and that it is much tighter in double -- approx 2e-14 vs 9e-7 for IEEE floating point double vs float, resp. Consider including those numbers in the comment to make it clear what this means.

Also, below you used ε rather than epsilon. Consider ε^(7/8) in comments rather than the awkward C "pow" syntax.

Done


include/fcl/narrowphase/detail/gjk_solver_libccd-inl.h, line 906 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW why is this one still hardcoded?

Because I'm not addressing distance yet and didn't want to get into distance tests until I'm good and ready.


test/test_fcl_constant_eps.cpp, line 67 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW consider allowing for a small numerical error here?

Done

Added document acknowledging the expectation that the two values should match down to the last bit.


test/test_fcl_constant_eps.cpp, line 118 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

BTW missing newline at eof

Done


Comments from Reviewable

1. Extends fcl::constants to have scalar-dependent tolerances
  a. Documentation
  b. Tests
2. Modify default constructors for solvers to use default values
3. Modify "osculating" unit test to preserve its current
   behavior which is otherwise affected by tighter tolerances.
@sherm1
Copy link
Member

sherm1 commented Apr 17, 2018

Reviewed 2 of 3 files at r2, 1 of 1 files at r3.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@SeanCurtis-TRI
Copy link
Contributor Author

@sherm1 CI failures on the "standard" failures. Merge?


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


Comments from Reviewable

@SeanCurtis-TRI
Copy link
Contributor Author

Ooops.. CI failures are the "standard" failures. Merge?


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


Comments from Reviewable

@sherm1 sherm1 merged commit 1271b65 into flexible-collision-library:master Apr 18, 2018
@SeanCurtis-TRI SeanCurtis-TRI deleted the PR_scalar_dependent_tolerances branch April 20, 2018 17:30
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