{"action":"created","issue":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/6349","repository_url":"https://api.github.com/repos/RobotLocomotion/drake","labels_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/6349/labels{/name}","comments_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/6349/comments","events_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/6349/events","html_url":"https://github.com/RobotLocomotion/drake/pull/6349","id":236319176,"number":6349,"title":"RNDF Spline lane implementation","user":{"login":"agalbachicar","id":3825465,"avatar_url":"https://avatars1.githubusercontent.com/u/3825465?v=3","gravatar_id":"","url":"https://api.github.com/users/agalbachicar","html_url":"https://github.com/agalbachicar","followers_url":"https://api.github.com/users/agalbachicar/followers","following_url":"https://api.github.com/users/agalbachicar/following{/other_user}","gists_url":"https://api.github.com/users/agalbachicar/gists{/gist_id}","starred_url":"https://api.github.com/users/agalbachicar/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/agalbachicar/subscriptions","organizations_url":"https://api.github.com/users/agalbachicar/orgs","repos_url":"https://api.github.com/users/agalbachicar/repos","events_url":"https://api.github.com/users/agalbachicar/events{/privacy}","received_events_url":"https://api.github.com/users/agalbachicar/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"liangfok","id":1388098,"avatar_url":"https://avatars3.githubusercontent.com/u/1388098?v=3","gravatar_id":"","url":"https://api.github.com/users/liangfok","html_url":"https://github.com/liangfok","followers_url":"https://api.github.com/users/liangfok/followers","following_url":"https://api.github.com/users/liangfok/following{/other_user}","gists_url":"https://api.github.com/users/liangfok/gists{/gist_id}","starred_url":"https://api.github.com/users/liangfok/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/liangfok/subscriptions","organizations_url":"https://api.github.com/users/liangfok/orgs","repos_url":"https://api.github.com/users/liangfok/repos","events_url":"https://api.github.com/users/liangfok/events{/privacy}","received_events_url":"https://api.github.com/users/liangfok/received_events","type":"User","site_admin":false},"assignees":[{"login":"liangfok","id":1388098,"avatar_url":"https://avatars3.githubusercontent.com/u/1388098?v=3","gravatar_id":"","url":"https://api.github.com/users/liangfok","html_url":"https://github.com/liangfok","followers_url":"https://api.github.com/users/liangfok/followers","following_url":"https://api.github.com/users/liangfok/following{/other_user}","gists_url":"https://api.github.com/users/liangfok/gists{/gist_id}","starred_url":"https://api.github.com/users/liangfok/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/liangfok/subscriptions","organizations_url":"https://api.github.com/users/liangfok/orgs","repos_url":"https://api.github.com/users/liangfok/repos","events_url":"https://api.github.com/users/liangfok/events{/privacy}","received_events_url":"https://api.github.com/users/liangfok/received_events","type":"User","site_admin":false}],"milestone":null,"comments":12,"created_at":"2017-06-15T21:26:55Z","updated_at":"2017-06-29T14:29:18Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/RobotLocomotion/drake/pulls/6349","html_url":"https://github.com/RobotLocomotion/drake/pull/6349","diff_url":"https://github.com/RobotLocomotion/drake/pull/6349.diff","patch_url":"https://github.com/RobotLocomotion/drake/pull/6349.patch"},"body":"This PR removes the stub implementation of `drake::maliput::rndf::SplineLane` and provides a meaningful one. It includes also a set of unit tests to validate the API. \r\n\r\nThis PR concludes the implementation of the Maliput API for RNDF.\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/robotlocomotion/drake/6349)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/comments/311984210","html_url":"https://github.com/RobotLocomotion/drake/pull/6349#issuecomment-311984210","issue_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/6349","id":311984210,"user":{"login":"liangfok","id":1388098,"avatar_url":"https://avatars3.githubusercontent.com/u/1388098?v=3","gravatar_id":"","url":"https://api.github.com/users/liangfok","html_url":"https://github.com/liangfok","followers_url":"https://api.github.com/users/liangfok/followers","following_url":"https://api.github.com/users/liangfok/following{/other_user}","gists_url":"https://api.github.com/users/liangfok/gists{/gist_id}","starred_url":"https://api.github.com/users/liangfok/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/liangfok/subscriptions","organizations_url":"https://api.github.com/users/liangfok/orgs","repos_url":"https://api.github.com/users/liangfok/repos","events_url":"https://api.github.com/users/liangfok/events{/privacy}","received_events_url":"https://api.github.com/users/liangfok/received_events","type":"User","site_admin":false},"created_at":"2017-06-29T14:29:18Z","updated_at":"2017-06-29T14:29:18Z","body":"Checkpoint.\n\n---\n\nReviewed 3 of 9 files at r4, 2 of 5 files at r5, 2 of 3 files at r6.\nReview status: 9 of 11 files reviewed at latest revision, 14 unresolved discussions.\n\n---\n\n*[a discussion](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-KnoEDayfQWvKq3LIt4j:-KnoEDayfQWvKq3LIt4k:b-xekxb9) (no related file):*\nSince `drake::maliput::rndf::Lane` only one one subclass, `drake::maliput::rndf::SplineLane`, why separate the two?\n\n---\n\n*[drake/automotive/maliput/rndf/segment.h, line 28 at r6](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-KnoBk8kn6x-ci8rpVAK:-KnoBk8kn6x-ci8rpVAL:b-imoo91) ([raw file](https://github.com/robotlocomotion/drake/blob/688badceb55adfd43d6ed63fbc1c318133e58313/drake/automotive/maliput/rndf/segment.h#L28)):*\n> ```objc\n> \n>   /// Constructs a new Segment.\n>   /// @param This segment's ID.\n> ```\n\nMissing \"id\" after \"@param\".\n\n---\n\n*[drake/automotive/maliput/rndf/segment.h, line 29 at r6](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-KnoBnJE7AhF9Yhm4Gu0:-KnoBnJE7AhF9Yhm4Gu1:bggu6w2) ([raw file](https://github.com/robotlocomotion/drake/blob/688badceb55adfd43d6ed63fbc1c318133e58313/drake/automotive/maliput/rndf/segment.h#L29)):*\n> ```objc\n>   /// Constructs a new Segment.\n>   /// @param This segment's ID.\n>   /// @param The api::Junction that contains this Segment. It must remain valid\n> ```\n\nMissing \"junction\" after \"@param\".\n\n---\n\n*[drake/automotive/maliput/rndf/segment.h, line 30 at r6](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-KnoBs_dQVw0HKM0lKV5:-KnoBs_dQVw0HKM0lKV6:b-z2t5qq) ([raw file](https://github.com/robotlocomotion/drake/blob/688badceb55adfd43d6ed63fbc1c318133e58313/drake/automotive/maliput/rndf/segment.h#L30)):*\n> ```objc\n>   /// @param This segment's ID.\n>   /// @param The api::Junction that contains this Segment. It must remain valid\n>   /// for the lifetime of this class.\n> ```\n\nLet's change \"class\" to \"object\".\n\n---\n\n*[drake/automotive/maliput/rndf/segment.h, line 36 at r6](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-KnoBwbTMu3eal0Wjq-A:-KnoBwbTMu3eal0Wjq-B:b-imoo91) ([raw file](https://github.com/robotlocomotion/drake/blob/688badceb55adfd43d6ed63fbc1c318133e58313/drake/automotive/maliput/rndf/segment.h#L36)):*\n> ```objc\n>   /// Gives the segment a newly constructed SplineLane.\n>   ///\n>   /// @param The lane's ID.\n> ```\n\nMissing \"id\" after \"@param\".\n\n---\n\n*[drake/automotive/maliput/rndf/spline_lane.cc, line 182 at r3](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-Kna6j262GJ6c5wAcFZU:-KnoAZWr-Hk9RlmMF905:b-b0oqgs) ([raw file](https://github.com/robotlocomotion/drake/blob/efab03078d1cae4b07c0dea414d2cddd516bd6ea/drake/automotive/maliput/rndf/spline_lane.cc#L182)):*\n<details><summary><i>Previously, agalbachicar (Agustin Alba Chicar) wrote…</i></summary><blockquote>\n\nDone. AFAIK it's just line intersection in 3-D space. However as I do not support all the cases because it has no sense to do it for collinear and coincident cases and math should serve the purpose I'm not creating a specific math method for it. If you consider this should be done please let me know.\n</blockquote></details>\n\nThanks. I'm satisfied with the latest revision.\n\n---\n\n*[drake/automotive/maliput/rndf/spline_lane.cc, line 126 at r5](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-KneSp8DNQ_QZZct-kec:-KneSp8DNQ_QZZct-ked:b-wc5bhi) ([raw file](https://github.com/robotlocomotion/drake/blob/9d8dd7a0907933a75dc193d4e776a765b3eeb327/drake/automotive/maliput/rndf/spline_lane.cc#L126)):*\nThis method needs to be called `DoDriveableBounds()` since it is more than 5 lines long:\n\n> The Function Names rule specifies that the names of “very cheap” methods may be all lower-case with underscores between words. It defines “very cheap” as a method that you wouldn’t hesitate calling from within a loop. We clarify that this method should have a time complexity of O(1) and be less than 5 lines long.\n\nhttp://drake.mit.edu/code_style_guide.html#clarifications\n\n---\n\n*[drake/automotive/maliput/rndf/spline_lane.cc, line 174 at r5](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-KnoAVDTBWOxRGxoKWxe:-KnoAVDUs0XVIHGid0fb:b-lbdio2) ([raw file](https://github.com/robotlocomotion/drake/blob/9d8dd7a0907933a75dc193d4e776a765b3eeb327/drake/automotive/maliput/rndf/spline_lane.cc#L174)):*\n> ```cpp\n>   ignition::math::Vector3d t_q = (q_ending - q);\n>   t_q.Normalize();\n>   // Checks if lines are collinear.\n> ```\n\nMissing \"the\" before \"lines\".\n\n---\n\n*[drake/automotive/maliput/rndf/spline_lane.h, line 192 at r3](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-Kn_ntKgAhYRMGQfCwNz:-Knis73XDP8o7dkkpW0_:b-1s073f) ([raw file](https://github.com/robotlocomotion/drake/blob/efab03078d1cae4b07c0dea414d2cddd516bd6ea/drake/automotive/maliput/rndf/spline_lane.h#L192)):*\n<details><summary><i>Previously, agalbachicar (Agustin Alba Chicar) wrote…</i></summary><blockquote>\n\nYes, it may have a high error.  The builder (not pull requested yet) has code to create interpolated waypoints over the segment's lanes and improve the sampling over the lanes. This will reduce the error and couple lanes by having paired waypoints. [Here](https://bitbucket.org/ekumen/terminus-simulation/issues/143/variable-driveable-bounds#comment-36927662) you have something similar to a pair of U-shaped lanes, as you can see there are several arrows at the curved parts of the lanes. This is because of waypoint interpolation from one lane to the other based on the map information. As you can see, error is decreased by doing so.\n</blockquote></details>\n\nThanks for the explanation. I'm marking myself satisfied.\n\n---\n\n*[drake/automotive/maliput/rndf/spline_lane.h, line 63 at r5](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-KneXgDIsACRSUvMihd4:-KneXgDIsACRSUvMihd5:b-v3u4dx) ([raw file](https://github.com/robotlocomotion/drake/blob/9d8dd7a0907933a75dc193d4e776a765b3eeb327/drake/automotive/maliput/rndf/spline_lane.h#L63)):*\nInstead of: \n\n> The tuple will be interpreted as with the first value as the knot and the second as the tangent.\n\nUse:\n\n> The tuple's first value is the knot point while its second value is the tangent at the knot point.\n\n---\n\n*[drake/automotive/maliput/rndf/spline_lane.h, line 87 at r5](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-Kno9nvYTc4f7xs3jYyj:-Kno9nvYTc4f7xs3jYyk:b8ixqvx) ([raw file](https://github.com/robotlocomotion/drake/blob/9d8dd7a0907933a75dc193d4e776a765b3eeb327/drake/automotive/maliput/rndf/spline_lane.h#L87)):*\n> ```objc\n>   /// Computes the length of a SplineLane created using @p control_points.\n>   /// The first value are the points and the second is the\n>   /// value at that point.\n> ```\n\nThis sentence can be deleted since the information is contained in the description of  parameter `control_points`.\n\n---\n\n*[drake/automotive/maliput/rndf/spline_lane.h, line 196 at r5](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-Kno8LCOJYuQMrKpi_VX:-Kno8LCOJYuQMrKpi_VY:b-8jkhkh) ([raw file](https://github.com/robotlocomotion/drake/blob/9d8dd7a0907933a75dc193d4e776a765b3eeb327/drake/automotive/maliput/rndf/spline_lane.h#L196)):*\n> ```objc\n>   // at (s, 0, 0).\n>   // Aborts if lane_id does not refer to a valid lane ID for the lane's segment.\n>   // Aborts the lane referred by @p lane_id has no intersection with this lane,\n> ```\n\nMissing \"if\" after \"Aborts\".\n\n---\n\n*[drake/automotive/maliput/rndf/spline_lane.h, line 196 at r5](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-Kno8Q34QHaIhY6GxDXR:-Kno8Q35oSR62zotpfwf:bj3qfgx) ([raw file](https://github.com/robotlocomotion/drake/blob/9d8dd7a0907933a75dc193d4e776a765b3eeb327/drake/automotive/maliput/rndf/spline_lane.h#L196)):*\n> ```objc\n>   // at (s, 0, 0).\n>   // Aborts if lane_id does not refer to a valid lane ID for the lane's segment.\n>   // Aborts the lane referred by @p lane_id has no intersection with this lane,\n> ```\n\nShould \"this lane\" be \"this lane's normal at @p s\"?\n\n---\n\n*[drake/automotive/maliput/rndf/spline_lane.h, line 197 at r5](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-Kno8UksjhJ7G5G_izsk:-Kno8Uktbeac0mlJx_SS:b-vtjpss) ([raw file](https://github.com/robotlocomotion/drake/blob/9d8dd7a0907933a75dc193d4e776a765b3eeb327/drake/automotive/maliput/rndf/spline_lane.h#L197)):*\n> ```objc\n>   // Aborts if lane_id does not refer to a valid lane ID for the lane's segment.\n>   // Aborts the lane referred by @p lane_id has no intersection with this lane,\n>   // or it is collinear and coincident.\n> ```\n\n\"it is\" should be \"the two lines are\"\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/robotlocomotion/drake/6349#-:-KnoFMi9ynM5Xc9tSQCh:by96gdd)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"liangfok"}}