Created aikido_perception package

[Shushman]

This is the first attempt at a perception module for AIKIDO.

• Introduction of aikido_perception as a package
• Abstract class in PerceptionModule.hpp
• Implementation in AprilTagsModule.hpp/cpp

---

This change is

[mkoval]

You should accept a ResourceRetriever and seamlessly fetch the configuration file by URI. This lets us seamlessly switch between retrieving files from a filesystem path (via LocalResourceRetriever), a Catkin package (via CatkinResourceRetriever), and other storage locations that we may potentially use in the future (e.g. an HTTP server, a database, etc).

The easiest way to do this is to read the entire file into an std::string and pass that string to yaml-cpp.

[Shushman]

Is there an example of this, even if in a different context, somewhere? I've having trouble envisioning what the exact code would be.

[mkoval]

There is an example of doing this in DART's TinyXML helper code. The snippet looks something like this:

const dart::common::ResourcePtr resource = retriever->retrieve(uri);
if (!resource)
  throw std::runtime_error("Failed opening URI.");

// C++11 guarantees that std::string has contiguous storage.
const std::size_t size = resource->getSize();
std::string content;
content.resize(size);

if (resource->read(&content.front(), size, 1) != 1)
    throw std::runtime_error("Failed reading from URI.");

[mkoval]

This is a good start. I left some specific commits on the diff, but I'd also like to open up discussion on a few high-level API discussions:

1. What is the DART equivalent to an OpenRAVE Environment?

   @Shushman is currently using std::vector<SkeletonPtr> as a placeholder for this type. The obvious choice is dart::simulation::World, but this actually instantiates the DART dynamics engine. This is overkill because we often want to instantiate an Environment without running a dynamical simulation.

   I see two compelling options: (1) use World anyway or (2) create a World-like class in the dynamics namespace that eschews simulation-specific data. It would simply represent a collection of Skeletons and enforce name uniqueness. @jslee02 @mxgrey @psigen Thoughts?

2. What should the detectObjects API be?

   At a minimum, I would like to extend the existing API to accept a timestamp. Any updates older than that timestamp should be ignored. This is critical to avoid race conditions under heavy latency, where you may receive a message that represents the state of the world many seconds ago. We could default this value to now(), at the time detectObjects is called, to have a conservative default.

   I'm not sure whether we should use ros::Time and ros::Duration in this API, the std::chrono equivalents, or something else entirely.

3. Separate loading of the YAML configuration file from its usage.

   I try to avoid configuration file parsing (which typically contains a lot of error checking) with code that actually implements the operation. At a minimum, I'd prefer if you split all of the YAML parsing code into a separate function that returns a generic struct that contains the data you need to resolve detections.

   Ideally, I'd like to isolate the code that loads the YAML file into a separate class that can be replaced with another source of configuration data. This opens up a lot of possibilities that would otherwise be impossible. For example, I could hard-code the tag transformation for testing purposes. Or programmatically specify that all tags in the range 10-30 map to Fuze bottles without generating a YAML file with 30 duplicate entries.

4. Class name.

   @psigen pointed out that most of the code in ApriltagsPerceptionModule works for a general fiducial tracking system. We'd like to rename the class to reflect this (perhaps FiducialPerceptionModule or MarkerPerceptionModule?) and avoid introducing Apriltag specific dependencies.

[mkoval]

Minor issues:

• DART uses _camelCase, not _snake_case for parameter names.
• Add a Doxygen comment block, including a one-line description of each argument.
• Take ros::NodeHandle by const reference to avoid an extra copy.
• Take std::string parameters by const reference to avoid an extra copy.
• Take a Frame * instead of a BodyNode * for _reference_link unless there is a strong reason to do otherwise.
• Take _marker_data_path and _urdf_path as dart::common::Uris if they will be passed into a ResourceRetriever; note that string implicitly converts to Uri so this introduces no syntax overhead.
• Naming the ResourceRetriever argument _delegate only make sense for ResourceRetriever adaptors. Naming this _resourceRetriever, or something similar, would be better.

[jslee02]

DART uses _camelCase, not _snake_case for parameter names.

To clarify, the recent convention of parameter names (Grey, Karen, and I agreed) in DART is camelCase (without leading underscore). Sorry for the confusing. 😞

[mkoval]

the recent convention of parameter names [...] in DART is camelCase (without leading underscore).

🎊 That's great news! I was never a fan of the leading underscore. 😄

What is the convention for member variable names? Should we still use the m prefix?

[jslee02]

Yes, we still use m prefix for member variable names of class (e.g., mCamelCase), but it's unclear for struct.

[coveralls]

Coverage remained the same at 89.68% when pulling 63091a5 on feature/perceptionmodule into bcf5b32 on master.

[coveralls]

Coverage decreased (-0.08%) to 89.602% when pulling cc917b3 on feature/perceptionmodule into bcf5b32 on master.

[coveralls]

Coverage remained the same at 89.602% when pulling 734f34f on feature/perceptionmodule into e0edcad on master.

[mkoval]

We're getting there! 😓

A few very minor high-level comments:

• Improve the naming of the ConfigLoader classes.
• Write more informative docstrings for the classes and functions. Try to think about what the function does and any information the user needs to know to call it, but is not captured in the functions' signature. Default behavior, error conditions, and edge cases usually fall into this category.

---

Reviewed 2 of 8 files at r2, 1 of 1 files at r11, 8 of 9 files at r12.
Review status: all files reviewed at latest revision, 15 unresolved discussions.

---

aikido_perception/include/aikido/perception/AprilTagsModule.hpp, line 21 [r13] (raw file):
The comment should describe what the class is used for. E.g. something like this:

Perception module that detects objects using an AprilTag detector running in a separate process. This class subscribes to a visualization_msg::Marker ROS topic published by the AprilTag node, uses a \c AprilTagDatabase to resolve each marker to a \c dart::common::Uri, and updates the environment accordingly.

---

aikido_perception/include/aikido/perception/AprilTagsModule.hpp, line 36 [r13] (raw file):
This is not a useful comment - I'd prefer to remove it.

---

aikido_perception/include/aikido/perception/ConfigDataLoader.hpp, line 11 [r13] (raw file):
This is not a very descriptive name. I believe this class is only useful for AprilTag markers, so it should be named accordingly (e.g. AprilTagDatabase). Sorry I didn't notice this before.

---

aikido_perception/include/aikido/perception/ConfigDataLoader.hpp, line 17 [r13] (raw file):
Nit: The docstring should say what the function does, not what it is. For example, something like this would be ideal:

Gets the name, resource URI, and relative offset of an Apriltag attached to a body."

A good litmus test for this is to look for verbs (e.g. "gets", "sets", "computes") and be wary of docstrings that have none.

---

aikido_perception/include/aikido/perception/ConfigDataLoader.hpp, line 22 [r13] (raw file):

• \return comment is inaccurate. This actually returns a bool that indicates success or failure.

---

aikido_perception/include/aikido/perception/PerceptionModule.hpp, line 17 [r13] (raw file):
This should be a \param[in,out] if I am not mistaken.

---

aikido_perception/include/aikido/perception/PerceptionModule.hpp, line 18 [r13] (raw file):
If we're committing to ROS types, then duration should probably be a ros::Duration. The standard behavior is generally to default to ros::Duration(0) and treat this as "no timeout".

---

aikido_perception/include/aikido/perception/PerceptionModule.hpp, line 19 [r13] (raw file):
Document the fact that an invalid (i.e. zero) timestamp greedily takes the first available message. I suggest making that the default behavior.

---

aikido_perception/include/aikido/perception/PerceptionModule.hpp, line 21 [r13] (raw file):
There is currently no way to tell whether this function timed out or succeeded. I suggest making the return value bool to indicate this.

---

aikido_perception/include/aikido/perception/shape_conversions.hpp, line 23 [r10] (raw file):
Still missing docstrings.

---

aikido_perception/include/aikido/perception/YamlFileLoader.hpp, line 16 [r13] (raw file):

• This is a docstring for the class, not the header.
• YamlFileLoader is a very generic name. Rename this to something more descriptive, e.g. YamlAprilTagsDatabase.

---

aikido_perception/src/AprilTagsModule.cpp, line 49 [r9] (raw file):
Please make an issue to track this change.

---

aikido_perception/src/AprilTagsModule.cpp, line 31 [r13] (raw file):

• Initialize this in the initialization list unless there is a compelling reason to do otherwise.
• Why manage the TransformListener by unique_ptr, instead of by value (i.e. putting it directly in the class)?

---

aikido_perception/src/shape_conversions.cpp, line 7 [r10] (raw file):
Still need to remove this header.

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 89.602% when pulling da74cb3 on feature/perceptionmodule into e0edcad on master.

[mkoval]

Reviewed 2 of 8 files at r2, 12 of 12 files at r14.
Review status: all files reviewed at latest revision, 12 unresolved discussions.

---

aikido_perception/include/aikido/perception/AprilTagsDatabase.hpp, line 12 [r14] (raw file):
Typo: last sentence is just the word "This."

---

aikido_perception/include/aikido/perception/PerceptionModule.hpp, line 16 [r14] (raw file):
This still says what the function is, not what it does - there is no verb here. Please change it to something like this:

