Marshall Vielmetti Starter Project PR #1
Conversation
| @@ -0,0 +1,156 @@ | |||
| #include "tag_detector.hpp" | |||
|
|
|||
| namespace mrover { | |||
There was a problem hiding this comment.
you may want to make a commit to remove src/perception/tag_detector from this branch
| @@ -45,6 +49,9 @@ namespace mrover { | |||
| std::vector<StarterProjectTag> mTags; | |||
| ros::Publisher mTagPublisher; | |||
|
|
|||
| size_t image_width; | |||
There was a problem hiding this comment.
I would instead modify selectTag to take in a cv::Mat instead of keeping track of global state
| ret.first = center_x; | ||
| ret.second = center_y; | ||
|
|
||
| return ret; |
| // TODO: implement me! | ||
| return {}; | ||
| float center_x = float(tagCorners[0].x + tagCorners[1].x) / float(2.0); |
There was a problem hiding this comment.
You can use cv::Point2f from OpenCV which implements element-wise operations. It saves you from defining multiple variables and repetition.
| // TODO: implement me! | ||
| return {}; | ||
| float center_x = float(tagCorners[0].x + tagCorners[1].x) / float(2.0); | ||
| float center_y = float(tagCorners[0].y + tagCorners[3].y) / float(2.0); |
There was a problem hiding this comment.
Prefer static_cast<float>(...) instead of float(...)
|
|
||
| for (int i = 1; i < tags.size(); i++) { | ||
| if ((tags[i].xTagCenterPixel*tags[i].xTagCenterPixel + tags[i].yTagCenterPixel*tags[i].yTagCenterPixel) < best_dist) { | ||
| best_dist = tags[i].xTagCenterPixel*tags[i].xTagCenterPixel + tags[i].yTagCenterPixel*tags[i].yTagCenterPixel; |
There was a problem hiding this comment.
you also reuse it here, so may as well name it
| // } | ||
|
|
||
| StarterProjectTag Perception::selectTag(std::vector<StarterProjectTag> const& tags) const { // NOLINT(*-convert-member-functions-to-static) | ||
| if (tags.empty()) |
| StarterProjectTag best = tags[0]; | ||
| double best_dist = best.xTagCenterPixel*best.xTagCenterPixel + best.yTagCenterPixel*best.yTagCenterPixel; | ||
|
|
||
| for (int i = 1; i < tags.size(); i++) { |
There was a problem hiding this comment.
Consider using https://en.cppreference.com/w/cpp/algorithm/ranges/min_element with a projection function
Also tags.size() returns a size_t so int i is narrowing
|
|
||
| float corner_area = abs((tagCorners[1].x - tagCorners[0].x) * (tagCorners[0].y - tagCorners[3].y)); | ||
|
|
||
| return 1 - float((1.0*corner_area) / (1.0*total_area)); |
There was a problem hiding this comment.
Pull out float((1.0*corner_area) / (1.0*total_area)); as a variable for readability
|
|
||
| cv::aruco::detectMarkers(image, mTagDictionary, mTagCorners, mTagIds, mTagDetectorParams); | ||
|
|
||
| std::vector<float> closeness_metrics; |
There was a problem hiding this comment.
I think these collections are redundant. You have everything you need in mTagCorners and mTagIds right. The following two for loops could just be one
|
also fyi you can run |
No description provided.