-
Notifications
You must be signed in to change notification settings - Fork 6
2D->3D detection to pointcloud new pipeline #94
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Conversation
…e refitConvexHull and one on cluster algorithm to append to the point cloud only points which have close depth to their neighbooring pixels.
…metric features for better generalization
… we calculate posteriors not only points)
…ot fully correct)
…m) for 2D-3D inference
…sam_seg_module. Will be used as verbose features.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
This PR integrates YOLO, SAM, and Bayesian model capabilities into the ed_sensor_integration package to enable 2D-to-3D pointcloud detection. The integration replaces traditional depth-based clustering with AI-driven segmentation using YOLO for object detection, SAM for precise mask generation, and a Bayesian mixture model for point cloud denoising.
- Replaces depth-based clustering with YOLO+SAM segmentation pipeline
- Adds Bayesian mixture model (BMM) for statistical point cloud filtering
- Integrates ROS publishers for visualization of segmentation results
Reviewed Changes
Copilot reviewed 17 out of 18 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| hero_sam | Submodule reference to external SAM implementation |
| ed_sensor_integration_py/ | New Python package structure for sensor integration |
| ed_sensor_integration/src/kinect/updater.cpp | Modified to use new segmentation pipeline and added outlier filtering |
| ed_sensor_integration/src/kinect/segmenter.cpp | Replaced clustering algorithm with YOLO+SAM pipeline integration |
| ed_sensor_integration/src/kinect/sam_seg_module.cpp | New module implementing YOLO+SAM segmentation pipeline |
| ed_sensor_integration/include/ | Updated headers to support new segmentation architecture |
| ed_sensor_integration/CMakeLists.txt | Added dependencies and build configuration for new components |
| ed_sensor_integration/package.xml | Added package dependencies for YOLO, SAM, and BMM |
Comments suppressed due to low confidence (1)
ed_sensor_integration/src/kinect/updater.cpp:1
- Commented-out code should be removed. If this is needed for testing, consider using a configuration parameter instead.
#include "ed/kinect/updater.h"
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| ROS_ERROR("No segmenter configuration found, cannot initialize Updater."); | ||
| throw std::runtime_error("No segmenter configuration found"); |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The error message should be more specific about what configuration is missing. Consider changing to 'Failed to read segmenter configuration group from config file'.
| ROS_ERROR("No segmenter configuration found, cannot initialize Updater."); | |
| throw std::runtime_error("No segmenter configuration found"); | |
| ROS_ERROR("Failed to read segmenter configuration group from config file, cannot initialize Updater."); | |
| throw std::runtime_error("Failed to read segmenter configuration group from config file"); |
| for (int y = 0; y < rgb_image.rows; y++) { | ||
| for (int x = 0; x < rgb_image.cols; x++) { | ||
| int idx = y * rgb_image.cols + x; | ||
| if (filtered_depth_image.at<float>(idx) > 0) { |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The index calculation idx = y * rgb_image.cols + x assumes the depth image is stored as a 1D array, but filtered_depth_image.at<float>(idx) accesses it as such. However, OpenCV Mat objects are typically accessed as 2D with at<float>(y, x). This could cause out-of-bounds access or incorrect pixel mapping.
| if (filtered_depth_image.at<float>(idx) > 0) { | |
| if (filtered_depth_image.at<float>(y, x) > 0) { |
| if (result.classId == 60) { | ||
| std::cout << "Class ID is: " << yoloDetector->classes[result.classId] << " So we dont append"<< std::endl; |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number 60 should be replaced with a named constant or configuration parameter. Consider defining constexpr int IGNORED_CLASS_ID = 60; or making this configurable.
| { | ||
| // // Overlay masks on the RGB image | ||
| cv::Mat visualization = rgb.clone(); | ||
| cv::imwrite("/tmp/visualization.png", visualization); |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded file paths in /tmp/ should be configurable or use proper temporary file handling. These debug outputs could fill up disk space and should be controlled by a debug flag or configuration parameter.
| // Apply a colormap for better visibility | ||
| cv::Mat depth_color; | ||
| cv::applyColorMap(depth_vis, depth_color, cv::COLORMAP_JET); | ||
| cv::imwrite("/tmp/visualization_depth_color.png", depth_color); |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded file paths in /tmp/ should be configurable or use proper temporary file handling. These debug outputs could fill up disk space and should be controlled by a debug flag or configuration parameter.
| cv::imwrite("/tmp/visualization_depth.png", depth_vis); | ||
| overlayMasksOnImage_(visualization, clustered_images); | ||
| // save after overlaying masks | ||
| cv::imwrite("/tmp/visualization_with_masks.png", visualization); |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded file paths in /tmp/ should be configurable or use proper temporary file handling. These debug outputs could fill up disk space and should be controlled by a debug flag or configuration parameter.
| } | ||
| } | ||
| // Safety check | ||
| if (filtered_points.size() > 10 && filtered_points.size() > 0.1 * cluster.points.size()) { |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic numbers 10 and 0.1 should be replaced with named constants. Consider defining constexpr size_t MIN_FILTERED_POINTS = 10; and constexpr double MIN_RETENTION_RATIO = 0.1;.
ed_sensor_integration/CMakeLists.txt
Outdated
|
|
||
| # -------------- ONNXRuntime Setup (define this early) ------------------# | ||
| set(ONNXRUNTIME_VERSION 1.21.1) | ||
| set(ONNXRUNTIME_ROOT "/home/amigo/Documents/repos/hero_sam.bak/onnxruntime-linux-x64-gpu-${ONNXRUNTIME_VERSION}") |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded absolute paths make the build non-portable. These paths should be configurable through environment variables or CMake cache variables, e.g., set(ONNXRUNTIME_ROOT $ENV{ONNXRUNTIME_ROOT} CACHE PATH \"Path to ONNX Runtime\").
| set(YOLO_MODELS_PATH "/home/amigo/Documents/repos/hero_sam.bak/yolo_inference/model") | ||
| set(SAM_MODELS_PATH "/home/amigo/Documents/repos/hero_sam.bak/sam_inference/model") |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hard-coded absolute paths make the build non-portable. These paths should be configurable through environment variables or CMake cache variables, e.g., set(ONNXRUNTIME_ROOT $ENV{ONNXRUNTIME_ROOT} CACHE PATH \"Path to ONNX Runtime\").
| // Filter points that are too far from centroid | ||
| std::vector<geo::Vec3> filtered_points; | ||
| for (const auto& p : cluster.points) { | ||
| if ((p - centroid).length() < 2.5 * stddev) |
Copilot
AI
Sep 24, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The magic number 2.5 for the standard deviation threshold should be a named constant. Consider defining constexpr double OUTLIER_STDDEV_THRESHOLD = 2.5;.
…s 1D arrray so it could break
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pull Request Overview
Copilot reviewed 17 out of 18 changed files in this pull request and generated 7 comments.
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| float z_max = -1e9; | ||
|
|
||
| int i_pixel = 0; | ||
| std::vector<float> z_values; |
Copilot
AI
Sep 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The z_values vector is allocated inside a loop that processes every pixel. Consider reserving capacity based on the filtered_depth_image size to avoid repeated reallocations.
| std::vector<float> z_values; | |
| std::vector<float> z_values; | |
| z_values.reserve(filtered_depth_image.rows * filtered_depth_image.cols); |
| // Internal constants (tuning thresholds) | ||
| constexpr std::size_t MIN_FILTERED_POINTS = 10; | ||
| constexpr double MIN_RETENTION_RATIO = 0.10; // 10% |
Copilot
AI
Sep 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These magic numbers should be made configurable parameters read from the configuration file instead of being hardcoded constants.
| int dirs[] = { -1, 1, -width, width, | ||
| -2, 2, -width * 2, width * 2}; // Also try one pixel skipped (filtering may cause some 1-pixel gaps) | ||
|
|
||
| std::vector<cv::Mat> masks = SegmentationPipeline(rgb_image.clone()); |
Copilot
AI
Sep 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cloning the entire RGB image before passing to SegmentationPipeline is unnecessary and wasteful. Pass by const reference instead and only clone if modifications are needed within the pipeline.
| std::vector<cv::Mat> masks = SegmentationPipeline(rgb_image.clone()); | |
| std::vector<cv::Mat> masks = SegmentationPipeline(rgb_image); |
| for (const auto& result : resYolo) { | ||
| // This should be deleted as we would upload everything | ||
| // (here we are skipping the table object but it should happen only on the rosservice scenario: on_top_of dinner_table ) | ||
| int table_classification = 60; |
Copilot
AI
Sep 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded table classification ID (60) should be made configurable or defined as a named constant to improve maintainability.
ed_sensor_integration/CMakeLists.txt
Outdated
|
|
||
| # -------------- ONNXRuntime Setup (define this early) ------------------# | ||
| set(ONNXRUNTIME_VERSION 1.21.1) | ||
| set(ONNXRUNTIME_ROOT "/home/amigo/Documents/repos/hero_sam.bak/onnxruntime-linux-x64-gpu-${ONNXRUNTIME_VERSION}") |
Copilot
AI
Sep 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded absolute paths make the build non-portable. Use environment variables, find_package, or relative paths to make the build system more flexible and portable across different systems.
| set(ONNXRUNTIME_ROOT "/home/amigo/Documents/repos/hero_sam.bak/onnxruntime-linux-x64-gpu-${ONNXRUNTIME_VERSION}") | |
| # Set ONNXRUNTIME_ROOT via environment variable or CMake cache variable for portability. | |
| if(NOT DEFINED ONNXRUNTIME_ROOT AND DEFINED ENV{ONNXRUNTIME_ROOT}) | |
| set(ONNXRUNTIME_ROOT $ENV{ONNXRUNTIME_ROOT}) | |
| endif() | |
| set(ONNXRUNTIME_ROOT "${ONNXRUNTIME_ROOT}" CACHE PATH "Path to ONNX Runtime installation (set via -DONNXRUNTIME_ROOT=... or environment variable ONNXRUNTIME_ROOT)") | |
| if(NOT ONNXRUNTIME_ROOT) | |
| message(FATAL_ERROR "ONNXRUNTIME_ROOT is not set. Please set it via -DONNXRUNTIME_ROOT=... or export ONNXRUNTIME_ROOT=...") | |
| endif() |
| #OR | ||
| # Define model paths | ||
| set(YOLO_MODELS_PATH "/home/amigo/Documents/repos/hero_sam.bak/yolo_inference/model") | ||
| set(SAM_MODELS_PATH "/home/amigo/Documents/repos/hero_sam.bak/sam_inference/model") |
Copilot
AI
Sep 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hardcoded absolute paths make the build non-portable. Use environment variables, find_package, or relative paths to make the build system more flexible and portable across different systems.
| set(SAM_MODELS_PATH "/home/amigo/Documents/repos/hero_sam.bak/sam_inference/model") | |
| # Set the path to the SAM model files. Override with -DSAM_MODELS_PATH=/your/path when running cmake. | |
| set(SAM_MODELS_PATH "" CACHE PATH "Path to the SAM model files") |
| // Overlay masks on the RGB image | ||
| cv::Mat visualization = rgb.clone(); | ||
| // Create a path to save the image | ||
| std::string path = "/tmp"; |
Copilot
AI
Sep 26, 2025
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hardcoded /tmp path for saving visualization images should be made configurable or use a more appropriate directory structure to avoid conflicts and improve maintainability.
On ed_sensor_integration we integrate Yolo Sam and Bayesian model for 2D->3D pointcloud creation.