Feature/refactoring #1

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

No description provided.

…rrecting preprocessing scalling issue on long images

…the custom result structs properly

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.

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

IasonTheodorou added 14 commits July 4, 2025 09:11

Initial refactoring

269a64c

create catkin package

3f0aa16

included CI tests

fe77196

update of CMakeLists to include some initial needed components

17bd100

Return the mask on SegmentAnything function (not working properly)

2ef2fc4

Updated CMake and removed not needed parts of the code

70fd58a

Updated code format

6c5c097

Small refactoring of the module

e10d45d

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

5cb4ab4

…rrecting preprocessing scalling issue on long images

Added tests (still not working with catkin)

0fe00da

Fixed catkin workspace for both code and tests

9f21012

fixed functionallity for the tests to pass and added logging definition

a9fce07

renamed private members of utils and sam_inference

dffbcd3

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

959a3ff

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

IasonTheodorou added 3 commits September 10, 2025 14:35

Updated initializer and SegmentAnything modules to store the data to …

fa97e64

…the custom result structs properly

Enabled cuda on the decoder as well

53989b6

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

7490f03

IasonTheodorou requested a review from Copilot September 10, 2025 14:34

Copilot AI reviewed Sep 10, 2025

View reviewed changes

IasonTheodorou added 2 commits September 12, 2025 12:13

Aligned dimensions [high width] between onnx and opencv

01da1fc

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

7cdf39a

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

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

Removed typo / from model path

b587a1c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Feature/refactoring #1

Feature/refactoring #1

Uh oh!

IasonTheodorou commented Sep 5, 2025

Uh oh!

Copilot AI left a comment

Uh oh!

Copilot AI Sep 10, 2025

Uh oh!

Copilot AI Sep 10, 2025

Uh oh!

Copilot AI Sep 10, 2025

Uh oh!

Copilot AI Sep 10, 2025

Uh oh!

Copilot AI Sep 10, 2025

Uh oh!

Copilot AI Sep 10, 2025

Uh oh!

Copilot AI Sep 10, 2025

Uh oh!

Uh oh!

	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)

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!

Uh oh!