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 a guard to faceNormalPointingOutward to detect degeneracy #324

Conversation

SeanCurtis-TRI
Copy link
Contributor

@SeanCurtis-TRI SeanCurtis-TRI commented Aug 3, 2018

This adds the triangle_area_is_zero test to faceNormalPointingOutward (with assert effect).
This is a prerequisite to the isOutsidePolytopeFace test. The test is
rendered meaningless if the reported normal is defined by numerical noise.
If a scenario is failing due to the isOutsidePolytopFace assertion
failing, this will detect if it's due to face degeneracies.

relates to #322


This change is Reviewable

@SeanCurtis-TRI SeanCurtis-TRI force-pushed the PR_EPA_has_degenerate_faces branch 3 times, most recently from 1c559c2 to d2471f2 Compare August 4, 2018 20:05
@Levi-Armstrong
Copy link
Contributor

@SeanCurtis-TRI I have several test in my new environment library and when I check a box to cylinder I receive the following error. My plan is to push the test back to FCL which are checking each shape to another with different conditions.

The box is 1m x 1m x 1m at <0,0,0>.
The cylinder is <0.25,0.25> at <0.2,0,0>

tesseract_collision_box_cylinder_unit: /home/larmstrong/catkin_ws/trajopt_public_ws/src/tesseract-1/tesseract_ext/fcl_ros/fcl/include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h:848: ccd_vec3_t fcl::detail::libccd_extension::faceNormalPointingOutward(const ccd_pt_t*, const ccd_pt_face_t*): Assertion `!triangle_area_is_zero(a, b, c)' failed.

@SeanCurtis-TRI
Copy link
Contributor Author

That's good to hear. And thanks, @Levi-Armstrong for the very specific example. Nice to know we can recreate this problem without having to create a Convex. So, the problem (or at least one problem) is that we are generating degenerate faces.

@hongkai-dai this doesn't seem overly surprising. Something to look into.

Copy link
Contributor

@hongkai-dai hongkai-dai left a comment

Choose a reason for hiding this comment

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

Let's discuss this f2f tomorrow.

When we rewrote the EPA implementation, we know that it didn't handle the degenerate cases, and decided to handle degeneracy in future PRs. Now it seems like a good time to work on the future PRs.

Reviewable status: 0 of 2 files reviewed, all discussions resolved

Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

+@hongkai-dai for review please

Reviewable status: 0 of 2 files reviewed, all discussions resolved

Copy link
Contributor

@hongkai-dai hongkai-dai left a comment

Choose a reason for hiding this comment

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

:lgtm: +@sherm1 for platform review please.

Reviewed 2 of 2 files at r1.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Nice! :lgtm: with a few nits.

Reviewed 2 of 2 files at r1.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @SeanCurtis-TRI)


include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 776 at r1 (raw file):

  //
  // For dimension i, two values are considered the same if:
  //   |pᵢ - qᵢ| <= ε·max(1, |pᵢ|, |pᵢ|)

Minor: second pᵢ -> qᵢ


include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 777 at r1 (raw file):

  // For dimension i, two values are considered the same if:
  //   |pᵢ - qᵢ| <= ε·max(1, |pᵢ|, |pᵢ|)
  // And the points are coincident if the previous condition for all

Minor: condition for -> condition holds for


include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 789 at r1 (raw file):

        max({ccd_real_t{1}, abs(p.v[i]), abs(q.v[i])}) * eps;
    const ccd_real_t delta = abs(p.v[i] - q.v[i]);
    if (delta > scale) return false;

Nit: the "scale" here is scale=max(|pi|,|vi|). Then the tolerance is
tol=max(1,scale)*eps and the final condition is delta > tol. Consider writing it like that to be more precise.

Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

Done

Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @hongkai-dai and @sherm1)


include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 776 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Minor: second pᵢ -> qᵢ

Done


include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 777 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Minor: condition for -> condition holds for

Done


include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 789 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

Nit: the "scale" here is scale=max(|pi|,|vi|). Then the tolerance is
tol=max(1,scale)*eps and the final condition is delta > tol. Consider writing it like that to be more precise.

Done

@sherm1
Copy link
Member

sherm1 commented Aug 7, 2018


include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 789 at r1 (raw file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Done

No change?

Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

Reviewable status: 1 of 2 files reviewed, all discussions resolved (waiting on @hongkai-dai and @sherm1)


include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 789 at r1 (raw file):

Previously, sherm1 (Michael Sherman) wrote…

No change?

I guess my CLion doesn't autosave as much as I thought.

Copy link
Member

@sherm1 sherm1 left a comment

Choose a reason for hiding this comment

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

Reviewed 1 of 1 files at r2.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

This adds the `triangle_area_is_zero` test to `faceNormalPointingOutward`.
This is a prerequisite to the `isOutsidePolytopeFace` test. The test is
rendered meaningless if the reported normal is defined by numerical noise.
If a scenario is failing due to the `isOutsidePolytopFace` assertion
failing, this will detect if its due to face degeneracies.
Copy link
Contributor Author

@SeanCurtis-TRI SeanCurtis-TRI left a comment

Choose a reason for hiding this comment

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

@sherm1 this pr previously passed the appveyor CI. I had to rebase it because master moved (#321 merged). The appveyor CI has been queued for an hour. Shall we just go ahead and merge this?

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

@sherm1 sherm1 merged commit 9d25b53 into flexible-collision-library:master Aug 8, 2018
@SeanCurtis-TRI SeanCurtis-TRI deleted the PR_EPA_has_degenerate_faces branch August 10, 2018 16:40
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.

4 participants