Skip to content

Conversation

IasonTheodorou
Copy link
Member

On ed_sensor_integration we integrate Yolo Sam and Bayesian model for 2D->3D pointcloud creation.

MatthijsBurgh and others added 30 commits February 11, 2025 21:14
…e refitConvexHull and one on cluster algorithm to append to the point cloud only points which have close depth to their neighbooring pixels.
Copy link

@Copilot Copilot AI left a 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.

Comment on lines 217 to 218
ROS_ERROR("No segmenter configuration found, cannot initialize Updater.");
throw std::runtime_error("No segmenter configuration found");
Copy link
Preview

Copilot AI Sep 24, 2025

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'.

Suggested change
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");

Copilot uses AI. Check for mistakes.

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) {
Copy link
Preview

Copilot AI Sep 24, 2025

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.

Suggested change
if (filtered_depth_image.at<float>(idx) > 0) {
if (filtered_depth_image.at<float>(y, x) > 0) {

Copilot uses AI. Check for mistakes.

Comment on lines 51 to 52
if (result.classId == 60) {
std::cout << "Class ID is: " << yoloDetector->classes[result.classId] << " So we dont append"<< std::endl;
Copy link
Preview

Copilot AI Sep 24, 2025

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.

Copilot uses AI. Check for mistakes.

{
// // Overlay masks on the RGB image
cv::Mat visualization = rgb.clone();
cv::imwrite("/tmp/visualization.png", visualization);
Copy link
Preview

Copilot AI Sep 24, 2025

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.

Copilot uses AI. Check for mistakes.

// 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);
Copy link
Preview

Copilot AI Sep 24, 2025

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.

Copilot uses AI. Check for mistakes.

cv::imwrite("/tmp/visualization_depth.png", depth_vis);
overlayMasksOnImage_(visualization, clustered_images);
// save after overlaying masks
cv::imwrite("/tmp/visualization_with_masks.png", visualization);
Copy link
Preview

Copilot AI Sep 24, 2025

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.

Copilot uses AI. Check for mistakes.

}
}
// Safety check
if (filtered_points.size() > 10 && filtered_points.size() > 0.1 * cluster.points.size()) {
Copy link
Preview

Copilot AI Sep 24, 2025

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;.

Copilot uses AI. Check for mistakes.


# -------------- 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}")
Copy link
Preview

Copilot AI Sep 24, 2025

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\").

Copilot uses AI. Check for mistakes.

Comment on lines +21 to +22
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")
Copy link
Preview

Copilot AI Sep 24, 2025

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\").

Copilot uses AI. Check for mistakes.

// 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)
Copy link
Preview

Copilot AI Sep 24, 2025

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;.

Copilot uses AI. Check for mistakes.

Copy link

@Copilot Copilot AI left a 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;
Copy link
Preview

Copilot AI Sep 26, 2025

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.

Suggested change
std::vector<float> z_values;
std::vector<float> z_values;
z_values.reserve(filtered_depth_image.rows * filtered_depth_image.cols);

Copilot uses AI. Check for mistakes.

Comment on lines +46 to +48
// Internal constants (tuning thresholds)
constexpr std::size_t MIN_FILTERED_POINTS = 10;
constexpr double MIN_RETENTION_RATIO = 0.10; // 10%
Copy link
Preview

Copilot AI Sep 26, 2025

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.

Copilot uses AI. Check for mistakes.

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());
Copy link
Preview

Copilot AI Sep 26, 2025

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.

Suggested change
std::vector<cv::Mat> masks = SegmentationPipeline(rgb_image.clone());
std::vector<cv::Mat> masks = SegmentationPipeline(rgb_image);

Copilot uses AI. Check for mistakes.

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;
Copy link
Preview

Copilot AI Sep 26, 2025

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.

Copilot uses AI. Check for mistakes.


# -------------- 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}")
Copy link
Preview

Copilot AI Sep 26, 2025

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.

Suggested change
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()

Copilot uses AI. Check for mistakes.

#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")
Copy link
Preview

Copilot AI Sep 26, 2025

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.

Suggested change
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")

Copilot uses AI. Check for mistakes.

// Overlay masks on the RGB image
cv::Mat visualization = rgb.clone();
// Create a path to save the image
std::string path = "/tmp";
Copy link
Preview

Copilot AI Sep 26, 2025

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.

Copilot uses AI. Check for mistakes.

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.

2 participants