Addes TSR visualizer to aikido/rviz. #136

gilwoolee · 2017-03-10T23:40:10Z

This change is

coveralls · 2017-03-10T23:56:41Z

Coverage remained the same at 86.288% when pulling 48e929c on rviz/visualizeTSR into 6a94d4b on master.

mkoval

🎉 Thanks for adding this. This is very useful.

I added a few minor comments, but am happy to merge this once they are addressed.

mkoval · 2017-03-17T02:13:38Z

src/rviz/InteractiveMarkerViewer.cpp

+
+  for(int i = 0; i < nSamples && sampler->canSample(); ++i)
+  {
+    sampler->sample(state);


Check the return value of sample. In principle it should not return false if canSample() returned true, so an assertion would be fine.

mkoval · 2017-03-17T02:15:01Z

src/rviz/InteractiveMarkerViewer.cpp

+    sampler->sample(state);
+
+    std::stringstream sstm;
+    sstm << "tsr" << i;


Suggestion: Make the base name a std::string parameter. I suggest making it default to something that includes the &tsr (or, even better, the memory address of the return value) to avoid name collisions.

mkoval · 2017-03-17T02:17:15Z

src/rviz/InteractiveMarkerViewer.cpp

+    auto tsrFrame = std::make_shared<SimpleFrame>(
+      Frame::World(), sstm.str(), state.getIsometry());
+    mSimpleFrames.push_back(tsrFrame);
+    this->addFrame(tsrFrame.get());


Nit: We don't typically explicitly specify this-> unless there is a name conflict.

Also, I'd prefer to aggregate all of the FrameMarkerPtrs returned by addFrame into an RAII object (perhaps TSRMarker 😉) that we return from this function. That would also make clearTSRs and the mSimpleFrames member variable superfluous, since they could be replaced by the RAII object.

mkoval · 2017-03-17T02:17:33Z

include/aikido/rviz/InteractiveMarkerViewer.hpp

@@ -32,6 +33,10 @@ class InteractiveMarkerViewer {
  SkeletonMarkerPtr CreateSkeletonMarker(
    dart::dynamics::SkeletonPtr const &skeleton);

+  /// Visualizes tsr with at most n samples
+  void visualizeTSR(const aikido::constraint::TSR& _tsr, int nSamples = 10);


Please add a full docstring.

coveralls · 2017-03-17T02:39:15Z

Coverage remained the same at 86.288% when pulling 929f988 on rviz/visualizeTSR into 67747e1 on master.

…nto rviz/visualizeTSR

…cs/aikido into rviz/visualizeTSR

coveralls · 2017-03-22T16:04:47Z

Coverage remained the same at 85.391% when pulling a68bdae on rviz/visualizeTSR into 963a5d7 on master.

coveralls · 2017-03-27T20:34:26Z

Coverage remained the same at 85.391% when pulling 09ca6c9 on rviz/visualizeTSR into 963a5d7 on master.

coveralls · 2017-03-27T20:42:57Z

Coverage remained the same at 85.391% when pulling 09ca6c9 on rviz/visualizeTSR into 963a5d7 on master.

mkoval · 2017-03-28T03:02:07Z

src/rviz/InteractiveMarkerViewer.cpp

@@ -49,6 +50,42 @@ FrameMarkerPtr InteractiveMarkerViewer::addFrame(
  return marker;
 }

+TSRMarkerPtr InteractiveMarkerViewer::visualizeTSR(


Rename this to addTSRMarker to be consistent with addFrameMarker.

mkoval · 2017-03-28T03:02:19Z

src/rviz/InteractiveMarkerViewer.cpp

+TSRMarkerPtr InteractiveMarkerViewer::visualizeTSR(
+  aikido::constraint::TSR const &_tsr,
+  int nSamples,
+  std::string basename)


Take basename by const std::string&.

mkoval · 2017-03-28T03:02:48Z

src/rviz/InteractiveMarkerViewer.cpp

+  if (basename == "")
+  {
+      std::ostringstream ost;
+      ost << &tsrMarker;


tsrMarker is a smart_ptr, so this should be tsrMarker.get() to get the memory address of the heap memory. It's possible that repeated calls of this function in a loop could allocate tsrMarker itself at the same memory address on the stack.

mkoval · 2017-03-28T03:03:30Z

src/rviz/InteractiveMarkerViewer.cpp

+  auto state = _tsr.getSE3()->createState();
+  auto tsrMarker = std::make_shared<TSRMarker>();
+
+  if (basename == "")


Nit: basename.empty().

mkoval · 2017-03-28T03:03:55Z

src/rviz/InteractiveMarkerViewer.cpp

+    assert(sampler->sample(state));
+
+    std::stringstream ss;
+    ss << "Marker[" << basename << "].frame[" << i << "]";


Nit: Marker -> TSRMarker to avoid collisions with other markers we add in the future.

mkoval · 2017-03-28T03:05:40Z

include/aikido/rviz/TSRMarker.hpp

+  TSRMarker();
+  ~TSRMarker(){};
+
+  void addFrameMarker(FrameMarkerPtr const &marker);


I don't think this should be part of the public API. The simplest option would be to modify TSRMarker's constructor to directly accept the std::set<FrameMarkerPtr>.

mkoval · 2017-03-28T03:05:50Z

include/aikido/rviz/TSRMarker.hpp

+class TSRMarker {
+public:
+  TSRMarker();
+  ~TSRMarker(){};


Nit: Use ~TSRMarker = default instead.

mkoval · 2017-03-28T03:06:03Z

include/aikido/rviz/TSRMarker.hpp

+namespace aikido {
+namespace rviz {
+
+class TSRMarker {


Nit: Either mark this as final or mark the destructor as virtual.

mkoval · 2017-03-28T03:06:37Z

include/aikido/rviz/TSRMarker.hpp

+  void addFrameMarker(FrameMarkerPtr const &marker);
+
+private:
+  std::set<FrameMarkerPtr> mFrameMarkers;


Nit: Why does this need to be an std::set? I'd prefer an std::vector here, since linear memory is a lot simpler than a red-black tree, if we don't require uniqueness.

gilwoolee · 2017-03-28T16:53:47Z

@mkoval I think I addressed all of your comments. Please take a look. Thanks for the help!

coveralls · 2017-03-28T17:06:45Z

Coverage remained the same at 85.391% when pulling e964439 on rviz/visualizeTSR into 963a5d7 on master.

coveralls · 2017-03-28T17:12:52Z

Coverage remained the same at 85.391% when pulling e964439 on rviz/visualizeTSR into 963a5d7 on master.

mkoval

LGTM. I left one minor comment, but feel free to ignore.

mkoval · 2017-03-31T02:21:22Z

src/rviz/InteractiveMarkerViewer.cpp

+    auto tsrFrame = std::make_shared<SimpleFrame>(
+      Frame::World(), ss.str(), state.getIsometry());
+    auto frameMarker = addFrame(tsrFrame.get());
+    tsrFrames.emplace_back(tsrFrame);


Nit: Could we use unique_ptr instead of shared_ptr here?

…nto rviz/visualizeTSR

coveralls · 2017-03-31T18:45:18Z

Coverage remained the same at 85.391% when pulling 4e62ebc on rviz/visualizeTSR into 4cd2795 on master.

gilwoolee · 2017-04-02T18:17:35Z

@mkoval Would you mind taking a last look before I merge this? It just changes shared_ptr to unique_ptr, taking your last suggestion. :)

mkoval

Looks good to me! @gilwoolee feel free to merge.

mkoval · 2017-04-02T22:03:55Z

include/aikido/rviz/SmartPointers.hpp

@@ -17,6 +17,10 @@ class SkeletonMarker;
 typedef std::shared_ptr<SkeletonMarker> SkeletonMarkerPtr;
 typedef std::shared_ptr<SkeletonMarker const> SkeletonMarkerConstPtr;

+class TSRMarker;
+typedef std::shared_ptr<TSRMarker> TSRMarkerPtr;
+typedef std::shared_ptr<TSRMarker const> TSRMarkerConstPtr;


Nit: Prefer using aliases to typedefs.

Please place const before type. See above.

jslee02

Commented some style related nitpicks.

jslee02 · 2017-04-02T22:12:40Z

src/rviz/TSRMarker.cpp

+namespace aikido {
+namespace rviz {
+
+TSRMarker::TSRMarker(std::vector<std::unique_ptr<SimpleFrame>> tsrFrames)


We could use rvalue reference (std::vector<std::unique_ptr<SimpleFrame>>&& tsrFrames) here to prevent unnecessary copy.

This already doesn't make any copies if the caller uses std::move, e.g.:

// Caller std::move's from local variable to argument: std::vector<std::unique_ptr<SimpleFrame>> frames = /* ... */; auto marker = TSRMarker{std::move(frames)}; // Constructor std::moves from argument to member variable: TSRMarker::TSRMarker(std::vector<std::unique_ptr<SimpleFrame>> frames) : markers{std::move(frames)} {}

Note that std::vector<std::unique_ptr<...>> is not copyable, so we already must not be introducing an extra copy. 😉

My general understanding is that rvalue references are only useful for: (1) move constructors and (2) perfect forwarding in templates due to reference collapsing.

This already doesn't make any copies if the caller uses std::move

That is true, but the explicit rvalue argument could prevent passing lvalue by accident (if I'm missing something here). For example:

class TSRMarker { public: TSRMarker(std::vector<std::unique_ptr<SimpleFrame>>&& tsrFrames); // ... }; std::vector<std::unique_ptr<SimpleFrame>> frames = /* ... */; auto marker1 = TSRMarker{frames}; // error auto marker2 = TSRMarker{std::move(frames)}; // okay

Note that std::vector<std::unique_ptr<...>> is not copyable, so we already must not be introducing an extra copy.

Hm, this is what I missed. It still prevents passing lvalue even without making it rvalue reference but in a different way (we get different error messages).

I still prefer to make the argument explicitly rvalue reference because we always expect to pass rvalue here. However, I am not strongly against leaving it as it is since it works as we intend anyway.

I disagree! 😬 I prefer to leave copy versus move up to the caller (i.e. leave the code as it is).

I don't feel particularly strongly either way, so let's leave it up to @gilwoolee to decide. 😄

I don't feel particularly strongly either way, so let's leave it up to @gilwoolee to decide.

👍

My only concern is that the caller is not able to pass lvalue anyway (whether or not the argument is rvalue reference because copy assignment of std::unique_ptr is not defined), but explicit rvalue reference argument could give us hint why the caller cannot pass lvalue.

Alright, let's pass the baton to @gilwoolee. 😈

jslee02 · 2017-04-02T22:14:48Z

include/aikido/rviz/InteractiveMarkerViewer.hpp

+  /// \param basename Basename for markers
+  /// \return TSRMarkerPtr contains sampled frames of TSR.
+  TSRMarkerPtr addTSRMarker(
+    aikido::constraint::TSR const &tsr,


I believe the convention is to place const before type and space after & as const aikido::constraint::TSR& tsr.

jslee02 · 2017-04-02T22:17:00Z

include/aikido/rviz/InteractiveMarkerViewer.hpp

+  /// \return TSRMarkerPtr contains sampled frames of TSR.
+  TSRMarkerPtr addTSRMarker(
+    aikido::constraint::TSR const &tsr,
+    int nSamples = 10, const std::string &basename = "");


Please put a space after & not not before. See above.

jslee02 · 2017-04-02T22:17:20Z

include/aikido/rviz/SmartPointers.hpp

@@ -17,6 +17,10 @@ class SkeletonMarker;
 typedef std::shared_ptr<SkeletonMarker> SkeletonMarkerPtr;
 typedef std::shared_ptr<SkeletonMarker const> SkeletonMarkerConstPtr;

+class TSRMarker;
+typedef std::shared_ptr<TSRMarker> TSRMarkerPtr;
+typedef std::shared_ptr<TSRMarker const> TSRMarkerConstPtr;


Please place const before type. See above.

coveralls · 2017-04-03T22:46:35Z

Coverage remained the same at 85.391% when pulling 7fd58e1 on rviz/visualizeTSR into 4cd2795 on master.

Addes TSR visualizer to aikido/rviz.

48e929c

gilwoolee requested a review from ClintLiddick March 10, 2017 23:40

mkoval requested changes Mar 17, 2017

View reviewed changes

Merge branch 'master' into rviz/visualizeTSR

929f988

gilwoolee added 2 commits March 22, 2017 11:41

Merge branch 'master' of https://github.com/personalrobotics/aikido i…

91a4e1a

…nto rviz/visualizeTSR

Merge branch 'rviz/visualizeTSR' of https://github.com/personalroboti…

a68bdae

…cs/aikido into rviz/visualizeTSR

gilwoolee added 2 commits March 27, 2017 16:16

Addressing Mike's comments, adding TSRMarker class.

0266e6f

Removing a print line.

09ca6c9

mkoval requested changes Mar 28, 2017

View reviewed changes

gilwoolee added 4 commits March 28, 2017 00:49

Fixing destructor.

35fabc5

Addressing Mike's comments.

53c5634

Adding docstring for addTSRMarker.

c1756d4

Adding a newline.

e964439

mkoval approved these changes Mar 31, 2017

View reviewed changes

gilwoolee added 2 commits March 31, 2017 14:25

Changing shared_ptr to unique_ptr for TSRFrames.

9fef551

Merge branch 'master' of https://github.com/personalrobotics/aikido i…

4e62ebc

…nto rviz/visualizeTSR

mkoval approved these changes Apr 2, 2017

View reviewed changes

jslee02 suggested changes Apr 2, 2017

View reviewed changes

jslee02 approved these changes Apr 2, 2017

View reviewed changes

Fix style

7fd58e1

jslee02 added this to the Aikido 0.1.0 milestone Apr 4, 2017

gilwoolee merged commit 9c977e3 into master Apr 4, 2017

gilwoolee deleted the rviz/visualizeTSR branch April 4, 2017 02:19

Addes TSR visualizer to aikido/rviz. #136

Addes TSR visualizer to aikido/rviz. #136

Conversation

gilwoolee commented Mar 10, 2017 • edited by mkoval Loading

coveralls commented Mar 10, 2017

mkoval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 17, 2017

coveralls commented Mar 22, 2017

coveralls commented Mar 27, 2017

coveralls commented Mar 27, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gilwoolee commented Mar 28, 2017

coveralls commented Mar 28, 2017

coveralls commented Mar 28, 2017

mkoval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Mar 31, 2017

gilwoolee commented Apr 2, 2017

mkoval left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslee02 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslee02 Apr 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Apr 3, 2017

gilwoolee commented Mar 10, 2017 •

edited by mkoval

Loading

jslee02 Apr 3, 2017 •

edited

Loading