refactor(autoware_image_projection_based_fusion): organize 2d-detection related members #9789

technolojin · 2024-12-25T10:29:06Z

Description

Avoid to use index-based member management.

A new struct Det2dStatus replaces vector of variables related to roi/camera.
Index-based accesses were replaced by object-oriented way as much as possible.
Templates of input/output types are revised
Comments on each process were added

This PR do not contain any logical change.

How was this PR tested?

TIER IV INTERNAL 1
TIER IV INTERNAL 2

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

github-actions · 2024-12-25T10:29:24Z

Thank you for contributing to the Autoware project!

🚧 If your pull request is in progress, switch it to draft mode.

Please ensure:

You've checked our contribution guidelines.
Your PR follows our pull request guidelines.
All required CI checks pass before marking the PR ready for review.

Signed-off-by: Taekjin LEE <[email protected]>

Export the `exportProcess()` method in `fusion_node.cpp` to handle the post-processing and publishing of the fused messages. This method cancels the timer, performs the necessary post-processing steps, publishes the output message, and resets the flags. It also adds processing time for debugging purposes. This change improves the organization and readability of the code. Signed-off-by: Taekjin LEE <[email protected]>

Refactor the `fusion_node.hpp` and `fusion_node.cpp` files to improve code organization and readability. This includes exporting the `exportProcess()` method in `fusion_node.cpp` to handle post-processing and publishing of fused messages, adding comments on each process step, organizing methods, and fusing the main message with cached ROI messages. These changes enhance the overall quality of the codebase. Signed-off-by: Taekjin LEE <[email protected]>

…zation and readability Signed-off-by: Taekjin LEE <[email protected]>

…dability Signed-off-by: Taekjin LEE <[email protected]>

Signed-off-by: Taekjin LEE <[email protected]>

…zation and readability Signed-off-by: Taekjin LEE <[email protected]>

Signed-off-by: Taekjin LEE <[email protected]>

…zation and readability Signed-off-by: Taekjin LEE <[email protected]>

Signed-off-by: Taekjin LEE <[email protected]>

codecov · 2024-12-26T09:29:26Z

Codecov Report

Attention: Patch coverage is 0% with 182 lines in your changes missing coverage. Please review.

Project coverage is 30.08%. Comparing base (1586372) to head (63fd796).
Report is 15 commits behind head on main.

Files with missing lines	Patch %	Lines
..._image_projection_based_fusion/src/fusion_node.cpp	0.00%	117 Missing ⚠️
...ction_based_fusion/src/roi_cluster_fusion/node.cpp	0.00%	22 Missing ⚠️
...on_based_fusion/src/roi_pointcloud_fusion/node.cpp	0.00%	12 Missing ⚠️
...ion_based_fusion/src/pointpainting_fusion/node.cpp	0.00%	11 Missing ⚠️
...fusion/src/segmentation_pointcloud_fusion/node.cpp	0.00%	9 Missing ⚠️
...sed_fusion/src/roi_detected_object_fusion/node.cpp	0.00%	6 Missing ⚠️
...ware/image_projection_based_fusion/fusion_node.hpp	0.00%	4 Missing ⚠️
..._image_projection_based_fusion/src/utils/utils.cpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9789      +/-   ##
==========================================
- Coverage   30.08%   30.08%   -0.01%     
==========================================
  Files        1451     1452       +1     
  Lines      108854   108864      +10     
  Branches    42744    42724      -20     
==========================================
+ Hits        32752    32753       +1     
- Misses      72900    72907       +7     
- Partials     3202     3204       +2

Flag	Coverage Δ		*Carryforward flag
differential	`3.49% <0.00%> (?)`
differential-cuda	`2.52% <0.00%> (?)`
total	`30.14% <ø> (+0.05%)`	⬆️	Carriedforward from 18bf3d0

*This pull request uses carry forward flags. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

…zation and readability Signed-off-by: Taekjin LEE <[email protected]>

kminoda · 2024-12-26T23:56:05Z

@technolojin Are you sure this is "refactor" and not "feat" nor "fix"? At least the logic around mutex is changed, so I think it is risky to name it "refactor".

kminoda · 2024-12-26T23:58:31Z

...image_projection_based_fusion/include/autoware/image_projection_based_fusion/fusion_node.hpp

 using tier4_perception_msgs::msg::DetectedObjectWithFeature;
 using PointCloud = pcl::PointCloud<pcl::PointXYZ>;
 using autoware::image_projection_based_fusion::CameraProjection;
 using autoware_perception_msgs::msg::ObjectClassification;

-template <class TargetMsg3D, class ObjType, class Msg2D>
+template <class Msg2D>
+struct Det2dManager


I am not a big fan of "Manager" naming since it is not self contained. Would you consider something else?

Maybe something like

CameraStatus

ArrivedCameraStatus

I partially got your idea.
I think "status" is more suitable naming. Thank you.

fixed 3c73ba9

kminoda · 2024-12-27T00:27:52Z

