Skip to content

feat(shape_estimation): correcting cluster yaw and refactoring #11309

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 3 commits into
base: main
Choose a base branch
from
Open

feat(shape_estimation): correcting cluster yaw and refactoring #11309

wants to merge 3 commits into from

Conversation

emuemuJP
Copy link

@emuemuJP emuemuJP commented Sep 2, 2025

Description

  • Correcting cluster yaw angle estimated with fitLShape function when a cluster exists in front of or behind the own behicle
  • Refactored the correctWithDefaultValue function in autoware_shape_estimation/lib/corrector/utils.cpp to improve code maintainability, performance, and floating-point comparison reliability.

Related links

Parent Issue:

  • Link

How was this PR tested?

test with the following rosbag

Notes for reviewers

None.

Interface changes

None.

@github-actions github-actions bot added the component:perception Advanced sensor data processing and environment understanding. (auto-assigned) label Sep 2, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Sep 2, 2025 •
edited
Loading

Thank you for contributing to the Autoware project!

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

Please ensure:

Owen-Liuyuxuan
Owen-Liuyuxuan requested changes Sep 3, 2025
View reviewed changes
Copy link
Contributor

@Owen-Liuyuxuan Owen-Liuyuxuan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emuemuJP

image

It seems that in this case, the shape estimation will be mis-corrected as straight.

My expectation is that vehicles, like this one in the graph, should be made sure not to extend to left but only right

@emuemuJP
Copy link
Author

emuemuJP commented Sep 3, 2025

@Owen-Liuyuxuan Is there any rosbag to check that problem is happening or not.

Owen-Liuyuxuan
Owen-Liuyuxuan reviewed Sep 3, 2025
View reviewed changes
perception/autoware_shape_estimation/lib/corrector/utils.cpp
bool first_in_length_range = (param.min_length < first_point_distance && first_point_distance < param.max_length);
bool second_in_width_range = (second_point_distance < param.max_width);
bool second_in_length_range = (param.min_length < second_point_distance && second_point_distance < param.max_length);
bool third_in_width_range = (third_point_distance < param.max_width);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the refactoring! That is very helpful.

@technolojin technolojin self-assigned this Sep 3, 2025
@technolojin technolojin added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Sep 3, 2025
@technolojin
Copy link
Contributor

@emuemuJP

Suggestion:

Can we separate the refactoring and the logic change?

  • refactoring is always welcomed
  • for the logic change, it need to tested and verified considering under various situations, so it may takes a lot of time

For the logic change, can you provide any visual comparison before and after this PR?
Example: #11230

a-maumau
a-maumau reviewed Sep 8, 2025
View reviewed changes
Copy link
Contributor

@a-maumau a-maumau left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@emuemuJP Hi, thanks for the refactoring! I also wanted to do something for this package.
I have some points to mention.

perception/autoware_shape_estimation/lib/corrector/utils.cpp
double second_point_distance = (v_point.at(second_most_distant_index) * 2.0).norm();
double third_point_distance = (v_point.at(third_most_distant_index) * 2.0).norm();
// Check if points are within parameter ranges
bool first_in_width_range = (first_point_distance < param.max_width);
Copy link
Contributor

@a-maumau a-maumau Sep 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I might misunderstanding something here, but why we don't need to check the param.min_width for *_in_width_range part?
In the original implementation, it was checking like param.min_width < (...).norm() && (...).norm() < param.max_width.

perception/autoware_shape_estimation/lib/model/bounding_box.cpp
Comment on lines +171 to +172
if (-FRONT_BACK_ANGLE_THRESHOLD <= cluster_angle_rad ||
cluster_angle_rad < FRONT_BACK_ANGLE_THRESHOLD) { // front
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is a bug.
Since || is used here, all the condition will go in this branch.
&& should be used.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@a-maumau a-maumau a-maumau left review comments

@Owen-Liuyuxuan Owen-Liuyuxuan Owen-Liuyuxuan requested changes

@kaancolak kaancolak Awaiting requested review from kaancolak kaancolak is a code owner

@ogrex ogrex Awaiting requested review from ogrex ogrex is a code owner

@technolojin technolojin Awaiting requested review from technolojin technolojin is a code owner

@YoshiRi YoshiRi Awaiting requested review from YoshiRi YoshiRi is a code owner

@yukkysaito yukkysaito Awaiting requested review from yukkysaito yukkysaito is a code owner

Requested changes must be addressed to merge this pull request.

Assignees

@technolojin technolojin

Labels
component:perception Advanced sensor data processing and environment understanding. (auto-assigned) run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci)
Projects
Software Working Group
Status: To Triage
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@emuemuJP @technolojin @a-maumau @Owen-Liuyuxuan