Skip to content
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.

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

fix(object_association_merger_node): fix the frame id of output object msg #8674

Conversation

vividf
Copy link
Contributor

@vividf vividf commented Aug 29, 2024

Description

This PR solves the issue #8530.

If the frame id of output_msg is not base_link, the merged object will have the wrong coordinate.
This issue happens because all of the objects (0 and 1) are transformed to base_link, but the frame of output_msg is from input_objects0_msg->header

Before fixed:
before_fixed

After fixed:
after_fixed

Related links

Parent Issue:

  • Link

How was this PR tested?

  • rosbag: sample rosbag
  • CenterPoint detection apply on top pointcloud
ros2 launch autoware_launch logging_simulator.launch.xml map_path:=$HOME/autoware_map/sample-map-rosbag vehicle_model:=sample_vehicle sensor_model:=sample_sensor_kit

Notes for reviewers

None.

Interface changes

None.

Effects on system behavior

None.

Signed-off-by: vividf <yihsiang.fang@tier4.jp>
@vividf vividf self-assigned this Aug 29, 2024
@github-actions github-actions bot added component:perception Advanced sensor data processing and environment understanding. (auto-assigned) tag:require-cuda-build-and-test labels Aug 29, 2024
Copy link

github-actions bot commented Aug 29, 2024

Thank you for contributing to the Autoware project!

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

Please ensure:

@cyn-liu
Copy link
Contributor

cyn-liu commented Aug 29, 2024

Thank you very much for your fix, but I think there are some problem with this PR.

I don't think this bug should be fixed in this way. In theory, the frame_id of the two input topic of node object_merger should be base_link, so output_msg.header = input_objects0_msg->header is not a bug.
The real problem is that the frame_id of input/object0 for node object_merger is lidar frame_id instead of base_link, so we should modify the frame_id of node lidar_centerpoint output topic to base_link.

What do you think?

@vividf
Copy link
Contributor Author

vividf commented Aug 29, 2024

@cyn-liu
Thanks for your comment.
However, I don't think restricting the output of the centerpoint would be a better option. Since centerpoint should be able to run without providing the baselink frame and I think the object frame should also be the same as the pointcloud frame.

Since this node assigns the header of the output_msg to be the same as the input_objects0_msg, and the objects are transformed and merged in the base_link (which also means that the input can be in the different frame), I think setting the output frame id to base_link here probably is a better option.

@technolojin @YoshiRi @yukkysaito What do you think, thanks!

@vividf vividf added the run:build-and-test-differential Mark to enable build-and-test-differential workflow. (used-by-ci) label Aug 29, 2024
Copy link

codecov bot commented Aug 29, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 24.12%. Comparing base (21ce55f) to head (6a8ba53).
Report is 3 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8674      +/-   ##
==========================================
- Coverage   24.12%   24.12%   -0.01%     
==========================================
  Files        1400     1400              
  Lines      102282   102295      +13     
  Branches    38782    38785       +3     
==========================================
  Hits        24675    24675              
- Misses      75124    75137      +13     
  Partials     2483     2483              
Flag Coverage Δ *Carryforward flag
differential 0.00% <ø> (?)
total 24.12% <ø> (ø) Carriedforward from 21ce55f

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

Copy link
Contributor

@technolojin technolojin left a comment

Choose a reason for hiding this comment

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

I think it was a bug, and will be fixed by this PR.

LGTM

Copy link
Contributor

@yukkysaito yukkysaito left a comment

Choose a reason for hiding this comment

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

LGTM

@vividf vividf enabled auto-merge (squash) August 30, 2024 01:35
@vividf vividf merged commit ee38d95 into autowarefoundation:main Aug 30, 2024
31 checks passed
@cyn-liu
Copy link
Contributor

cyn-liu commented Aug 30, 2024

@vividf Thanks for your fix this bug.

By the way, I want ask a question: What is the mean of parameter world_frame_id: map in package autoware_lidar_centerpoint? I thought this parameter controlled the output frame_id of node lidar_centerpoint, but after verification, it has no relationship with the output frame_id.

a-maumau pushed a commit to a-maumau/autoware.universe that referenced this pull request Sep 2, 2024
…t msg (autowarefoundation#8674)

fix: fix the object msg header

Signed-off-by: vividf <yihsiang.fang@tier4.jp>
@vividf
Copy link
Contributor Author

vividf commented Sep 3, 2024

@cyn-liu
From the documentation https://autowarefoundation.github.io/autoware.universe/main/perception/autoware_lidar_centerpoint/
image

ktro2828 pushed a commit to ktro2828/autoware.universe that referenced this pull request Sep 18, 2024
…t msg (autowarefoundation#8674)

fix: fix the object msg header

Signed-off-by: vividf <yihsiang.fang@tier4.jp>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
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) tag:require-cuda-build-and-test
Projects
No open projects
Development

Successfully merging this pull request may close these issues.

4 participants