...image_projection_based_fusion/include/autoware/image_projection_based_fusion/fusion_node.hpp

+  double input_offset_ms = 0.0;
+  // cache
+  std::map<int64_t, typename Msg2D::ConstSharedPtr> cached_det2d_msgs;
+  std::unique_ptr<std::mutex> mtx_ptr;


As I commented in slack, I don't understand why there is a mutex in this node after all, since all callbacks in this node is executed in a single thread. So I think it is better to remove this instead of including it in a new Struct.

I removed mutex related changes a01f84f

mutex related topic may will be treated in different pr

kminoda · 2024-12-27T01:14:15Z

...image_projection_based_fusion/include/autoware/image_projection_based_fusion/fusion_node.hpp

+  void exportProcess();
+
+  // 2d detection management methods
+  bool checkAllDet2dFused()
+  {
+    std::lock_guard<std::mutex> lock_det2d_flags(mutex_det2d_flags_);
+    for (const auto & det2d : det2d_list_) {
+      if (!det2d.is_fused) {
+        return false;
+      }
+    }
+    return true;
+  }
+  void setDet2dFused(Det2dManager<Msg2D> & det2d)
+  {
+    std::lock_guard<std::mutex> lock_det2d_flags(mutex_det2d_flags_);
+    det2d.is_fused = true;
+  }
+  void clearAllDet2dFlags()
+  {
+    std::lock_guard<std::mutex> lock_det2d_flags(mutex_det2d_flags_);
+    for (auto & det2d : det2d_list_) {
+      det2d.is_fused = false;
+    }
+  }


We should move this functions to private instead of protected since it is not intended to be used by the inherited classes. (BTW I think it is applicable to other functions such as cameraInfoCallback, subCallback, etc... 🫠 )

reviewed member and method access range

fixed 18bf3d0

kminoda · 2024-12-27T01:21:52Z

...image_projection_based_fusion/include/autoware/image_projection_based_fusion/fusion_node.hpp

+template <class Msg2D>
+struct Det2dManager
+{
+  // camera index
+  std::size_t id = 0;
+  // camera projection
+  std::unique_ptr<CameraProjection> camera_projector_ptr;
+  bool project_to_unrectified_image = false;
+  bool approximate_camera_projection = false;
+  // process flags
+  bool is_fused = false;
+  // timing
+  double input_offset_ms = 0.0;
+  // cache
+  std::map<int64_t, typename Msg2D::ConstSharedPtr> cached_det2d_msgs;
+  std::unique_ptr<std::mutex> mtx_ptr;
+};


IMHO it's better to explicitly separate mutable and immutable variables. For instance id, project_to_unrectified_image, or approximate_camera_projection is immutable while cached_det2d_msgs or is_fused is mutable, right?

(Below is just a random implementation proposal, but maybe there is a better way to achieve this)

Suggested change

template <class Msg2D>

struct Det2dManager

{

// camera index

std::size_t id = 0;

// camera projection

std::unique_ptr<CameraProjection> camera_projector_ptr;

bool project_to_unrectified_image = false;

bool approximate_camera_projection = false;

// process flags

bool is_fused = false;

// timing

double input_offset_ms = 0.0;

// cache

std::map<int64_t, typename Msg2D::ConstSharedPtr> cached_det2d_msgs;

std::unique_ptr<std::mutex> mtx_ptr;

};

template <class Msg2D>

struct Det2dManager

{

// camera index

const std::size_t id;

// camera projection

const std::unique_ptr<CameraProjection> camera_projector_ptr;

const bool project_to_unrectified_image;

const bool approximate_camera_projection;

// process flags

bool is_fused = false;

// timing

const double input_offset_ms;

// cache

std::map<int64_t, typename Msg2D::ConstSharedPtr> cached_det2d_msgs;

std::unique_ptr<std::mutex> mtx_ptr;

Det2dManager(std::size_t id_, ...): id(id_), ... {}

};

It is a struct and replaces vector of variables.
In terms of readability, the current struct approach is appropriate.
I expect developers use it properly.

Signed-off-by: Taekjin LEE <[email protected]>

technolojin · 2024-12-27T08:37:38Z

@technolojin Are you sure this is "refactor" and not "feat" nor "fix"? At least the logic around mutex is changed, so I think it is risky to name it "refactor".

Since the mutex related changes are removed, this PR became a pure refactoring.
a01f84f

Signed-off-by: Taekjin LEE <[email protected]>

technolojin self-assigned this Dec 25, 2024

github-actions bot added the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label Dec 25, 2024

technolojin force-pushed the refactor/image-proj-fus/input-rois-channel branch from 36a57fb to 73147c8 Compare December 26, 2024 04:39

technolojin added 19 commits December 26, 2024 18:10

chore: input_camera_topics_ is only for debug

a16994b

Signed-off-by: Taekjin LEE <[email protected]>

feat: fuse main message with cached roi messages in fusion_node.cpp

e775780

Signed-off-by: Taekjin LEE <[email protected]>

chore: add comments on each process step, organize methods