Detect objects and updates the DART environment by creating, removing, and updating the pose of Skeletons in \c skeleton_list.

---

aikido_perception/include/aikido/perception/PerceptionModule.hpp, line 21 [r14] (raw file):
Missing \return documentation. You could move the second sentence of the timeout parameter to that line.

---

aikido_perception/include/aikido/perception/YamlAprilTagsDatabase.hpp, line 16 [r14] (raw file):
Add documentation about the file format here. Ideally, also include a snippet of example YAML in a \code block.

---

aikido_perception/include/aikido/perception/YamlAprilTagsDatabase.hpp, line 22 [r14] (raw file):
Again: please explain what the function does, not what it is. E.g.:

Constructs a \c YamlApriltagDatabase that uses \c resourceRetriever to load configuration data from a YAML file at the URI \c configDataURI.

---

aikido_perception/src/AprilTagsModule.cpp, line 30 [r14] (raw file):
It's not safe to use node here, since you std::moveed from it a few lines above this. You should pass mNode (not node) by value. I suspect this only works because NodeHandle in ROS indigo does not define a move constructor.

---

aikido_perception/src/AprilTagsModule.cpp, line 47 [r14] (raw file):
I'm not sure if we should treat this as a warning and return false. It's reasonable for the detector to detect nothing and publish an empty message. Thoughts?

If we do decide to keep this, note that we're missing a newline after the warning.

---

aikido_perception/src/AprilTagsModule.cpp, line 84 [r14] (raw file):
Missing space between timestamp and the exception error message. Also, I don't think the std::string conversion is necessary here.

---

aikido_perception/src/YamlAprilTagsDatabase.cpp, line 59 [r14] (raw file):
Don't add the [class::method] prefix to exception error messages. This is only necessary in dtwarn and dterr log messages, where a stack trace is not available.

---

Comments from Reviewable

[Shushman]

Review status: 8 of 14 files reviewed at latest revision, 12 unresolved discussions.

---

aikido_perception/include/aikido/perception/PerceptionModule.hpp, line 16 [r14] (raw file):
Hmm. I had put that in the docstring for the class instead of the but I see what you mean

---

aikido_perception/include/aikido/perception/YamlAprilTagsDatabase.hpp, line 16 [r14] (raw file):
Not sure exactly how the \code block works. Will speak with you

---

aikido_perception/src/AprilTagsModule.cpp, line 30 [r14] (raw file):
Done.

---

aikido_perception/src/AprilTagsModule.cpp, line 47 [r14] (raw file):
Well in terms of the detectObjects(), a false result makes sense to me because it has not actually detected any objects. And in this case it will return false and not affect the skeleton_list, so I thought that made sense.

---

aikido_perception/src/YamlAprilTagsDatabase.cpp, line 58 [r9] (raw file):
I'll think a bit more about how to do this. For now, I added an error check for the third component

---

aikido_perception/src/YamlAprilTagsDatabase.cpp, line 59 [r14] (raw file):
Done.

---

Comments from Reviewable

[mkoval]

Reviewed 2 of 8 files at r2, 3 of 12 files at r14, 6 of 6 files at r15.
Review status: all files reviewed at latest revision, 2 unresolved discussions.

---

aikido_perception/include/aikido/perception/YamlAprilTagsDatabase.hpp, line 16 [r14] (raw file):
Doxygen supports putting code in a \code ... \endcode block. See here for an example.

---

aikido_perception/src/AprilTagsModule.cpp, line 49 [r9] (raw file):
@Shushman posted an issue here: #93.

---

Comments from Reviewable

[coveralls]

Coverage increased (+0.2%) to 89.759% when pulling 54dcc29 on feature/perceptionmodule into e0edcad on master.

[coveralls]

Coverage remained the same at 89.759% when pulling b254630 on feature/perceptionmodule into 02b50b7 on master.

[coveralls]

Coverage remained the same at 89.759% when pulling 06d12a2 on feature/perceptionmodule into 02b50b7 on master.

[mkoval]

Reviewed 2 of 8 files at r2, 2 of 12 files at r14, 5 of 5 files at r17.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

Comments from Reviewable

[coveralls]

Coverage remained the same at 89.759% when pulling fc71e56 on feature/perceptionmodule into 02b50b7 on master.

[mkoval]

---

Review status: 13 of 14 files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[mkoval]

Reviewed 1 of 12 files at r14, 1 of 1 files at r18.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from Reviewable

[mkoval]

Great work @Shushman. Thanks for putting up with all of the comments!

[Shushman]

Haha no worries @mkoval. It was an important learning experience for me too. And I'm sure there are many more to come 👍