Aruco grid filter#18
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #18 +/- ##
=====================================
Coverage 0.00% 0.00%
=====================================
Files 20 21 +1
Lines 418 490 +72
Branches 34 39 +5
=====================================
- Misses 418 490 +72
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
kluge7
left a comment
There was a problem hiding this comment.
What you need to do:
- Fix pre-commit errors
- Make it so when you rotate the box, you only rotate that and not the rest of the image
- Optimize the code?
- Use binary_threshold (apply_fixed_threshold?) instead (avoids duplicating code)
There was a problem hiding this comment.
Pull request overview
This PR introduces a new image filter called "RemoveGrid" to address a competition issue where ArUco markers on a structure were obscured by a green grid, preventing successful detection. The filter removes the grid using color-based masking and inpaints the affected regions using OpenCV's inpainting algorithms, followed by binary thresholding to reconstruct the ArUco marker for detection.
Changes:
- Added RemoveGrid filter implementation with rotation, cropping, green channel masking, inpainting, and binary thresholding
- Integrated the new filter into the ROS node with configurable parameters
- Updated configuration defaults to use the new filter with specific camera topics
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
| image-filtering/include/image_filtering/lib/filters/remove_grid.hpp | New filter implementation that rotates, crops, masks green grid pixels, inpaints, and applies binary threshold |
| image-filtering/include/image_filtering/lib/typedef.hpp | Added RemoveGrid enum value and string mapping for filter type selection |
| image-filtering/include/image_filtering/lib/filters/all_filters.hpp | Added include for the new remove_grid.hpp header |
| image-filtering/src/ros/image_filtering_ros.cpp | Added parameter handling and instantiation logic for RemoveGrid filter |
| image-filtering/config/image_filtering_params.yaml | Changed default filter to remove_grid and updated topic subscriptions and encodings for testing |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (original.empty()) { | ||
| spdlog::error("RemoveGrid: input image is empty"); | ||
| return; | ||
| } | ||
|
|
||
| if (original.type() != CV_8UC3) { | ||
| spdlog::error("RemoveGrid: expected CV_8UC3 image"); | ||
| original.copyTo(filtered); | ||
| return; |
There was a problem hiding this comment.
The early return on line 41 when the input image is empty leaves the 'filtered' output parameter uninitialized. This could cause undefined behavior if the caller attempts to use the filtered image. Consider either copying the original to filtered (as done in the type check case on line 46), or ensuring the filtered image is initialized to an empty Mat before returning.
| cv::invertAffineTransform(rotation_matrix, inv_rot); | ||
|
|
||
| cv::warpAffine(rotated_out, filtered, inv_rot, original.size(), | ||
| cv::INTER_LINEAR, cv::BORDER_CONSTANT, cv::Scalar(0, 0, 0)); |
There was a problem hiding this comment.
The forward rotation uses INTER_NEAREST interpolation, while the inverse rotation on line 126 uses INTER_LINEAR. This inconsistency could introduce artifacts or alignment issues. For geometric transformations that should be reversible, it's generally better to use the same interpolation method for both forward and inverse operations.
| cv::INTER_LINEAR, cv::BORDER_CONSTANT, cv::Scalar(0, 0, 0)); | |
| cv::INTER_NEAREST, cv::BORDER_CONSTANT, cv::Scalar(0, 0, 0)); |
| sub_topic: "/cam/image_color" | ||
| pub_topic: "/filtered_image" | ||
| input_encoding: "mono8" | ||
| output_encoding: "mono8" | ||
| input_encoding: "rgb8" | ||
| output_encoding: "rgb8" | ||
|
|
||
| filter_params: | ||
| filter_type: "otsu" | ||
| filter_type: "remove_grid" |
There was a problem hiding this comment.
The default configuration has been changed to use the new 'remove_grid' filter and different topics ('/cam/image_color' instead of '/fls_publisher/display_mono', 'rgb8' encoding instead of 'mono8'). This changes the default behavior of the image filtering node. Consider whether these changes should be committed as defaults, or if they should be reverted to maintain the original default behavior, with the new filter being opt-in via configuration override.
| cv::Mat& filtered) const override; | ||
|
|
||
| private: | ||
| RemoveGridParams params_; |
There was a problem hiding this comment.
The member variable is named 'params_' instead of following the codebase convention of 'filter_params_' used by other filters (e.g., Flip, Overlap, MedianBinary, BinaryThreshold, OtsuSegmentation). This inconsistency should be corrected to maintain naming convention uniformity across filter implementations.
| // Center crop | ||
| // ---------------------------- | ||
| int crop_w = std::min(params_.width, rotated.cols); | ||
| int crop_h = std::min(params_.height, rotated.rows); |
There was a problem hiding this comment.
If the configured width or height exceeds the rotated image dimensions, the crop parameters are safely clamped using std::min. However, there's no warning or logging to inform the user that their configured dimensions were not achievable. Consider adding a debug or warning log when the crop dimensions need to be clamped, so users understand their configuration may not be having the intended effect.
| int crop_h = std::min(params_.height, rotated.rows); | |
| int crop_h = std::min(params_.height, rotated.rows); | |
| if (crop_w != params_.width || crop_h != params_.height) { | |
| spdlog::warn( | |
| "RemoveGrid: requested crop size (width={}, height={}) exceeds " | |
| "rotated image size (width={}, height={}); clamping to " | |
| "(width={}, height={})", | |
| params_.width, | |
| params_.height, | |
| rotated.cols, | |
| rotated.rows, | |
| crop_w, | |
| crop_h); | |
| } |
replaced the binary thresholding with apply_fixed_threshold to avoid duplicating code
| if (original.empty()) { | ||
| spdlog::error("RemoveGrid: input image is empty"); | ||
| return; | ||
| } |
| if (original.type() != CV_8UC3) { | ||
| spdlog::error("RemoveGrid: expected CV_8UC3 image"); | ||
| original.copyTo(filtered); | ||
| return; |
| dilated.col(dilated.cols - 1).setTo(0); | ||
|
|
||
| if (cv::countNonZero(dilated) == 0) { | ||
| original.copyTo(filtered); // or original.copyTo(filtered); |
There was a problem hiding this comment.
I dont understand the comment
| inpaint_radius: 1.0 | ||
| rotation: 0 | ||
| height: 400 | ||
| width: 400 |
|
|
||
| ///////////////////////////// | ||
| // Remove grid filter | ||
| ///////////////////////////// |
|
|
||
| struct RemoveGridParams { | ||
| double threshold_green; | ||
| int threshold_binary; |
There was a problem hiding this comment.
threshold_binary was double in config. why dis int
|
|
||
| // ---------------------------- | ||
| // Rotate directly into cropped output | ||
| // ---------------------------- |
| int crop_w = std::min(params_.width, original.cols); | ||
| int crop_h = std::min(params_.height, original.rows); | ||
|
|
||
| if (crop_w != params_.width || crop_h != params_.height) { |
There was a problem hiding this comment.
if (crop_w > params_.width || crop_h > params_.height) { ?
There was a problem hiding this comment.
Not a problem but you use a lot of redundant variables in this file, a lot of them are not necessary and you would shorten the code
| dilated.col(dilated.cols - 1).setTo(0); | ||
|
|
||
| if (cv::countNonZero(dilated) == 0) { | ||
| original.copyTo(filtered); // or original.copyTo(filtered); |
| // ---------------------------- | ||
| // Inpaint grid | ||
| // ---------------------------- |
| // ---------------------------- | ||
| // Binary threshold (on cropped ROI) |
|
|
||
| cv::Mat thresh_bgr; | ||
| cv::cvtColor(thresh_gray, thresh_bgr, cv::COLOR_GRAY2BGR); |
There was a problem hiding this comment.
Is this necessary for the ArUco detector to work? This should be optional (converting is a slow operation, and this is mainly for debugging)
| // ---------------------------- | ||
| // Undo rotation & merge (using M) |
4779026 to
93a3bdb
Compare
ArUco grid filter
During the last competition, one of the ArUco markers on the structure was obscured by a grid, causing the detector to fail. This is because successful scanning requires that the detector can first identify the marker’s encoded pattern.
The filter removes the grid from the image and uses the inpaint function from OpenCV which is constrained by a bounding box to reconstruct the ArUco marker.
colcon build --symlink-install --packages-up-to image_filtering aruco_detector source install/setup.bash ros2 launch image_filtering image_filtering.launch.pyand in another terminal write
Use the simulator with the scenario:=tacc.