Skip to content

Conversation

@hongkai-dai
Copy link
Contributor

@hongkai-dai hongkai-dai commented Feb 28, 2025

Use the version with the witness point.
Add more tests.

This PR also depends on libccd PR danfis/libccd#81


This change is Reviewable

Use the version with the witness point.
Add more tests.
Copy link
Contributor Author

@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.

Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @SeanCurtis-TRI)


a discussion (no related file):
+@SeanCurtis-TRI for feature review please, thanks!

Copy link
Contributor

@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.

Reviewed 2 of 2 files at r1.
Reviewable status: 1 of 2 files reviewed, 3 unresolved discussions (waiting on @hongkai-dai)


a discussion (no related file):

Previously, hongkai-dai (Hongkai Dai) wrote…

+@SeanCurtis-TRI for feature review please, thanks!

Pass done. I did a refactor; ptal.

I'm not sure why the previous round of CI vanished as it did. Hopefully, this time we'll get more meaningful results.


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

  // precision numerical errors. We use the version with the witness point, to
  // get a numerical robust distance. See
  // https://github.com/danfis/libccd/issues/55 for an explanation.

I've pushed a commit to handle the overhead of this issue. Localize and centralize the patch. It also makes it easier to confirm that we haven't missed any (fewer invocations of ccdVec3PointTriDist2().


test/test_fcl_signed_distance.cpp line 1053 at r1 (raw file):

template <typename S>
void test_distance_convex_to_sphere() {

BTW I'm kind of tempted to do some refactoring. With these massive declarations, it'd be nice if we could move the data into its own build unit, and just exercise them here. Anything to reduce the bloat on this PR.

Copy link
Contributor

@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, 3 unresolved discussions (waiting on @hongkai-dai)


a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

Pass done. I did a refactor; ptal.

I'm not sure why the previous round of CI vanished as it did. Hopefully, this time we'll get more meaningful results.

(I'm deferring the stamp until we see a happy CI, or can at least justify that it is happy enough.)

Copy link
Contributor

@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.

Reviewed 2 of 2 files at r3, all commit messages.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @hongkai-dai)


a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

(I'm deferring the stamp until we see a happy CI, or can at least justify that it is happy enough.)

:LGTM: with some editorial nits.

I'm trying to decide if I should be worried about mac coverage. We want to make sure these fixes apply reasonably well to the mac as well, and this CI doesn't seem to be doing it anymore.


include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h line 214 at r3 (raw file):

  //
  // Any actual invocation of ccdVec3PointTriDist2() in the code should request
  // a witness point *or* call this invocation.

nit: Oops. In my juggling, I left these three lines in. They should be deleted.

Code quote:

  //
  // Any actual invocation of ccdVec3PointTriDist2() in the code should request
  // a witness point *or* call this invocation.

include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h line 222 at r3 (raw file):

// `ccdVec3PointTriDist2()` directly. It has some precision quirks. For those
// invocations that want the squared distance *with* a witness point, call
// *this* function.

nit: Similarly, the first two sentences are no longer valid and should be deleted.

Suggestion:

// For invocations that want the squared distance *with* a witness point,
// call *this* function.

Copy link
Contributor

@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: all files reviewed, 3 unresolved discussions (waiting on @hongkai-dai)


a discussion (no related file):

Previously, SeanCurtis-TRI (Sean Curtis) wrote…

:LGTM: with some editorial nits.

I'm trying to decide if I should be worried about mac coverage. We want to make sure these fixes apply reasonably well to the mac as well, and this CI doesn't seem to be doing it anymore.

I've given up on getting the mac CI to work. :-/

@SeanCurtis-TRI SeanCurtis-TRI merged commit 3c2b993 into flexible-collision-library:master Mar 12, 2025
5 of 8 checks passed
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