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
TIER IV INTERNAL 3

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.

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

Codecov Report

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

Project coverage is 29.62%. Comparing base (1e68602) to head (b819259).
Report is 2 commits behind head on main.

Files with missing lines	Patch %	Lines
..._image_projection_based_fusion/src/fusion_node.cpp	0.00%	134 Missing ⚠️
...on_based_fusion/src/roi_pointcloud_fusion/node.cpp	0.00%	21 Missing ⚠️
...fusion/src/segmentation_pointcloud_fusion/node.cpp	0.00%	18 Missing ⚠️
...ction_based_fusion/src/roi_cluster_fusion/node.cpp	0.00%	16 Missing ⚠️
...sed_fusion/src/roi_detected_object_fusion/node.cpp	0.00%	15 Missing ⚠️
...ion_based_fusion/src/pointpainting_fusion/node.cpp	0.00%	10 Missing ⚠️
...ware/image_projection_based_fusion/fusion_node.hpp	0.00%	4 Missing ⚠️
...sed_fusion/segmentation_pointcloud_fusion/node.hpp	0.00%	3 Missing ⚠️
..._image_projection_based_fusion/src/utils/utils.cpp	0.00%	1 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #9789      +/-   ##
==========================================
- Coverage   29.72%   29.62%   -0.10%     
==========================================
  Files        1453     1453              
  Lines      108989   108841     -148     
  Branches    42627    42510     -117     
==========================================
- Hits        32392    32244     -148     
+ Misses      73484    73391      -93     
- Partials     3113     3206      +93

Flag	Coverage Δ		*Carryforward flag
differential	`3.50% <0.00%> (?)`
differential-cuda	`2.51% <0.00%> (?)`
total	`29.69% <0.00%> (-0.03%)`	⬇️	Carriedforward from dd60d8e

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

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

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

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

vividf

Thank you for your thoughtful refactoring! The updated code is significantly more readable and well-structured compared to the previous version.
In my opinion, there are likely areas within roi_pointcloud_fusion and pointpainting_fusion that could also be improved further.

perception/autoware_image_projection_based_fusion/src/roi_pointcloud_fusion/node.cpp

perception/autoware_image_projection_based_fusion/src/roi_cluster_fusion/node.cpp

kminoda

Would you consider making splitting this PR into several ones? Current changes are bit too many to assess.

My suggestion is:

PR 1: Simply replacing the type names with "using PointCloudMsgType", "using RoiMsgType"
PR 2: Introduce Det2dStatus

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

perception/autoware_image_projection_based_fusion/src/pointpainting_fusion/node.cpp

technolojin · 2025-01-07T00:40:26Z

Would you consider making splitting this PR into several ones? Current changes are bit too many to assess.

My suggestion is:
* PR 1: Simply replacing the type names with "using PointCloudMsgType", "using RoiMsgType"

* PR 2: Introduce `Det2dStatus`

I understand your point.
Unfortunately, those two changes are overlapped, therefore, it will take a big effort from now.
How much do you think separating PR is important? Is it worth 5h for example?

kminoda · 2025-01-07T00:53:34Z

@technolojin I see, I thought it is worth splitting if it only takes 20-30min, but if it takes 5hr then it's not necessary. Let me review this PR as it is.