Skip to content

Conversation

oahelmer
Copy link
Contributor

@oahelmer oahelmer commented Apr 6, 2025

line detection tuned for office, needs good lighting.

@oahelmer oahelmer requested a review from jorgenfj April 6, 2025 11:53
@oahelmer oahelmer self-assigned this Apr 6, 2025
Comment on lines 51 to 62
pointcloud_pub_ = this->create_publisher<sensor_msgs::msg::PointCloud2>(
"point_cloud", 10);
line_pose_pub_ = this->create_publisher<geometry_msgs::msg::PoseStamped>(
"line_pose", 10);
"valve_pose", 10);
line_points_pub_ = this->create_publisher<sensor_msgs::msg::PointCloud2>(
"line_points", 10);
near_plane_cloud_pub_ =
this->create_publisher<sensor_msgs::msg::PointCloud2>(
"near_plane_cloud", 10);
canny_debug_image_pub_ = this->create_publisher<sensor_msgs::msg::Image>(
"canny_valve_detection_image", 10);
}

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would be mindful about the buffer size here and qos here. For image streams the best effort qos variable you defined over should be good instead

Comment on lines 161 to 164
void ValveDetectionNode::process_and_publish_image(
const sensor_msgs::msg::Image::ConstSharedPtr& image,
const sensor_msgs::msg::Image::ConstSharedPtr& depth_image,
const sensor_msgs::msg::Image::ConstSharedPtr& color_image,
const vision_msgs::msg::Detection2DArray::ConstSharedPtr& detections) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This method is very long, has too many indentations and does a lot of stuff. For a reader it is not so easy to see what is going on. I would suggest to look at these points that could improve the code readability and maintainability, and maybe also performance improvements.

Avoid unnecessary nesting:

Instead of

if (!condition) {
    // Continue with stuff
}

you can do

if (condition) {
    return; // or handle it in other ways
}
// Continue with stuff

This is especially nice for the reader since they don't have to keep track of the condition when reading the code.

Variable naming

Clear and more explanatory names are also something that ups the overall readability of the code, and makes it easier for readers to easier understand it. Although you have some good and clear names, there are some that can be improved. For instance cx, cy, dx, dy, x, y. It isnt easy to immediately see what they represent, even with context.

Helper functions

Well-named helper functions make the code easier to read and maintain. They are also following the single-responsibility principle that says that each function has one job.

Example:

float distance =
    std::abs(coefficients->values[0] * point.x +
           coefficients->values[1] * point.y +
           coefficients->values[2] * point.z +
           coefficients->values[3]) /
    std::sqrt(coefficients->values[0] *
                coefficients->values[0] +
            coefficients->values[1] *
                coefficients->values[1] +
            coefficients->values[2] *
                coefficients->values[2]);

could be a helper function. If a code block is repeated, then you could think about generalizing it as a helper function.

Use functions from the Standard Library / CV

There are a lot of nice algorithms and function from both the STL and CV, which can replace the longer and nested for loops

Copy link

codecov bot commented Apr 13, 2025

Codecov Report

Attention: Patch coverage is 0% with 137 lines in your changes missing coverage. Please review.

Project coverage is 0.00%. Comparing base (4568de3) to head (410219d).

Files with missing lines Patch % Lines
src/valve_detection.cpp 0.00% 137 Missing ⚠️
Additional details and impacted files
@@          Coverage Diff          @@
##            main      #3   +/-   ##
=====================================
  Coverage   0.00%   0.00%           
=====================================
  Files          2       2           
  Lines        256     335   +79     
  Branches      21      29    +8     
=====================================
- Misses       256     335   +79     
Flag Coverage Δ
unittests 0.00% <0.00%> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Files with missing lines Coverage Δ
include/valve_detection/valve_detection.hpp 0.00% <ø> (ø)
src/valve_detection.cpp 0.00% <0.00%> (ø)
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@jorgenfj jorgenfj linked an issue Aug 15, 2025 that may be closed by this pull request
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TASK] 6DOF Localization of Valve Gripping Point
5 participants