clean up and integrate pose estimators #336

yskim041 · 2018-02-16T22:22:56Z

In this PR, I integrated pose estimator modules for AprilTags and RcnnPose into one PoseEstimator module. I updated ObjectDatabase and corresponding YAML files in pr-ordata (personalrobotics/pr_assets#32) as well.

Before creating a pull request

Document new methods and classes
Format code with make format

Before merging a pull request

Set version target by selecting a milestone on the right side
Summarize this change in CHANGELOG.md

codecov · 2018-02-16T23:34:26Z

Codecov Report

Merging #336 into master will not change coverage.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #336   +/-   ##
=======================================
  Coverage   81.14%   81.14%           
=======================================
  Files         200      200           
  Lines        5687     5687           
=======================================
  Hits         4615     4615           
  Misses       1072     1072

brianhou · 2018-02-17T00:30:09Z

include/aikido/perception/ObjectDatabase.hpp

@@ -44,7 +44,8 @@ class ObjectDatabase
  void getObjectByKey(


Can we document this method? I'm not entirely clear what obj_offset is.

I added the documentation for this. objectName, objectResource, and objectOffset are what should be retrieved from ObjectDatabase. For each key, those values are stored in tag_data_xx.json in pr_ordata.

objectOffset is the offset between an AprilTag and the actual origin of an object.

Should this be an offset or a transform? Since it's an Isometry3d it seems like it should be the latter?

It is a transformation matrix for the offset. I think Isometry3d is just a way to express it.

Some of them in the apriltag tag_data (https://github.com/personalrobotics/pr_ordata/blob/master/data/objects/tag_data_apriltags.json) have rotation components as well.

jslee02 · 2018-02-18T15:09:54Z

This PR looks like removing AprilTags and adding offset to RcnnPose (and changing the name). Having offset was the only difference of AprilTags from RcnnPose? 🤔

brianhou · 2018-04-13T18:35:36Z

Let's try to update this PR and merge this soon!

yskim041 · 2018-04-21T10:39:40Z

Tested with the can detector by using personalrobotics/herb3_demos#13, and this works well. Since personalrobotics/pr_assets#32 is merged, this branch works with the master branch of pr_ordata.

brianhou · 2018-04-21T22:03:25Z

include/aikido/perception/ObjectDatabase.hpp

@@ -41,10 +47,17 @@ class ObjectDatabase

  virtual ~ObjectDatabase() = default;

+  /// Get the object name, resource, and offset from database by objectKey
+  /// \param[in]  objectKey: The key(string) of an object in ObjectDatabase


I believe we don't want colons in Doxygen comments. Also, minor typo: key(string) -> key (string).

brianhou · 2018-04-21T22:04:40Z

include/aikido/perception/ObjectDatabase.hpp

+  /// \param[out] objectName: The retrieved object name from ObjectDatabase
+  /// \param[out] objectResource: The retrieved uri of the object
+  /// \param[out] objectOffset: The retrieved offset matrix of the object
+  ///     e.g. the offset between a tag and the actual origin of an object


I think this should be i.e. rather than e.g.. Also, I think we should be calling this a transform rather than an offset, if we're using Isometry3d.

brianhou · 2018-04-21T22:05:10Z

include/aikido/perception/PoseEstimatorModule.hpp

 /// \c dart::common::Uri, and updates the environment which is an
 /// \c aikido::planner::World.
-class RcnnPoseModule : public PerceptionModule
+class PoseEstimatorModule : public PerceptionModule


What do you think about renaming this to RosPoseEstimationModule?

If we want to rename it with a more ros related name, I think RosMarkerArrayClient would be the most clear option. If not, I personally prefer PoseEstimatorModule.

brianhou · 2018-04-21T22:07:25Z

Looks good! I had a few minor nits, but I think this should be otherwise good to go.

jslee02

Looks good to me as well.

I have a nitpick on the local variable names. Our convention is to use Pascal case, but many of local variables names are not following it. Could you either fix them in this PR or create an issue?

jslee02 · 2018-04-21T22:14:49Z

src/perception/PoseEstimatorModule.cpp

@@ -1,4 +1,4 @@
-#include <aikido/perception/RcnnPoseModule.hpp>
+#include <aikido/perception/PoseEstimatorModule.hpp>


Nit: Our convention is to use "..." for AIKIDO headers instead of <...>.

jslee02 · 2018-04-21T22:21:35Z

src/perception/PoseEstimatorModule.cpp

@@ -45,14 +47,14 @@ bool RcnnPoseModule::detectObjects(
  // Making sure the Message from RcnnPose is non empty
  if (marker_message == nullptr)
  {
-    dtwarn << "[RcnnPoseModule::detectObjects] nullptr Marker Message "
+    dtwarn << "[PoseEstimatorModule::detectObjects] nullptr Marker Message "
           << mMarkerTopic << std::endl;
    return false;
  }

  if (marker_message->markers.size() == 0)


Nit: Using empty() is more preferred way to check its emptiness rather than size().

yskim041 · 2018-04-23T20:47:03Z

The last commit addressed all the issues above.

* clean up and integrate pose estimators * format PoseEstimatorModule * update CHANGELOG.md * add documentation * add documentation * fix the issues on comments in PR#336

clean up and integrate pose estimators

9265756

yskim041 self-assigned this Feb 16, 2018

yskim041 added this to the Aikido 0.3.0 milestone Feb 16, 2018

format PoseEstimatorModule

38a3a95

yskim041 mentioned this pull request Feb 16, 2018

update tag_data personalrobotics/pr_assets#32

Merged

update CHANGELOG.md

c9b1860

yskim041 requested review from brianhou and jslee02 February 16, 2018 22:52

brianhou suggested changes Feb 17, 2018

View reviewed changes

brianhou and others added 3 commits February 23, 2018 11:11

Merge branch 'master' into enhancement/yskim041/pose_estimator

26cc349

Merge branch 'master' into enhancement/yskim041/pose_estimator

34ff6d5

Merge branch 'master' into enhancement/yskim041/pose_estimator

ada4470

yskim041 added 3 commits April 20, 2018 23:00

merge master, add documentations for perception modules

b076c4b

add documentation

3e36578

add documentation

7bc9548

brianhou suggested changes Apr 21, 2018

View reviewed changes

jslee02 reviewed Apr 21, 2018

View reviewed changes

fix the issues on comments in PR#336

be1c842

Merge branch 'master' into enhancement/yskim041/pose_estimator

172a2b8

brianhou approved these changes Apr 27, 2018

View reviewed changes

brianhou merged commit 4a63536 into master Apr 28, 2018

brianhou deleted the enhancement/yskim041/pose_estimator branch April 28, 2018 00:51

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

clean up and integrate pose estimators #336

clean up and integrate pose estimators #336

yskim041 commented Feb 16, 2018 •

edited

Loading

codecov bot commented Feb 16, 2018 •

edited

Loading

brianhou Feb 17, 2018

yskim041 Apr 21, 2018

yskim041 Apr 21, 2018 •

edited

Loading

brianhou Apr 24, 2018

yskim041 Apr 25, 2018 •

edited

Loading

brianhou Apr 25, 2018

jslee02 commented Feb 18, 2018

brianhou commented Apr 13, 2018

yskim041 commented Apr 21, 2018

brianhou Apr 21, 2018

brianhou Apr 21, 2018

brianhou Apr 21, 2018

yskim041 Apr 23, 2018

brianhou commented Apr 21, 2018

jslee02 left a comment

jslee02 Apr 21, 2018

jslee02 Apr 21, 2018

yskim041 commented Apr 23, 2018 •

edited

Loading

		@@ -1,4 +1,4 @@
		#include <aikido/perception/RcnnPoseModule.hpp>
		#include <aikido/perception/PoseEstimatorModule.hpp>

clean up and integrate pose estimators #336

clean up and integrate pose estimators #336

Conversation

yskim041 commented Feb 16, 2018 • edited Loading

codecov bot commented Feb 16, 2018 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yskim041 Apr 21, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yskim041 Apr 25, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jslee02 commented Feb 18, 2018

brianhou commented Apr 13, 2018

yskim041 commented Apr 21, 2018

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

brianhou commented Apr 21, 2018

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

yskim041 commented Apr 23, 2018 • edited Loading

yskim041 commented Feb 16, 2018 •

edited

Loading

codecov bot commented Feb 16, 2018 •

edited

Loading

yskim041 Apr 21, 2018 •

edited

Loading

yskim041 Apr 25, 2018 •

edited

Loading

yskim041 commented Apr 23, 2018 •

edited

Loading