Feature/refactoring #1

IasonTheodorou · 2025-09-05T13:24:50Z

No description provided.

Copilot

Pull Request Overview

This PR represents a significant refactoring of a Segment Anything Model (SAM) ONNX inference codebase, transitioning from a standalone C++ application to a ROS package with comprehensive testing infrastructure.

Key changes include:

Migration from standalone executable to ROS catkin package structure
Complete reorganization of include directories from inc/ to include/
Addition of comprehensive unit and integration tests using Google Test
Refactoring of core functionality with improved error handling and code organization

Reviewed Changes

Copilot reviewed 16 out of 17 changed files in this pull request and generated 7 comments.

Show a summary per file

File	Description
test/test_utils.cpp	New comprehensive unit tests for Utils class covering image preprocessing and coordinate scaling
test/sam_test.cpp	New integration tests for SAM inference pipeline with model file validation
src/utils.cpp	Refactored preprocessing and postprocessing with improved error handling and cleaner code structure
src/segmentation.cpp	New segmentation wrapper providing unified interface for SAM encoder/decoder operations
src/sam_inference.cpp	Major refactoring with consistent naming conventions and improved session management
src/main.cpp	Simplified main function using new segmentation interface
include/*.h	New header files replacing old inc/ structure with improved organization
CMakeLists.txt	Complete restructure for ROS catkin package with testing support
package.xml	New ROS package manifest

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-10T14:36:32Z

src/segmentation.cpp

+  std::vector<SEG::DL_RESULT> resSam;
+  params_encoder.rectConfidenceThreshold = 0.1;
+  params_encoder.iouThreshold = 0.5;
+  params_encoder.modelPath = "/home/amigo//Documents/repos/sam_onnx_ros/build/SAM_encoder.onnx";


Hard-coded absolute path with double slash '//' makes the code non-portable and fragile. Consider using relative paths or environment variables.

Copilot · 2025-09-10T14:36:32Z

src/segmentation.cpp

+
+  params_decoder = params_encoder;
+  params_decoder.modelType = SEG::SAM_SEGMENT_DECODER;
+  params_decoder.modelPath = "/home/amigo/Documents/repos/sam_onnx_ros/build/SAM_mask_decoder.onnx";


Hard-coded absolute path makes the code non-portable. Consider using relative paths or environment variables for better maintainability.

Copilot · 2025-09-10T14:36:33Z

src/main.cpp

+    SEG::DL_RESULT res;
+    std::tie(samSegmentors, params_encoder, params_decoder, res, resSam) = Initializer();
+    std::filesystem::path current_path = std::filesystem::current_path();
+    std::filesystem::path imgs_path =  "/home/amigo/Documents/repos/hero_sam/sam_inference/build/images"; // current_path / <- you could use


Hard-coded absolute path makes the code non-portable. The commented suggestion to use current_path / would be more maintainable.

Suggested change

std::filesystem::path imgs_path = "/home/amigo/Documents/repos/hero_sam/sam_inference/build/images"; // current_path / <- you could use

std::filesystem::path imgs_path = current_path / "build/images"; // Use current_path for portability

Copilot · 2025-09-10T14:36:33Z

CMakeLists.txt

 # -------------- ONNXRuntime  ------------------#
 set(ONNXRUNTIME_VERSION 1.21.0)
-set(ONNXRUNTIME_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/../onnxruntime-linux-x64-gpu-1.21.1")
+set(ONNXRUNTIME_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/../hero_sam.bak/onnxruntime-linux-x64-gpu-1.21.1")


Hard-coded relative path to a '.bak' directory is fragile and non-portable. Consider using find_package or environment variables to locate ONNX Runtime.

Suggested change

set(ONNXRUNTIME_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/../hero_sam.bak/onnxruntime-linux-x64-gpu-1.21.1")

# Allow user to specify ONNXRUNTIME_ROOT via CMake variable or environment variable

if(NOT DEFINED ONNXRUNTIME_ROOT)

if(DEFINED ENV{ONNXRUNTIME_ROOT})

set(ONNXRUNTIME_ROOT $ENV{ONNXRUNTIME_ROOT})

else()

message(FATAL_ERROR "ONNXRUNTIME_ROOT is not set. Please set the ONNXRUNTIME_ROOT environment variable or pass -DONNXRUNTIME_ROOT=... to cmake.")

endif()

endif()

Copilot · 2025-09-10T14:36:33Z

CMakeLists.txt

+configure_file(~/Documents/repos/hero_sam.bak/sam_inference/model/SAM_mask_decoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_mask_decoder.onnx  COPYONLY)
+configure_file(~/Documents/repos/hero_sam.bak/sam_inference/model/SAM_encoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_encoder.onnx COPYONLY)


Hard-coded absolute paths with tilde expansion and '.bak' directories are not portable and will fail on other systems. Consider using relative paths or making model paths configurable.

Copilot · 2025-09-10T14:36:34Z

test/sam_test.cpp

+        GTEST_SKIP() << "Models not found in build dir";
+    }
+
+    auto masks = SegmentAnything(samSegmentors, params_encoder, params_decoder, testImage_realistic);


Function call doesn't match the signature. SegmentAnything expects 6 parameters but only 4 are provided. Missing resSam and res parameters.

Copilot · 2025-09-10T14:36:34Z

src/segmentation.cpp

+void SegmentAnything(std::vector<std::unique_ptr<SAM>> &samSegmentors,
+                const SEG::DL_INIT_PARAM &params_encoder,
+                const SEG::DL_INIT_PARAM &params_decoder, const cv::Mat &img, std::vector<SEG::DL_RESULT> &resSam,
+  SEG::DL_RESULT &res) {


Function returns void but the test expects it to return a vector of masks. Consider returning the results or updating the test to match the actual function signature.

MatthijsBurgh

Fix missing newline at EOF

Copilot

Pull Request Overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 2 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-23T15:13:44Z

CMakeLists.txt

+configure_file(~/Documents/repos/hero_sam.bak/sam_inference/model/SAM_mask_decoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_mask_decoder.onnx  COPYONLY)
+configure_file(~/Documents/repos/hero_sam.bak/sam_inference/model/SAM_encoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_encoder.onnx COPYONLY)


Hard-coded paths containing user-specific directories and .bak folders make the build system non-portable. These should use relative paths or be configurable through CMake variables.

Suggested change

configure_file(~/Documents/repos/hero_sam.bak/sam_inference/model/SAM_mask_decoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_mask_decoder.onnx COPYONLY)

configure_file(~/Documents/repos/hero_sam.bak/sam_inference/model/SAM_encoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_encoder.onnx COPYONLY)

set(SAM_MODEL_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../hero_sam.bak/sam_inference/model" CACHE PATH "Directory containing SAM .onnx model files")

configure_file(${SAM_MODEL_DIR}/SAM_mask_decoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_mask_decoder.onnx COPYONLY)

configure_file(${SAM_MODEL_DIR}/SAM_encoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_encoder.onnx COPYONLY)

Copilot · 2025-09-23T15:13:45Z

src/sam_inference.cpp

    }
-    return RET_OK;
+
+    _outputNodeNames.size();


This line appears to be a leftover statement that has no effect. It should either be removed or used in a meaningful way (e.g., assigned to a variable or used in a function call).

Suggested change

_outputNodeNames.size();

Copilot

Pull Request Overview

Copilot reviewed 18 out of 19 changed files in this pull request and generated 3 comments.

_{Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.}

Copilot · 2025-09-26T10:49:49Z

src/segmentation.cpp

+
+  params_decoder = params_encoder;
+  params_decoder.modelType = SEG::SAM_SEGMENT_DECODER;
+  params_decoder.modelPath = "/home/amigo/Documents/repos/sam_onnx_ros/build/SAM_mask_decoder.onnx";


This hardcoded absolute path is not portable and will fail on other systems. Consider using a relative path, environment variable, or ROS parameter to make the code more maintainable.

Copilot · 2025-09-26T10:49:50Z

CMakeLists.txt

+configure_file(~/Documents/repos/hero_sam.bak/sam_inference/model/SAM_mask_decoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_mask_decoder.onnx  COPYONLY)
+configure_file(~/Documents/repos/hero_sam.bak/sam_inference/model/SAM_encoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_encoder.onnx COPYONLY)


These hardcoded paths with tilde expansion are not portable and will fail on other systems. Consider using environment variables or making these paths configurable through CMake options.

Copilot · 2025-09-26T10:49:50Z

test/sam_test.cpp

+    SegmentAnything(samSegmentors, params_encoder, params_decoder, testImage_realistic, resSam, res);
+
+    // We only check that a vector is returned. (You can strengthen this to EXPECT_FALSE(masks.empty()).)
+    EXPECT_TRUE(res.masks.size() >= 0) << "Masks should be a valid output vector";


This assertion will always pass since vector size() returns size_t which is always >= 0. Change to 'EXPECT_FALSE(res.masks.empty())' or 'EXPECT_GT(res.masks.size(), 0)' to test for meaningful output.

Suggested change

EXPECT_TRUE(res.masks.size() >= 0) << "Masks should be a valid output vector";

EXPECT_FALSE(res.masks.empty()) << "Masks should be a valid output vector";

…rrecting preprocessing scalling issue on long images

…the custom result structs properly

… and segment anything interfaces changed) and added one more test to check the image dimensions W,H

…e bridge

… and configuration .hpp.in file

MatthijsBurgh · 2025-10-27T08:00:11Z

.vscode/launch.json

            "type": "cppdbg",
            "request": "launch",
-            "program": "${workspaceFolder}/build/devel/lib/sam_onnx_ros/test_sam_onnx_ros", // Path to the executable
+            "program": "/home/amigo/ros/noetic/system/devel/lib/sam_onnx_ros/test11_sam_onnx_ros", // Path to the executable


This is not desired, as the location might not be the same for everyone.

MatthijsBurgh · 2025-10-27T08:00:43Z

src/main.cpp

    std::tie(samSegmentors, params_encoder, params_decoder, res, resSam) = Initializer();
    std::filesystem::path current_path = std::filesystem::current_path();
-    std::filesystem::path imgs_path =  "/home/amigo/Documents/repos/hero_sam.bak/sam_inference/build/images"; // current_path / <- you could use
+    std::filesystem::path imgs_path =  "/home/amigo/Documents/repos/yolo_onnx_ros/build/images"; // current_path / <- you could use


This path should also be more generic. So preferably it works for everyone.

MatthijsBurgh · 2025-10-27T08:01:11Z

src/sam_inference.cpp

-                utilities, result);
-
-  return Ret;
+const char* SAM::RunSession(const cv::Mat& iImg, std::vector<SEG::DL_RESULT>& oResult, SEG::MODEL_TYPE modelType, SEG::DL_RESULT& result) {


Suggested change

const char* SAM::RunSession(const cv::Mat& iImg, std::vector<SEG::DL_RESULT>& oResult, SEG::MODEL_TYPE modelType, SEG::DL_RESULT& result) {

const char* SAM::RunSession(const cv::Mat& iImg, std::vector<SEG::DL_RESULT>& oResult, SEG::MODEL_TYPE modelType, SEG::DL_RESULT& result)

{

MatthijsBurgh · 2025-10-27T08:02:24Z

src/sam_inference.cpp

-                utilities, result);
-
-  return Ret;
+const char* SAM::RunSession(const cv::Mat& iImg, std::vector<SEG::DL_RESULT>& oResult, SEG::MODEL_TYPE modelType, SEG::DL_RESULT& result) {


Do we really need to return a pointer? I think we can come up with a better system.

MatthijsBurgh · 2025-10-27T08:02:39Z

src/sam_inference.cpp

-    auto output_tensors =
-        _session->Run(_options, _inputNodeNames.data(), &input_tensor, 1,
-                     _outputNodeNames.data(), _outputNodeNames.size());
+  if (_modelType < 4) {


Suggested change

if (_modelType < 4) {

if (_modelType < 4)

{

MatthijsBurgh · 2025-10-27T08:05:31Z

src/utils.cpp

+        else if (shape.size() <= 2) scoresIdx = i;
+    }
+    if (masksIdx < 0) {
+        std::cerr << "[SAM]: No 4D mask tensor found in decoder outputs." << std::endl;


We should use console_bridge.

MatthijsBurgh · 2025-10-27T08:05:39Z

src/utils.cpp

-    auto masksTensor  = std::move(output_tensors[1]);
+    auto masksTensor = std::move(output_tensors[masksIdx]);
+    const float* scoresData = nullptr;
+    if (scoresIdx >= 0) {


Suggested change

if (scoresIdx >= 0) {

if (scoresIdx >= 0)

{

MatthijsBurgh · 2025-10-27T08:07:23Z

src/utils.cpp

+        if (scoresData) {
+            for (size_t i = 0; i < numMasks; ++i) {
+                const float s = scoresData[i];
+                if (s > bestScore) { bestScore = s; bestMaskIndex = i; }
+            }


Suggested change

if (scoresData) {

for (size_t i = 0; i < numMasks; ++i) {

const float s = scoresData[i];

if (s > bestScore) { bestScore = s; bestMaskIndex = i; }

}

if (scoresData)

{

for (size_t i = 0; i < numMasks; ++i)

{

const float s = scoresData[i];

if (s > bestScore)

{

bestScore = s;

bestMaskIndex = i;

}

}

MatthijsBurgh · 2025-10-27T08:07:35Z

src/utils.cpp

        int processedWidth, processedHeight;
-        if (iImg.cols >= iImg.rows)
-        {
+        if (iImg.cols >= iImg.rows) {


Suggested change

if (iImg.cols >= iImg.rows) {

if (iImg.cols >= iImg.rows)

{

MatthijsBurgh · 2025-10-27T08:07:45Z

src/utils.cpp

-        }
-        else
-        {
+        } else {


Suggested change

} else {

}

else

{

IasonTheodorou added 2 commits July 4, 2025 09:11

Initial refactoring

269a64c

create catkin package

3f0aa16

IasonTheodorou requested review from MatthijsBurgh and Copilot September 5, 2025 13:24

Copilot AI reviewed Sep 10, 2025

View reviewed changes

IasonTheodorou force-pushed the feature/Refactoring branch from 964d95f to 7cdf39a Compare September 12, 2025 10:21

MatthijsBurgh requested changes Sep 16, 2025

View reviewed changes

IasonTheodorou requested a review from Copilot September 23, 2025 15:12

Copilot AI reviewed Sep 23, 2025

View reviewed changes

IasonTheodorou force-pushed the feature/Refactoring branch from 1d6537d to 8aeb657 Compare September 24, 2025 14:22

IasonTheodorou requested a review from Copilot September 26, 2025 10:48

Copilot AI reviewed Sep 26, 2025

View reviewed changes

IasonTheodorou added 18 commits October 22, 2025 07:54

included CI tests

6ec4622

update of CMakeLists to include some initial needed components

2ae0d7b

Return the mask on SegmentAnything function (not working properly)

02af27f

Updated CMake and removed not needed parts of the code

7de1753

Updated code format

fbe8e00

Small refactoring of the module

a5a3c18

Refactor post processing for better accuracy and performance. Also co…

1957196

…rrecting preprocessing scalling issue on long images

Added tests (still not working with catkin)

e6623fc

Fixed catkin workspace for both code and tests

bdaf317

fixed functionallity for the tests to pass and added logging definition

bdab5a1

renamed private members of utils and sam_inference

8b7f913

Separrated test files per category (utils or sam related for now)

e1130c6

Updated initializer and SegmentAnything modules to store the data to …

c34410e

…the custom result structs properly

Enabled cuda on the decoder as well

69f3126

Fixed small bug of adding an extra (full img) bounding box

23d4790

Aligned dimensions [high width] between onnx and opencv

ce94fa8

corrected tests for the new segmentation way of inference (initialize…

ce19e96

… and segment anything interfaces changed) and added one more test to check the image dimensions W,H

Removed typo / from model path

24de2e5

MatthijsBurgh and others added 10 commits October 22, 2025 07:54

Bump min required cmake version to 3.14

53be89c

EOF line added and package.xml structure update

6ddc3e6

Update read me and include better comments

ee403f2

Deleted redundant code and fixed some brackets

33cde1a

made private member methods that were needed to be

80bd818

Fixed structurre of CMakeLists and package.xml and logged with consol…

ebe0539

…e bridge

updated CMakeLists, included .hpp suffix and sam_onnx_ros include dir…

0b63cef

… and configuration .hpp.in file

updated CMakeLists rosconsole bridge

b7a8338

updated CMakeLists for onnxruntime ros package and also .vscode configs

da16133

fix(cmake): opencv libraries

85b8df2

MatthijsBurgh force-pushed the feature/Refactoring branch from 94cb8c7 to 85b8df2 Compare October 22, 2025 05:55

MatthijsBurgh and others added 3 commits October 22, 2025 08:36

Add missing linking to opencv libs for test

a84c4f6

Correct linking of tests

9eced35

Checkpoint best working version

d9c1684

MatthijsBurgh requested changes Oct 27, 2025

View reviewed changes

MatthijsBurgh and others added 2 commits October 28, 2025 11:13

Various CMake fixes

515639d

Removed redundant .vscode lines

3591e21

	std::filesystem::path imgs_path = "/home/amigo/Documents/repos/hero_sam/sam_inference/build/images"; // current_path / <- you could use
	std::filesystem::path imgs_path = current_path / "build/images"; // Use current_path for portability

-set(ONNXRUNTIME_ROOT "${CMAKE_CURRENT_SOURCE_DIR}/../hero_sam.bak/onnxruntime-linux-x64-gpu-1.21.1")
+# Allow user to specify ONNXRUNTIME_ROOT via CMake variable or environment variable
+if(NOT DEFINED ONNXRUNTIME_ROOT)
+  if(DEFINED ENV{ONNXRUNTIME_ROOT})
+    set(ONNXRUNTIME_ROOT $ENV{ONNXRUNTIME_ROOT})
+  else()
+    message(FATAL_ERROR "ONNXRUNTIME_ROOT is not set. Please set the ONNXRUNTIME_ROOT environment variable or pass -DONNXRUNTIME_ROOT=... to cmake.")
+  endif()
+endif()

		configure_file(~/Documents/repos/hero_sam.bak/sam_inference/model/SAM_mask_decoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_mask_decoder.onnx COPYONLY)
		configure_file(~/Documents/repos/hero_sam.bak/sam_inference/model/SAM_encoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_encoder.onnx COPYONLY)

-configure_file(~/Documents/repos/hero_sam.bak/sam_inference/model/SAM_mask_decoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_mask_decoder.onnx  COPYONLY)
-configure_file(~/Documents/repos/hero_sam.bak/sam_inference/model/SAM_encoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_encoder.onnx COPYONLY)
+set(SAM_MODEL_DIR "${CMAKE_CURRENT_SOURCE_DIR}/../hero_sam.bak/sam_inference/model" CACHE PATH "Directory containing SAM .onnx model files")
+configure_file(${SAM_MODEL_DIR}/SAM_mask_decoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_mask_decoder.onnx COPYONLY)
+configure_file(${SAM_MODEL_DIR}/SAM_encoder.onnx ${CMAKE_CURRENT_BINARY_DIR}/SAM_encoder.onnx COPYONLY)

	EXPECT_TRUE(res.masks.size() >= 0) << "Masks should be a valid output vector";
	EXPECT_FALSE(res.masks.empty()) << "Masks should be a valid output vector";

	const char* SAM::RunSession(const cv::Mat& iImg, std::vector<SEG::DL_RESULT>& oResult, SEG::MODEL_TYPE modelType, SEG::DL_RESULT& result) {
	const char* SAM::RunSession(const cv::Mat& iImg, std::vector<SEG::DL_RESULT>& oResult, SEG::MODEL_TYPE modelType, SEG::DL_RESULT& result)
	{

Feature/refactoring #1

Are you sure you want to change the base?

Feature/refactoring #1

Uh oh!

Conversation

IasonTheodorou commented Sep 5, 2025

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Reviewed Changes

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

Uh oh!

MatthijsBurgh left a comment

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 23, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

Pull Request Overview

Uh oh!

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

Copilot AI Sep 26, 2025

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers