{"action":"created","issue":{"url":"https://api.github.com/repos/flexible-collision-library/fcl/issues/290","repository_url":"https://api.github.com/repos/flexible-collision-library/fcl","labels_url":"https://api.github.com/repos/flexible-collision-library/fcl/issues/290/labels{/name}","comments_url":"https://api.github.com/repos/flexible-collision-library/fcl/issues/290/comments","events_url":"https://api.github.com/repos/flexible-collision-library/fcl/issues/290/events","html_url":"https://github.com/flexible-collision-library/fcl/pull/290","id":325547311,"number":290,"title":"Fix \"dir\" in GJKDistance function","user":{"login":"hongkai-dai","id":6314000,"avatar_url":"https://avatars2.githubusercontent.com/u/6314000?v=4","gravatar_id":"","url":"https://api.github.com/users/hongkai-dai","html_url":"https://github.com/hongkai-dai","followers_url":"https://api.github.com/users/hongkai-dai/followers","following_url":"https://api.github.com/users/hongkai-dai/following{/other_user}","gists_url":"https://api.github.com/users/hongkai-dai/gists{/gist_id}","starred_url":"https://api.github.com/users/hongkai-dai/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hongkai-dai/subscriptions","organizations_url":"https://api.github.com/users/hongkai-dai/orgs","repos_url":"https://api.github.com/users/hongkai-dai/repos","events_url":"https://api.github.com/users/hongkai-dai/events{/privacy}","received_events_url":"https://api.github.com/users/hongkai-dai/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"SeanCurtis-TRI","id":22159799,"avatar_url":"https://avatars3.githubusercontent.com/u/22159799?v=4","gravatar_id":"","url":"https://api.github.com/users/SeanCurtis-TRI","html_url":"https://github.com/SeanCurtis-TRI","followers_url":"https://api.github.com/users/SeanCurtis-TRI/followers","following_url":"https://api.github.com/users/SeanCurtis-TRI/following{/other_user}","gists_url":"https://api.github.com/users/SeanCurtis-TRI/gists{/gist_id}","starred_url":"https://api.github.com/users/SeanCurtis-TRI/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/SeanCurtis-TRI/subscriptions","organizations_url":"https://api.github.com/users/SeanCurtis-TRI/orgs","repos_url":"https://api.github.com/users/SeanCurtis-TRI/repos","events_url":"https://api.github.com/users/SeanCurtis-TRI/events{/privacy}","received_events_url":"https://api.github.com/users/SeanCurtis-TRI/received_events","type":"User","site_admin":false},"assignees":[{"login":"SeanCurtis-TRI","id":22159799,"avatar_url":"https://avatars3.githubusercontent.com/u/22159799?v=4","gravatar_id":"","url":"https://api.github.com/users/SeanCurtis-TRI","html_url":"https://github.com/SeanCurtis-TRI","followers_url":"https://api.github.com/users/SeanCurtis-TRI/followers","following_url":"https://api.github.com/users/SeanCurtis-TRI/following{/other_user}","gists_url":"https://api.github.com/users/SeanCurtis-TRI/gists{/gist_id}","starred_url":"https://api.github.com/users/SeanCurtis-TRI/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/SeanCurtis-TRI/subscriptions","organizations_url":"https://api.github.com/users/SeanCurtis-TRI/orgs","repos_url":"https://api.github.com/users/SeanCurtis-TRI/repos","events_url":"https://api.github.com/users/SeanCurtis-TRI/events{/privacy}","received_events_url":"https://api.github.com/users/SeanCurtis-TRI/received_events","type":"User","site_admin":false}],"milestone":null,"comments":6,"created_at":"2018-05-23T04:42:32Z","updated_at":"2018-05-24T15:47:12Z","closed_at":null,"author_association":"NONE","pull_request":{"url":"https://api.github.com/repos/flexible-collision-library/fcl/pulls/290","html_url":"https://github.com/flexible-collision-library/fcl/pull/290","diff_url":"https://github.com/flexible-collision-library/fcl/pull/290.diff","patch_url":"https://github.com/flexible-collision-library/fcl/pull/290.patch"},"body":"The variable `dir` is overloaded in `GJKDistance` function, that it means both the point within the simplex that is closest to the origin, and the directional vector along which to compute the support mapping. One issue caused by this overload, is that when calling `extractClosestPoint`, we should pass in the closest point, but instead passed in the directional vector. In this PR I introduce a separate variable `closest_p` to represent the closest point.\n\n<!-- Reviewable:start -->\n---\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/flexible-collision-library/fcl/290)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/flexible-collision-library/fcl/issues/comments/391764019","html_url":"https://github.com/flexible-collision-library/fcl/pull/290#issuecomment-391764019","issue_url":"https://api.github.com/repos/flexible-collision-library/fcl/issues/290","id":391764019,"user":{"login":"SeanCurtis-TRI","id":22159799,"avatar_url":"https://avatars3.githubusercontent.com/u/22159799?v=4","gravatar_id":"","url":"https://api.github.com/users/SeanCurtis-TRI","html_url":"https://github.com/SeanCurtis-TRI","followers_url":"https://api.github.com/users/SeanCurtis-TRI/followers","following_url":"https://api.github.com/users/SeanCurtis-TRI/following{/other_user}","gists_url":"https://api.github.com/users/SeanCurtis-TRI/gists{/gist_id}","starred_url":"https://api.github.com/users/SeanCurtis-TRI/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/SeanCurtis-TRI/subscriptions","organizations_url":"https://api.github.com/users/SeanCurtis-TRI/orgs","repos_url":"https://api.github.com/users/SeanCurtis-TRI/repos","events_url":"https://api.github.com/users/SeanCurtis-TRI/events{/privacy}","received_events_url":"https://api.github.com/users/SeanCurtis-TRI/received_events","type":"User","site_admin":false},"created_at":"2018-05-24T15:47:11Z","updated_at":"2018-05-24T15:47:11Z","author_association":"CONTRIBUTOR","body":"Second pass\n\n---\n\nReviewed 1 of 3 files at r2.\nReview status: all files reviewed at latest revision, 7 unresolved discussions, some commit checks failed.\n\n---\n\n*[include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 1157 at r2](https://reviewable.io/reviews/flexible-collision-library/fcl/290#-LDHOkq9DqlFcPJA0IuZ:-LDHOkq9DqlFcPJA0Iu_:b-p8cuvt) ([raw file](https://github.com/flexible-collision-library/fcl/blob/c6a4ba73942e9fef8d289d37d9f6d22237d5d4bf/include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h#L1157)):*\nBTW I have a bunch of concerns about this documentation\n\n1. p1 and p2 are expressed in which frame? (Comment applies below as well).\n2. The return value is undocumented\n3. The relationship between the objects and the simplex isn't explained.\n4. The semantics of the simplex isn't clear -- is it properly initialized upon invocation? Or is it an output value?\n\nI know it's asking a lot, but if this were elevated to a Drake-quality piece of documentation, we'd all benefit.\n\nSomething along the lines of:\n\n> Computes the distance between two non-penetrating convex objects, `obj1` and `obj2` based on the warm-started witness `simplex`, returning the distance and nearest points on each object.\n@param obj1     The first convex object.\n@param obj2     The second convex object.\n@param ccd      The libccd configuration.\n@param simplex   A witness to the objects' separation generated by the GJK algorithm. NOTE: the simplex is not necessarily sufficiently refined to report the *actual* distance and may be further refined in this method.\n@param p1       If the objects are non-penetrating, the point on the surface of `obj1` closest to `obj2` (expressed in the world frame).\n@param p2       If the objects are non-penetrating, the point on the surface of `obj2` closest to `obj1` (expressed in the world frame).\n@returns The minimum distance between the two objects. If they are penetrating, -1 is returned.\n\n(With appropriate `//` injected at appropriate line width, of course.)\n\nUnknowns: What happens if the simplex is not initialized? Or it's lying?\n\n---\n\n*[include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h, line 1339 at r2](https://reviewable.io/reviews/flexible-collision-library/fcl/290#-LDHP_pY4uPAbZtKqK_B:-LDHP_pY4uPAbZtKqK_C:b-t1mls8) ([raw file](https://github.com/flexible-collision-library/fcl/blob/c6a4ba73942e9fef8d289d37d9f6d22237d5d4bf/include/fcl/narrowphase/detail/convexity_based_algorithm/gjk_libccd-inl.h#L1339)):*\n> ```objc\n> \n> \n> // Computes the (unsigned) distance between two geometric objects @p obj1 and @p\n> ```\n\nBTW I'd modify this to reflect the comment on the other function. The difference between the two is that this does the warm starting of the libccd GJK implementation.\n\n---\n\n*[test/test_fcl_sphere_sphere.cpp, line 49 at r2](https://reviewable.io/reviews/flexible-collision-library/fcl/290#-LDHjNxQBqydRx57skZ8:-LDHjNxQBqydRx57skZ9:bufypb5) ([raw file](https://github.com/flexible-collision-library/fcl/blob/c6a4ba73942e9fef8d289d37d9f6d22237d5d4bf/test/test_fcl_sphere_sphere.cpp#L49)):*\n> ```cpp\n> #include \"fcl/narrowphase/distance_result.h\"\n> \n> // For two spheres 1, 2, sphere 1 has radius1, and is centered at point A, with\n> ```\n\nFYI I just realized: if you had spheres A and B and radius_A and radius_B, that makes p_FB and p_FA fit better.\n\n---\n\n*[test/test_fcl_sphere_sphere.cpp, line 116 at r2](https://reviewable.io/reviews/flexible-collision-library/fcl/290#-LDHmRrY-eJALYDQKY-W:-LDHmRrY-eJALYDQKY-X:bynmg5s) ([raw file](https://github.com/flexible-collision-library/fcl/blob/c6a4ba73942e9fef8d289d37d9f6d22237d5d4bf/test/test_fcl_sphere_sphere.cpp#L116)):*\n> ```cpp\n>     min_distance = (sphere1.p_FC - sphere2.p_FC).norm() - sphere1.radius -\n>                    sphere2.radius;\n>     const fcl::Vector3<S> AB = (sphere1.p_FC- sphere2.p_FC).normalized();\n> ```\n\nBTW missing space before `-` operator.\n\n---\n\n*[test/test_fcl_sphere_sphere.cpp, line 117 at r2](https://reviewable.io/reviews/flexible-collision-library/fcl/290#-LDHmWN67BGJK5AFIJgm:-LDHmWN67BGJK5AFIJgn:b-xnj2ew) ([raw file](https://github.com/flexible-collision-library/fcl/blob/c6a4ba73942e9fef8d289d37d9f6d22237d5d4bf/test/test_fcl_sphere_sphere.cpp#L117)):*\n> ```cpp\n>                    sphere2.radius;\n>     const fcl::Vector3<S> AB = (sphere1.p_FC- sphere2.p_FC).normalized();\n>     p_S1P1 = -AB * sphere1.radius;\n> ```\n\nFYI *slightly* more efficient to negate the radius. But yours reads better. :)\n\n---\n\n*[test/test_fcl_sphere_sphere.cpp, line 123 at r2](https://reviewable.io/reviews/flexible-collision-library/fcl/290#-LDHmaTb0L_IXDTLuG1A:-LDHmaTb0L_IXDTLuG1B:bti55jq) ([raw file](https://github.com/flexible-collision-library/fcl/blob/c6a4ba73942e9fef8d289d37d9f6d22237d5d4bf/test/test_fcl_sphere_sphere.cpp#L123)):*\n> ```cpp\n>   SphereSpecification<S> sphere2;\n>   S min_distance;\n>   // The closest points are expressed in the sphere body frame.\n> ```\n\nBTW These three comments are slightly redundant. The first one includes the next two. So, I'd suggest one of two options:\n\n1. Delete the last two comments, or\n2. delete the first comment and then move the last comment to between the two Vector declarations (i.e., comment, object, comment, object.)\n\n---\n\n*[test/test_fcl_sphere_sphere.cpp, line 143 at r2](https://reviewable.io/reviews/flexible-collision-library/fcl/290#-LDHnHi77nFbCXuFLsT-:-LDHnHi77nFbCXuFLsT0:bcolnem) ([raw file](https://github.com/flexible-collision-library/fcl/blob/c6a4ba73942e9fef8d289d37d9f6d22237d5d4bf/test/test_fcl_sphere_sphere.cpp#L143)):*\n> ```cpp\n>     for (int j = i + 1; j < static_cast<int>(spheres.size()); ++j) {\n>       SphereSphereDistance<S> sphere_sphere_distance(spheres[i], spheres[j]);\n>       if (sphere_sphere_distance.min_distance > 0) {\n> ```\n\nBTW This is a dangerous test -- this should be a `ASSERT_GT(min_distance, 0)`. Otherwise, the spheres could be changed to be in collision and *no* testing would take place. Obviously, this will require you to guarantee that none of the sphere pairs are colliding.  :)\n\n---\n\n*[test/test_fcl_sphere_sphere.cpp, line 180 at r2](https://reviewable.io/reviews/flexible-collision-library/fcl/290#-LDEOkIVE9veSRoCuxRv:-LDHo1dD8jhNffSHE0pX:b-ngkrj5) ([raw file](https://github.com/flexible-collision-library/fcl/blob/c6a4ba73942e9fef8d289d37d9f6d22237d5d4bf/test/test_fcl_sphere_sphere.cpp#L180)):*\n<details><summary><i>Previously, sherm1 (Michael Sherman) wrote…</i></summary><blockquote>\n\nBTW Yikes! Is that really the best we can hope for with libccd?\n</blockquote></details>\n\nBTW Not all of the tests fail at this precision -- I'd like a note/todo indicating why this value is so high and which combination of spheres is ending so badly.\n\n---\n\n*[test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl.cpp, line 100 at r1](https://reviewable.io/reviews/flexible-collision-library/fcl/290#-LDD506u-7JF_tTN29Hb:-LDHoz_R0p5D86OX74ed:b-70drlg) ([raw file](https://github.com/flexible-collision-library/fcl/blob/e12e04828fc2b05cbe4457b2eaf1c12eb404f4e5/test/narrowphase/detail/convexity_based_algorithm/test_gjk_libccd-inl.cpp#L100)):*\n<details><summary><i>Previously, hongkai-dai (Hongkai Dai) wrote…</i></summary><blockquote>\n\nWithout the bug fix on `_ccdDist`, one of these checks would fail. So I think it still makes sense to test both orders here?\n</blockquote></details>\n\nRefined f2f\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io/reviews/flexible-collision-library/fcl/290#-:-LDHpG5C-Qecu2ERiShE:bbbhgqg)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"SeanCurtis-TRI"}}