{"action":"created","issue":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/8870","repository_url":"https://api.github.com/repos/RobotLocomotion/drake","labels_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/8870/labels{/name}","comments_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/8870/comments","events_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/8870/events","html_url":"https://github.com/RobotLocomotion/drake/pull/8870","id":326164200,"number":8870,"title":"Expose RoadCurve settings in Multilane YAMLs","user":{"login":"hidmic","id":13500507,"avatar_url":"https://avatars1.githubusercontent.com/u/13500507?v=4","gravatar_id":"","url":"https://api.github.com/users/hidmic","html_url":"https://github.com/hidmic","followers_url":"https://api.github.com/users/hidmic/followers","following_url":"https://api.github.com/users/hidmic/following{/other_user}","gists_url":"https://api.github.com/users/hidmic/gists{/gist_id}","starred_url":"https://api.github.com/users/hidmic/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hidmic/subscriptions","organizations_url":"https://api.github.com/users/hidmic/orgs","repos_url":"https://api.github.com/users/hidmic/repos","events_url":"https://api.github.com/users/hidmic/events{/privacy}","received_events_url":"https://api.github.com/users/hidmic/received_events","type":"User","site_admin":false},"labels":[],"state":"open","locked":false,"assignee":{"login":"maddog-tri","id":17651181,"avatar_url":"https://avatars3.githubusercontent.com/u/17651181?v=4","gravatar_id":"","url":"https://api.github.com/users/maddog-tri","html_url":"https://github.com/maddog-tri","followers_url":"https://api.github.com/users/maddog-tri/followers","following_url":"https://api.github.com/users/maddog-tri/following{/other_user}","gists_url":"https://api.github.com/users/maddog-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/maddog-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/maddog-tri/subscriptions","organizations_url":"https://api.github.com/users/maddog-tri/orgs","repos_url":"https://api.github.com/users/maddog-tri/repos","events_url":"https://api.github.com/users/maddog-tri/events{/privacy}","received_events_url":"https://api.github.com/users/maddog-tri/received_events","type":"User","site_admin":false},"assignees":[{"login":"maddog-tri","id":17651181,"avatar_url":"https://avatars3.githubusercontent.com/u/17651181?v=4","gravatar_id":"","url":"https://api.github.com/users/maddog-tri","html_url":"https://github.com/maddog-tri","followers_url":"https://api.github.com/users/maddog-tri/followers","following_url":"https://api.github.com/users/maddog-tri/following{/other_user}","gists_url":"https://api.github.com/users/maddog-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/maddog-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/maddog-tri/subscriptions","organizations_url":"https://api.github.com/users/maddog-tri/orgs","repos_url":"https://api.github.com/users/maddog-tri/repos","events_url":"https://api.github.com/users/maddog-tri/events{/privacy}","received_events_url":"https://api.github.com/users/maddog-tri/received_events","type":"User","site_admin":false}],"milestone":null,"comments":4,"created_at":"2018-05-24T15:04:19Z","updated_at":"2018-05-30T14:52:24Z","closed_at":null,"author_association":"CONTRIBUTOR","pull_request":{"url":"https://api.github.com/repos/RobotLocomotion/drake/pulls/8870","html_url":"https://github.com/RobotLocomotion/drake/pull/8870","diff_url":"https://github.com/RobotLocomotion/drake/pull/8870.diff","patch_url":"https://github.com/RobotLocomotion/drake/pull/8870.patch"},"body":"This pull request is a follow-up to #7594. It enables proper `RoadCurve` class construction by exposing its parameters in Multilane YAML description file, propagating them from the `Loader` class, through the `Builder` class and down to the `RoadCurve` class. Specifically, both `scale_length` (i.e. minimum spatial period) and `computation_policy` parameters can now be configured. Additionally, the `linear_tolerance` present at the `RoadGeometry` class level now reaches the `RoadCurve` class too. \r\n\r\nIt's also worth noting that this addition requires a Multilane format revision update.\r\n\r\nAs a follow-up, it depends on #7594 and thus should be merged afterwards.\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/8870)\n<!-- Reviewable:end -->\n"},"comment":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/comments/393192914","html_url":"https://github.com/RobotLocomotion/drake/pull/8870#issuecomment-393192914","issue_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/8870","id":393192914,"user":{"login":"hidmic","id":13500507,"avatar_url":"https://avatars1.githubusercontent.com/u/13500507?v=4","gravatar_id":"","url":"https://api.github.com/users/hidmic","html_url":"https://github.com/hidmic","followers_url":"https://api.github.com/users/hidmic/followers","following_url":"https://api.github.com/users/hidmic/following{/other_user}","gists_url":"https://api.github.com/users/hidmic/gists{/gist_id}","starred_url":"https://api.github.com/users/hidmic/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/hidmic/subscriptions","organizations_url":"https://api.github.com/users/hidmic/orgs","repos_url":"https://api.github.com/users/hidmic/repos","events_url":"https://api.github.com/users/hidmic/events{/privacy}","received_events_url":"https://api.github.com/users/hidmic/received_events","type":"User","site_admin":false},"created_at":"2018-05-30T14:52:23Z","updated_at":"2018-05-30T14:52:23Z","author_association":"CONTRIBUTOR","body":"Comments addressed.\n\n---\n\nReview status: all files reviewed at latest revision, 8 unresolved discussions.\n\n---\n\n*[automotive/maliput/multilane/arc_road_curve.h, line 43 at r1](https://reviewable.io/reviews/robotlocomotion/drake/8870#-LDdnoxI4wNb5SvqettJ:-LDgzDIe1CEK3KEpcU-R:b-896fix) ([raw file](https://github.com/robotlocomotion/drake/blob/79cfae60c051341835d2737257eaf3e2d656cd88/automotive/maliput/multilane/arc_road_curve.h#L43)):*\n<details><summary><i>Previously, maddog-tri (Matt Marjanović) wrote…</i></summary><blockquote>\n\n`tolerance` --> `linear_tolerance`\n</blockquote></details>\n\nDone.\n\n---\n\n*[automotive/maliput/multilane/builder.cc, line 55 at r1](https://reviewable.io/reviews/robotlocomotion/drake/8870#-LDeK0Al5F2n8y5nM_6_:-LDh72tH8ZtODAzIpMuQ:brj7mxx) ([raw file](https://github.com/robotlocomotion/drake/blob/79cfae60c051341835d2737257eaf3e2d656cd88/automotive/maliput/multilane/builder.cc#L55)):*\n<details><summary><i>Previously, maddog-tri (Matt Marjanović) wrote…</i></summary><blockquote>\n\nConstructor param order is `linear_tolerance, scale_length, computation_policy`!\n\n(Why was this not caught by a unit-test?)\n</blockquote></details>\n\nArgh! To be honest, it has happened to me before. Tests are not using getters to check for proper construction, just accuracy, having `scale_length` and `linear_tolerance` parameters reversed makes for a large tolerance but a tiny maximum step, which for most cases seems to be either enough or way above for our accuracy needs. I'm adding tests everywhere to check for proper construction (which means more getters).\n\n---\n\n*[automotive/maliput/multilane/builder.cc, line 73 at r1](https://reviewable.io/reviews/robotlocomotion/drake/8870#-LDeKGtO3TKi-2lJZJIA:-LDh7iaL8t6WGdkBxi0B:b2nreh6) ([raw file](https://github.com/robotlocomotion/drake/blob/79cfae60c051341835d2737257eaf3e2d656cd88/automotive/maliput/multilane/builder.cc#L73)):*\n<details><summary><i>Previously, maddog-tri (Matt Marjanović) wrote…</i></summary><blockquote>\n\nConstructor param order is `linear_tolerance, scale_length, computation_policy`!\n\n(Why was this not caught by a unit-test?)\n</blockquote></details>\n\nSame as above.\n\n---\n\n*[automotive/maliput/multilane/line_road_curve.h, line 41 at r1](https://reviewable.io/reviews/robotlocomotion/drake/8870#-LDdnisK0YVZVn8MX7EM:-LDgzhVj14sZSUqDNjEc:b-896fix) ([raw file](https://github.com/robotlocomotion/drake/blob/79cfae60c051341835d2737257eaf3e2d656cd88/automotive/maliput/multilane/line_road_curve.h#L41)):*\n<details><summary><i>Previously, maddog-tri (Matt Marjanović) wrote…</i></summary><blockquote>\n\n`tolerance` --> `linear_tolerance`\n</blockquote></details>\n\nDone.\n\n---\n\n*[automotive/maliput/multilane/loader.cc, line 306 at r1](https://reviewable.io/reviews/robotlocomotion/drake/8870#-LDeOhXZ4QZjyikR9yDK:-LDh1RxHEIXVnZxf6OaO:b-lfdw0) ([raw file](https://github.com/robotlocomotion/drake/blob/79cfae60c051341835d2737257eaf3e2d656cd88/automotive/maliput/multilane/loader.cc#L306)):*\n<details><summary><i>Previously, maddog-tri (Matt Marjanović) wrote…</i></summary><blockquote>\n\nThe yaml-cpp documentation is really poor.... Does this actually do what you claim it does?  (Which is, as I understand it, to set `scale_length` to `kDefaultScaleLength` if no `scale_length` key has been provided in the yaml file)\n\n * Are you sure this doesn't throw an exception if no `scale_length` key is in the yaml file?\n * Are you sure this doesn't fall back to the default value if the `scale_length` key has an unparseable value?\n   (In that case, it would be correct if it *does* throw an exception.)\n\nBetter yet:  just don't provide a default value.  Update the few existing multilane-YAML files with an explicit value, and require an explicit `scale_length` value in every YAML file.\n</blockquote></details>\n\n[Here](https://github.com/jbeder/yaml-cpp/blob/master/include/yaml-cpp/node/impl.h#L85) and [here](https://github.com/jbeder/yaml-cpp/blob/master/include/yaml-cpp/node/impl.h#L372) are the relevant snippets. It looks like:\n\n- It does provide the given argument as a default.\n- It doesn't throw if the key is missing.\n- It doesn't throw if the value is ill-formed/unparseable.\n\nNo problem with dropping defaults, it was a mere attempt to provide backwards compatibility for Multilane YAMLs. But if you're OK with changing them, I'll just do that.\n\n---\n\n*[automotive/maliput/multilane/loader.cc, line 315 at r1](https://reviewable.io/reviews/robotlocomotion/drake/8870#-LDeQUbQ3Ix3Mej1gjT4:-LDh2fB55hd9z7vh1AbB:b-hmc2he) ([raw file](https://github.com/robotlocomotion/drake/blob/79cfae60c051341835d2737257eaf3e2d656cd88/automotive/maliput/multilane/loader.cc#L315)):*\n<details><summary><i>Previously, maddog-tri (Matt Marjanović) wrote…</i></summary><blockquote>\n\nAs with scale_length:   \"Better yet:  just don't provide a default value.  Update the few existing multilane-YAML files with an explicit value, and require an explicit `computation_policy` value in every YAML file.\"\n</blockquote></details>\n\nAlright, no problem.\n\n---\n\n*[automotive/maliput/multilane/test/multilane_loader_test.cc, line 253 at r1](https://reviewable.io/reviews/robotlocomotion/drake/8870#-LDeUyORCLU5mDMzDvJM:-LDh2jm26aY6W4PMNOtG:b6h2leq) ([raw file](https://github.com/robotlocomotion/drake/blob/79cfae60c051341835d2737257eaf3e2d656cd88/automotive/maliput/multilane/test/multilane_loader_test.cc#L253)):*\n<details><summary><i>Previously, maddog-tri (Matt Marjanović) wrote…</i></summary><blockquote>\n\nNeither of these unit-tests exercises the new `scale_length` and `computation_policy` values in the YAML.  \n\n(...another reason not to provide defaults in the loader.  In fact, the loader should just be doing YAML translation into the Builder API.  If the Builder doesn't provide defaults, then neither should the Loader.)\n</blockquote></details>\n\nTrue. Tests augmented.\n\n---\n\n*[automotive/maliput/multilane/test/multilane_segments_test.cc, line 42 at r1](https://reviewable.io/reviews/robotlocomotion/drake/8870#-LDeZ6V36kwrgRVnl9Xl:-LDh5KL0769rLakDj6-g:b-m0eg1a) ([raw file](https://github.com/robotlocomotion/drake/blob/79cfae60c051341835d2737257eaf3e2d656cd88/automotive/maliput/multilane/test/multilane_segments_test.cc#L42)):*\n<details><summary><i>Previously, maddog-tri (Matt Marjanović) wrote…</i></summary><blockquote>\n\nWhat ensures that the linear-tolerance of a RoadCurve (e.g., the value set in this constructor here) matches the the linear-tolerance of the RoadGeometry which it gets attached to?\n\nThe Segment constructor needs an assertion for this.\n</blockquote></details>\n\nVery good point. Added.\n\n---\n\n*[automotive/test/pure_pursuit_test.cc, line 39 at r1](https://reviewable.io/reviews/robotlocomotion/drake/8870#-LDeXi6-7SlDGeqWiWUL:-LDh6GUo6w6KfcfzOAj_:b-896fix) ([raw file](https://github.com/robotlocomotion/drake/blob/79cfae60c051341835d2737257eaf3e2d656cd88/automotive/test/pure_pursuit_test.cc#L39)):*\n<details><summary><i>Previously, maddog-tri (Matt Marjanović) wrote…</i></summary><blockquote>\n\nBTW:  Now that you have named constants, you can take out the two comments, e.g. `/* linear_tolerance */`.\n</blockquote></details>\n\nDone.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io/reviews/robotlocomotion/drake/8870#-:-LDlX4gq1ZhyblnXw8SD:bel95ya)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"hidmic"}}