{"action":"created","issue":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/3245","repository_url":"https://api.github.com/repos/RobotLocomotion/drake","labels_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/3245/labels{/name}","comments_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/3245/comments","events_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/3245/events","html_url":"https://github.com/RobotLocomotion/drake/pull/3245","id":173039901,"number":3245,"title":"RigidBodySystem 2.0","user":{"login":"amcastro-tri","id":17601461,"avatar_url":"https://avatars.githubusercontent.com/u/17601461?v=3","gravatar_id":"","url":"https://api.github.com/users/amcastro-tri","html_url":"https://github.com/amcastro-tri","followers_url":"https://api.github.com/users/amcastro-tri/followers","following_url":"https://api.github.com/users/amcastro-tri/following{/other_user}","gists_url":"https://api.github.com/users/amcastro-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/amcastro-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/amcastro-tri/subscriptions","organizations_url":"https://api.github.com/users/amcastro-tri/orgs","repos_url":"https://api.github.com/users/amcastro-tri/repos","events_url":"https://api.github.com/users/amcastro-tri/events{/privacy}","received_events_url":"https://api.github.com/users/amcastro-tri/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/RobotLocomotion/drake/labels/priority:%20high","name":"priority: high","color":"b60205"},{"url":"https://api.github.com/repos/RobotLocomotion/drake/labels/team:%20dynamics","name":"team: dynamics","color":"0052cc"}],"state":"open","locked":false,"assignee":{"login":"liangfok","id":1388098,"avatar_url":"https://avatars.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://avatars.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},{"login":"sherm1","id":4088016,"avatar_url":"https://avatars.githubusercontent.com/u/4088016?v=3","gravatar_id":"","url":"https://api.github.com/users/sherm1","html_url":"https://github.com/sherm1","followers_url":"https://api.github.com/users/sherm1/followers","following_url":"https://api.github.com/users/sherm1/following{/other_user}","gists_url":"https://api.github.com/users/sherm1/gists{/gist_id}","starred_url":"https://api.github.com/users/sherm1/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/sherm1/subscriptions","organizations_url":"https://api.github.com/users/sherm1/orgs","repos_url":"https://api.github.com/users/sherm1/repos","events_url":"https://api.github.com/users/sherm1/events{/privacy}","received_events_url":"https://api.github.com/users/sherm1/received_events","type":"User","site_admin":false},{"login":"ggould-tri","id":17596413,"avatar_url":"https://avatars.githubusercontent.com/u/17596413?v=3","gravatar_id":"","url":"https://api.github.com/users/ggould-tri","html_url":"https://github.com/ggould-tri","followers_url":"https://api.github.com/users/ggould-tri/followers","following_url":"https://api.github.com/users/ggould-tri/following{/other_user}","gists_url":"https://api.github.com/users/ggould-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/ggould-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ggould-tri/subscriptions","organizations_url":"https://api.github.com/users/ggould-tri/orgs","repos_url":"https://api.github.com/users/ggould-tri/repos","events_url":"https://api.github.com/users/ggould-tri/events{/privacy}","received_events_url":"https://api.github.com/users/ggould-tri/received_events","type":"User","site_admin":false},{"login":"amcastro-tri","id":17601461,"avatar_url":"https://avatars.githubusercontent.com/u/17601461?v=3","gravatar_id":"","url":"https://api.github.com/users/amcastro-tri","html_url":"https://github.com/amcastro-tri","followers_url":"https://api.github.com/users/amcastro-tri/followers","following_url":"https://api.github.com/users/amcastro-tri/following{/other_user}","gists_url":"https://api.github.com/users/amcastro-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/amcastro-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/amcastro-tri/subscriptions","organizations_url":"https://api.github.com/users/amcastro-tri/orgs","repos_url":"https://api.github.com/users/amcastro-tri/repos","events_url":"https://api.github.com/users/amcastro-tri/events{/privacy}","received_events_url":"https://api.github.com/users/amcastro-tri/received_events","type":"User","site_admin":false}],"milestone":null,"comments":24,"created_at":"2016-08-24T19:40:16Z","updated_at":"2016-08-26T15:07:11Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/RobotLocomotion/drake/pulls/3245","html_url":"https://github.com/RobotLocomotion/drake/pull/3245","diff_url":"https://github.com/RobotLocomotion/drake/pull/3245.diff","patch_url":"https://github.com/RobotLocomotion/drake/pull/3245.patch"},"body":"This implements the System 2.0 version of RigidBodySystem.\r\nI decided to call this RigidBodyPlant since per off-line discussion it seems this name more appropriately describes the concept of the object we actually want to control. \r\nThere are two components in RBS1 not considered here yet that also justify this name change:\r\n- Sensors.\r\n- Force elements.\r\n\r\nThere was some discussion on whether these should be their own self-contained systems wired in a Diagram with an RigidBodyPlant. Not the subject of this PR, not needed for the sprint, and therefore I am delaying to such a decision for future work. \r\n\r\nThere is a unit test in `rbs_test.cc` if you want to start there. PTAL!\r\n\r\ncc'ing @naveenoid\r\n\r\n<!-- Reviewable:start -->\r\n---\r\nThis change is [<img src=\"https://reviewable.io/review_button.svg\" height=\"34\" align=\"absmiddle\" alt=\"Reviewable\"/>](https://reviewable.io/reviews/robotlocomotion/drake/3245)\r\n<!-- Reviewable:end -->\r\n"},"comment":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/comments/242761720","html_url":"https://github.com/RobotLocomotion/drake/pull/3245#issuecomment-242761720","issue_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/3245","id":242761720,"user":{"login":"ggould-tri","id":17596413,"avatar_url":"https://avatars.githubusercontent.com/u/17596413?v=3","gravatar_id":"","url":"https://api.github.com/users/ggould-tri","html_url":"https://github.com/ggould-tri","followers_url":"https://api.github.com/users/ggould-tri/followers","following_url":"https://api.github.com/users/ggould-tri/following{/other_user}","gists_url":"https://api.github.com/users/ggould-tri/gists{/gist_id}","starred_url":"https://api.github.com/users/ggould-tri/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/ggould-tri/subscriptions","organizations_url":"https://api.github.com/users/ggould-tri/orgs","repos_url":"https://api.github.com/users/ggould-tri/repos","events_url":"https://api.github.com/users/ggould-tri/events{/privacy}","received_events_url":"https://api.github.com/users/ggould-tri/received_events","type":"User","site_admin":false},"created_at":"2016-08-26T15:07:11Z","updated_at":"2016-08-26T15:07:11Z","body":"A few quick comments.  I haven't really started proper platform review, but I read a bit before I had to get back to other issues.\n\n---\n\nReviewed 1 of 9 files at r1, 3 of 11 files at r2.\nReview status: 8 of 9 files reviewed at latest revision, 110 unresolved discussions.\n\n---\n\n*[drake/systems/framework/basic_state_vector.h, line 75 \\[r3\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/3245#-KQ6JLNtwjC9GM-4RieI:-KQ6JLNtwjC9GM-4RieJ:b-iv6v6w) ([raw file](https://github.com/robotlocomotion/drake/blob/76d931d23bc46ea4ab89d4162356ae69fe7eade2/drake/systems/framework/basic_state_vector.h#L75)):*\n> ```objc\n>   VectorX<T> CopyToVector() const override { return vector_->get_value(); }\n> \n>   // Returns a reference to the underlying Eigen expression of this vector.\n> ```\n\nFYI -- I'm not sure \"reference\" is a good word to use here, as this is not a C++ reference.  A `VectorBlock` is more nearly a \"view\".\n\n---\n\n*[drake/systems/framework/context_base.h, line 79 \\[r3\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/3245#-KQ6JmcWzUg6jRJSxGit:-KQ6JmcWzUg6jRJSxGiu:be32ykb) ([raw file](https://github.com/robotlocomotion/drake/blob/76d931d23bc46ea4ab89d4162356ae69fe7eade2/drake/systems/framework/context_base.h#L79)):*\n> ```objc\n>   virtual State<T>* get_mutable_state() = 0;\n> \n>   // Returns a mutable pointer to the continuous component ot the state vector.\n> ```\n\nFYI -- \"ot\" -> \"of\"?\n\n---\n\n*[drake/systems/framework/context_base.h, line 84 \\[r3\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/3245#-KQ6Jp_BVoBuYdyrYPT3:-KQ6Jp_BVoBuYdyrYPT4:be32ykb) ([raw file](https://github.com/robotlocomotion/drake/blob/76d931d23bc46ea4ab89d4162356ae69fe7eade2/drake/systems/framework/context_base.h#L84)):*\n> ```objc\n>   }\n> \n>   // Returns a const pointer to the continuous component ot the state vector.\n> ```\n\nFYI -- \"ot\" -> \"of\"?\n\n---\n\n*[drake/systems/plants/rigid_body_system/rigid_body_plant.cc, line 81 \\[r3\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/3245#-KQ6L7avIL5C_LDpE1Fa:-KQ6L7avIL5C_LDpE1Fb:b-c188x0) ([raw file](https://github.com/robotlocomotion/drake/blob/76d931d23bc46ea4ab89d4162356ae69fe7eade2/drake/systems/plants/rigid_body_system/rigid_body_plant.cc#L81)):*\n> ```cpp\n> \n> template <typename T>\n> void RigidBodyPlant<T>::set_position(ContextBase<T>* context,\n> ```\n\nYou must assert (or document) that this bare pointer is not null.\n\n---\n\n*[drake/systems/plants/rigid_body_system/rigid_body_plant.cc, line 82 \\[r3\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/3245#-KQ6LCckPhNky1p5BQX3:-KQ6LCckPhNky1p5BQX4:b-lc99qg) ([raw file](https://github.com/robotlocomotion/drake/blob/76d931d23bc46ea4ab89d4162356ae69fe7eade2/drake/systems/plants/rigid_body_system/rigid_body_plant.cc#L82)):*\n> ```cpp\n> template <typename T>\n> void RigidBodyPlant<T>::set_position(ContextBase<T>* context,\n>                                       int position_index, T position) const {\n> ```\n\nFYI -- Indentation of this line looks wrong.\n\n---\n\n*[drake/systems/plants/rigid_body_system/rigid_body_plant.cc, line 88 \\[r3\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/3245#-KQ6LJB6VLCeXp1tRwfy:-KQ6LJB6VLCeXp1tRwfz:b-c188x0) ([raw file](https://github.com/robotlocomotion/drake/blob/76d931d23bc46ea4ab89d4162356ae69fe7eade2/drake/systems/plants/rigid_body_system/rigid_body_plant.cc#L88)):*\n> ```cpp\n> \n> template <typename T>\n> void RigidBodyPlant<T>::set_velocity(ContextBase<T>* context,\n> ```\n\nYou must assert (or document) that this bare pointer is not null.\n\n---\n\n*[drake/systems/plants/rigid_body_system/rigid_body_plant.cc, line 89 \\[r3\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/3245#-KQ6LKOJb5Tn4O0DB2DJ:-KQ6LKOJb5Tn4O0DB2DK:b-lc99qg) ([raw file](https://github.com/robotlocomotion/drake/blob/76d931d23bc46ea4ab89d4162356ae69fe7eade2/drake/systems/plants/rigid_body_system/rigid_body_plant.cc#L89)):*\n> ```cpp\n> template <typename T>\n> void RigidBodyPlant<T>::set_velocity(ContextBase<T>* context,\n>                   int velocity_index, T velocity) const {\n> ```\n\nFYI -- Indentation of this line looks wrong.\n\n---\n\n*[drake/systems/plants/rigid_body_system/rigid_body_plant.cc, line 95 \\[r3\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/3245#-KQ6LMUGBa38H4eJOafO:-KQ6LMUGBa38H4eJOafP:b-c188x0) ([raw file](https://github.com/robotlocomotion/drake/blob/76d931d23bc46ea4ab89d4162356ae69fe7eade2/drake/systems/plants/rigid_body_system/rigid_body_plant.cc#L95)):*\n> ```cpp\n> \n> template <typename T>\n> void RigidBodyPlant<T>::set_state_vector(ContextBase<T>* context,\n> ```\n\nYou must assert (or document) that this bare pointer is not null.\n\n---\n\n*[drake/systems/plants/rigid_body_system/rigid_body_plant.cc, line 96 \\[r3\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/3245#-KQ6LNZT-6uxPRjaU-FI:-KQ6LNZUa_psydyOPjCd:b-lc99qg) ([raw file](https://github.com/robotlocomotion/drake/blob/76d931d23bc46ea4ab89d4162356ae69fe7eade2/drake/systems/plants/rigid_body_system/rigid_body_plant.cc#L96)):*\n> ```cpp\n> template <typename T>\n> void RigidBodyPlant<T>::set_state_vector(ContextBase<T>* context,\n>                       const Eigen::Ref<const VectorX<T>> x) const {\n> ```\n\nFYI -- Indentation of this line looks wrong.\n\n---\n\n*[drake/systems/plants/rigid_body_system/rigid_body_plant.cc, line 116 \\[r3\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/3245#-KQ6M-pFWBRFpD--RA5w:-KQ6M-pFWBRFpD--RA5x:b-xhyvkb) ([raw file](https://github.com/robotlocomotion/drake/blob/76d931d23bc46ea4ab89d4162356ae69fe7eade2/drake/systems/plants/rigid_body_system/rigid_body_plant.cc#L116)):*\n> ```cpp\n> template <typename T>\n> void RigidBodyPlant<T>::EvalOutput(const ContextBase<T>& context,\n>                                     SystemOutput<T>* output) const {\n> ```\n\nFYI -- Indentation looks wrong.\n\n---\n\n*[drake/systems/plants/rigid_body_system/rigid_body_plant.cc, line 129 \\[r3\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/3245#-KQ6N6rhMc0FGM8vGdpb:-KQ6N6rhMc0FGM8vGdpc:b-l42zpc) ([raw file](https://github.com/robotlocomotion/drake/blob/76d931d23bc46ea4ab89d4162356ae69fe7eade2/drake/systems/plants/rigid_body_system/rigid_body_plant.cc#L129)):*\n> ```cpp\n> template <typename T>\n> void RigidBodyPlant<T>::EvalTimeDerivatives(\n>     const ContextBase<T>& context, ContinuousState<T>* derivatives) const {\n> ```\n\nBTW -- considering the length of this method, I would strongly suggest factoring the application of joint limit forces and the application of contact forces in to separate private helper methods.\n\nGiven the urgency of this PR, I am not going to insist on this change; I think the TODO comments you have added here adequately convey the issue to future readers.\n\n---\n\n*[drake/systems/plants/rigid_body_system/rigid_body_plant.cc, line 170 \\[r3\\]](https://reviewable.io:443/reviews/robotlocomotion/drake/3245#-KQ6MLIP_2urJllLbAwQ:-KQ6MLIP_2urJllLbAwR:b-l12vy9) ([raw file](https://github.com/robotlocomotion/drake/blob/76d931d23bc46ea4ab89d4162356ae69fe7eade2/drake/systems/plants/rigid_body_system/rigid_body_plant.cc#L170)):*\n> ```cpp\n>   // TODO(amcastro-tri): Maybe move to\n>   // RBT::ComputeGeneralizedJointLimitForces(C)?\n>   {\n> ```\n\nBTW -- What is accomplished by putting this in a block?  It looks like all it does is prevent the binding of `b` from escaping this context, but I don't see any conflicting uses of `b` below (and the better solution even if it were would be to use a more descriptive variable name).\n\nOr is the block just to suggest the refactoring boundary for the TODO comment?  If so, that makes sense to me.\n\n---\n\n\n*Comments from [Reviewable](https://reviewable.io:443/reviews/robotlocomotion/drake/3245#-:-KQ6NrkXkUQdqfJetmGw:b-erno5u)*\n<!-- Sent from Reviewable.io -->\n"}}
{"actor":{"display_login":"ggould-tri"}}