{"action":"created","issue":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/1547","labels_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/1547/labels{/name}","comments_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/1547/comments","events_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/1547/events","html_url":"https://github.com/RobotLocomotion/drake/pull/1547","id":123538706,"number":1547,"title":"restores full runLCM capabilities using the new C++ system concepts","user":{"login":"RussTedrake","id":6442292,"avatar_url":"https://avatars.githubusercontent.com/u/6442292?v=3","gravatar_id":"","url":"https://api.github.com/users/RussTedrake","html_url":"https://github.com/RussTedrake","followers_url":"https://api.github.com/users/RussTedrake/followers","following_url":"https://api.github.com/users/RussTedrake/following{/other_user}","gists_url":"https://api.github.com/users/RussTedrake/gists{/gist_id}","starred_url":"https://api.github.com/users/RussTedrake/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/RussTedrake/subscriptions","organizations_url":"https://api.github.com/users/RussTedrake/orgs","repos_url":"https://api.github.com/users/RussTedrake/repos","events_url":"https://api.github.com/users/RussTedrake/events{/privacy}","received_events_url":"https://api.github.com/users/RussTedrake/received_events","type":"User","site_admin":false},"labels":[{"url":"https://api.github.com/repos/RobotLocomotion/drake/labels/waiting%20for%20code%20review","name":"waiting for code review","color":"d4c5f9"}],"state":"open","locked":false,"assignee":null,"milestone":null,"comments":9,"created_at":"2015-12-22T19:06:50Z","updated_at":"2015-12-23T20:03:30Z","closed_at":null,"pull_request":{"url":"https://api.github.com/repos/RobotLocomotion/drake/pulls/1547","html_url":"https://github.com/RobotLocomotion/drake/pull/1547","diff_url":"https://github.com/RobotLocomotion/drake/pull/1547.diff","patch_url":"https://github.com/RobotLocomotion/drake/pull/1547.patch"},"body":"\r\nAddresses many of the issues in #1522. Zaps the CRTP pattern the system hierarchy in favor of a completely flat System \"concept\", which is \"modeled\" by the specific system implementations we have. I went through many iterations, but am finally happy with this.\r\n\r\nIn addition to updating all of the c++ examples, I've started the C++ doxygen interface documentation. I used the recommendation here for documenting the concepts. It's not perfect, but will do for now.\r\n\r\nAlso restores the full runLCM capabilities.  Note that everything lcm related is encapsulated in LCMSystem.h and LCMSystem.cpp (as it should be).\r\n\r\n\r\n"},"comment":{"url":"https://api.github.com/repos/RobotLocomotion/drake/issues/comments/166980992","html_url":"https://github.com/RobotLocomotion/drake/pull/1547#issuecomment-166980992","issue_url":"https://api.github.com/repos/RobotLocomotion/drake/issues/1547","id":166980992,"user":{"login":"rdeits","id":591886,"avatar_url":"https://avatars.githubusercontent.com/u/591886?v=3","gravatar_id":"","url":"https://api.github.com/users/rdeits","html_url":"https://github.com/rdeits","followers_url":"https://api.github.com/users/rdeits/followers","following_url":"https://api.github.com/users/rdeits/following{/other_user}","gists_url":"https://api.github.com/users/rdeits/gists{/gist_id}","starred_url":"https://api.github.com/users/rdeits/starred{/owner}{/repo}","subscriptions_url":"https://api.github.com/users/rdeits/subscriptions","organizations_url":"https://api.github.com/users/rdeits/orgs","repos_url":"https://api.github.com/users/rdeits/repos","events_url":"https://api.github.com/users/rdeits/events{/privacy}","received_events_url":"https://api.github.com/users/rdeits/received_events","type":"User","site_admin":false},"created_at":"2015-12-23T20:03:29Z","updated_at":"2015-12-23T20:03:29Z","body":"Okay, here's a radically different approach: https://github.com/rdeits/simple-systems/blob/master/src/systems.cpp\r\n\r\nIt's not nearly as well thought-out or developed as any of the ideas we've been working with, and it has obvious disadvantages, but I think it might be worth considering. The governing idea is a simple, concrete type system. System is an abstract class with virtual methods defined for each type we might want to send through:\r\n\r\n```c++\r\nclass System {\r\npublic:\r\n  virtual void dynamics(const Frame<double> &x, Frame<double> &xdot) const = 0;\r\n  virtual void dynamics(const Frame<int> &x, Frame<int> &xdot) const = 0;\r\n};\r\n```\r\n\r\n(here I'm using `double` and `int` as placeholders for `double`, `AutoDiffScalar`, etc). I've also only implemented `dynamics`, but adding `output` and `update`, etc. should be straightforward. \r\n\r\nChild systems must then implement each version of `dynamics`, which is annoying. But it's also incredibly easy to do with a template and a very simple macro:\r\n\r\n```c++\r\n#define DYNAMICS_DISPATCH_BOILERPLATE \\\r\n  void dynamics(const Frame<double> &x, Frame<double> &xdot) const { \\\r\n    dynamics<double>(x, xdot); \\\r\n  } \\\r\n  void dynamics(const Frame<int> &x, Frame<int> &xdot) const { \\\r\n    dynamics<int>(x, xdot); \\\r\n  } \\\r\n\r\n\r\nclass ExampleChildSystem : public System {\r\npublic:\r\n  template <typename Scalar> \r\n  void dynamics(const Frame<Scalar> &x, Frame<Scalar> &xdot) const {\r\n    xdot.setValue(xdot.getIndex(\"q\"), x.getValue(x.getIndex(\"qdot\")));\r\n    xdot.setValue(xdot.getIndex(\"qdot\"), 1);\r\n  }\r\n\r\n  DYNAMICS_DISPATCH_BOILERPLATE\r\n};\r\n```\r\n\r\nAdding a new type to all Drake Systems then only requires a one-line addition to the System class and a couple of lines in the `DYNAMICS_DISPATCH_BOILERPLATE` macro. \r\n\r\nThe other part of the implementation is a Frame class, templated on the scalar type: \r\n\r\n```c++\r\ntemplate <typename Scalar>\r\nclass Frame {\r\npublic:\r\n  virtual Scalar getValue(const int index) const = 0;\r\n  virtual void setValue(const int index, const Scalar &value) = 0;\r\n  virtual int getIndex(const char * str) const = 0;\r\n  virtual ~Frame() { }\r\n  virtual Frame* clone() const = 0;\r\n};\r\n```\r\n\r\nThe obvious disadvantage here is that index lookup by name must happen at runtime, which will certainly be slower than accessing a field in a struct. But, if the set of names is known at compile-time, then we can use constexpr metaprogramming to do most of the lookup work at compile-time:\r\n\r\n```c++\r\nconstexpr bool static_strequal(const char * a, const char * b) {\r\n  return (*a == 0 && *b == 0) ? true :\r\n         (*a == 0) ? false :\r\n         (*b == 0) ? false : \r\n         (*a != *b) ? false :\r\n         static_strequal(a + 1, b + 1);\r\n}\r\n\r\nstatic constexpr int getIndexStatic(const char *name) {\r\n    return static_strequal(name, \"q\") ? 0 : \r\n           static_strequal(name, \"qdot\") ? 1 :\r\n           -1;\r\n  }\r\n``` \r\n\r\nIf the set of names is not known at compile-time (such as when a user loads a brand-new URDF), then `getIndex` can be implemented with an `unordered_map`. \r\n\r\nAccessing a value by name is also very verbose in this implementation: \r\n\r\n```c++\r\nxdot.setValue(xdot.getIndex(\"qdot\"), 1);\r\n```\r\n\r\nbut that's very easily fixed with some appropriate operator overloads. \r\n\r\nThe other big disadvantage is that checking the types and sizes of frames for compatibility must be done at run-time (and isn't implemented here). That's another performance penalty. But, again, I have a suspicion that constexpr might let us avoid some of the penalty. \r\n\r\nThe advantage is that we end up with a finite (and small) set of types that can be used from a wrapper language in exactly the same way they're used in C++. The disadvantage is performance and the loss of compile-time verification of frame types. \r\n\r\nAny thoughts? "}}