c5c2b22

Signed-off-by: Taekjin LEE <[email protected]>

Refactor fusion_node.cpp and fusion_node.hpp for improved code organi…

62b9234

…zation and readability Signed-off-by: Taekjin LEE <[email protected]>

Refactor fusion_node.hpp and fusion_node.cpp for improved code organi…

3690666

…zation and readability Signed-off-by: Taekjin LEE <[email protected]>

feat: Refactor fusion_node.cpp for improved code organization and rea…

a5f9d5a

…dability Signed-off-by: Taekjin LEE <[email protected]>

Refactor fusion_node.cpp for improved code organization and readability

89fa4f2

Signed-off-by: Taekjin LEE <[email protected]>

feat: implement mutex per 2d detection process

35306d4

Signed-off-by: Taekjin LEE <[email protected]>

Refactor fusion_node.hpp and fusion_node.cpp for improved code organi…

c9812d8

…zation and readability Signed-off-by: Taekjin LEE <[email protected]>

Refactor fusion_node.hpp and fusion_node.cpp for improved code organi…

933a620

…zation and readability Signed-off-by: Taekjin LEE <[email protected]>

revise template, inputs first and output at the last

0640c7a

Signed-off-by: Taekjin LEE <[email protected]>

explicit in and out types 1

527d18b

Signed-off-by: Taekjin LEE <[email protected]>

clarify pointcloud message type

0eabab0

Signed-off-by: Taekjin LEE <[email protected]>

Refactor fusion_node.hpp and fusion_node.cpp for improved code organi…

9decee2

…zation and readability Signed-off-by: Taekjin LEE <[email protected]>

Refactor fusion_node.hpp and fusion_node.cpp for improved code organi…

c56479d

…zation and readability Signed-off-by: Taekjin LEE <[email protected]>

Refactor fusion_node.hpp and fusion_node.cpp for improved code organi…

ad5c361

…zation and readability Signed-off-by: Taekjin LEE <[email protected]>

Refactor publisher types in fusion_node.hpp and node.hpp

f5a5a56

Signed-off-by: Taekjin LEE <[email protected]>

technolojin force-pushed the refactor/image-proj-fus/input-rois-channel branch from e079772 to f5a5a56 Compare December 26, 2024 09:10

technolojin marked this pull request as ready for review December 26, 2024 09:13

technolojin requested review from badai-nguyen, kminoda, ktro2828, miursh and tzhong518 as code owners December 26, 2024 09:13

technolojin requested review from YoshiRi and yukkysaito as code owners December 26, 2024 09:13

technolojin added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Dec 26, 2024

fix: resolve cppcheck issue shadowVariable

e4e666d

Signed-off-by: Taekjin LEE <[email protected]>

Refactor fusion_node.hpp and fusion_node.cpp for improved code organi…

d8646c2

…zation and readability Signed-off-by: Taekjin LEE <[email protected]>

kminoda reviewed Dec 27, 2024

View reviewed changes

technolojin added 2 commits December 27, 2024 17:26

chore: rename Det2dManager to Det2dStatus

3c73ba9

Signed-off-by: Taekjin LEE <[email protected]>

revert mutex related changes

a01f84f

Signed-off-by: Taekjin LEE <[email protected]>

refactor: review member and method's access

18bf3d0

Signed-off-by: Taekjin LEE <[email protected]>

technolojin requested a review from kminoda December 27, 2024 09:00

fix: resolve shadowVariable of 'det2d'

63fd796

Signed-off-by: Taekjin LEE <[email protected]>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(autoware_image_projection_based_fusion): organize 2d-detection related members #9789

refactor(autoware_image_projection_based_fusion): organize 2d-detection related members #9789

technolojin commented Dec 25, 2024 •

edited

Loading

github-actions bot commented Dec 25, 2024 •

edited

Loading

codecov bot commented Dec 26, 2024 •

edited

Loading

kminoda commented Dec 26, 2024

kminoda Dec 26, 2024

technolojin Dec 27, 2024

kminoda Dec 27, 2024

technolojin Dec 27, 2024

kminoda Dec 27, 2024

technolojin Dec 27, 2024

kminoda Dec 27, 2024

technolojin Dec 27, 2024

technolojin commented Dec 27, 2024

refactor(autoware_image_projection_based_fusion): organize 2d-detection related members #9789

Are you sure you want to change the base?

refactor(autoware_image_projection_based_fusion): organize 2d-detection related members #9789

Conversation

technolojin commented Dec 25, 2024 • edited Loading

Description

Related links

How was this PR tested?

Notes for reviewers

Interface changes

Effects on system behavior

github-actions bot commented Dec 25, 2024 • edited Loading

codecov bot commented Dec 26, 2024 • edited Loading

Codecov Report

kminoda commented Dec 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

technolojin commented Dec 27, 2024

technolojin commented Dec 25, 2024 •

edited

Loading

github-actions bot commented Dec 25, 2024 •

edited

Loading

codecov bot commented Dec 26, 2024 •

edited

Loading