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

feat: add ByteTrack package #3023

Merged
merged 19 commits into from
Mar 27, 2023
Merged

Conversation

manato
Copy link
Contributor

@manato manato commented Mar 8, 2023

Description

This PR adds ByteTrack packages to the perception stack.
To explicitly discriminate the ByteTrack function itself and visualization utility, this package consists of two nodes:

  • bytetrack: the interface between ByteTrack and Autoware
  • bytetrack_visualizer: the utility node to draw tracking results on images

Related links

TIER IV INTERNAL

Tests performed

Before performing tests, make sure that /sensing/camera/camera0/image_rect_color/compressed, which is the default topic to be subscribed by yolox and bytetrack nodes, is streamed on the system.

$ ros2 launch tensorrt_yolox yolox.launch.xml
$ ros2 launch bytetrack bytetrack.launch.xml
# Since the visualization results are published as `/bytetrack_visualizer/out/image`,
# you can see them using viewers you prefer
$ ros2 run image_view image_view --ros-args -r image:=/bytetrack_visualizer/out/image

Notes for reviewers

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

  • The PR follows the pull request guidelines.
  • The PR has been properly tested.
  • The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

  • There are no open discussions or they are tracked via tickets.
  • The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

@github-actions github-actions bot added type:documentation Creating or refining documentation. (auto-assigned) component:perception Advanced sensor data processing and environment understanding. (auto-assigned) labels Mar 8, 2023
@manato manato requested review from yukkysaito and miursh March 8, 2023 06:58
@@ -0,0 +1,62 @@
// Copyright 2022 Tier IV, Inc.
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
// Copyright 2022 Tier IV, Inc.
// Copyright 2023 TIER IV, Inc.

Copy link
Contributor Author

@manato manato Mar 8, 2023

Choose a reason for hiding this comment

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

This should also be handled in the modification at 0408d43

Copy link
Contributor

@yukkysaito yukkysaito Mar 8, 2023

Choose a reason for hiding this comment

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

Is it OK to add the license in the lib files? since it is external files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Because the files under lib directory have been slightly modified to follow the autoware coding guideline (also, some of them are renamed to be lower-snake-case), I guess TIER IV's license statements might be allowed to be there. If you think the statements should not be there, I'll remove them, so could you let me know your thought?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thank you for your reply.
IMO,

  • Only changing lower-snake-case is difficult to claim as a copyrighted work.
  • Also, the original license is not written in the file. It looks like a TIER IV work.

So, If you want to claim it as a copyrighted work, it it better to write it under the original license.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@yukkysaito
I appreciate your instruction. By adding copyright statements, I just would like to indicate these codes are not pure copies and need attention to read them. I agree with your opinion, so I re-arranged the order of the copyright statements (55b00a1)

@yukkysaito
Copy link
Contributor

If you have a time, Could you please share a demo video?

@manato
Copy link
Contributor Author

manato commented Mar 9, 2023

If you have a time, Could you please share a demo video?

@yukkysaito
Of course, I'll make one. Let me confirm some points:

  • Do you intend to add the demo video to README or use it for another purpose?
    • I wonder which kind of data (TIER IV's internal or some public ones) I should use
  • Do you prefer sharing the video here?

@yukkysaito
Copy link
Contributor

yukkysaito commented Mar 9, 2023

Of course, I'll make one. Let me confirm some points:
Do you intend to add the demo video to README or use it for another purpose?
I wonder which kind of data (TIER IV's internal or some public ones) I should use
Do you prefer sharing the video here?

@manato Thank you for the question.

  • For reviewers to judge to approve of this PR.
  • To help people who use this package.

So, if possible, it might be better to have them take it as a gif, etc. and put it on the readme.

@manato
Copy link
Contributor Author

manato commented Mar 17, 2023

@yukkysaito
Thank you for the response. Now I understand your intention.
I made a demo video and put it on the README (6949c6f). I'd appreciate it if you check whether it looks acceptable. Besides, thanks to making the video, I noticed a few problems in the codes. So I include the modification (50a1d1a) in this PullReq.

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

Co-authored-by: Kenji Miyake <31987104+kenji-miyake@users.noreply.github.com>
@codecov
Copy link

codecov bot commented Mar 27, 2023

Codecov Report

Patch coverage has no change and project coverage change: -0.10 ⚠️

Comparison is base (b28d792) 11.52% compared to head (40f0cb8) 11.43%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3023      +/-   ##
==========================================
- Coverage   11.52%   11.43%   -0.10%     
==========================================
  Files        1314     1324      +10     
  Lines       92794    93532     +738     
  Branches    24997    24997              
==========================================
  Hits        10699    10699              
- Misses      70853    71591     +738     
  Partials    11242    11242              
Flag Coverage Δ *Carryforward flag
differential 0.00% <0.00%> (?)
total 11.52% <ø> (ø) Carriedforward from b28d792

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

Impacted Files Coverage Δ
...ck/include/bytetrack/bytetrack_visualizer_node.hpp 0.00% <0.00%> (ø)
perception/bytetrack/lib/include/byte_tracker.h 0.00% <0.00%> (ø)
perception/bytetrack/lib/src/byte_tracker.cpp 0.00% <0.00%> (ø)
perception/bytetrack/lib/src/kalman_filter.cpp 0.00% <0.00%> (ø)
perception/bytetrack/lib/src/lapjv.cpp 0.00% <0.00%> (ø)
perception/bytetrack/lib/src/strack.cpp 0.00% <0.00%> (ø)
perception/bytetrack/lib/src/utils.cpp 0.00% <0.00%> (ø)
perception/bytetrack/src/bytetrack.cpp 0.00% <0.00%> (ø)
perception/bytetrack/src/bytetrack_node.cpp 0.00% <0.00%> (ø)
...eption/bytetrack/src/bytetrack_visualizer_node.cpp 0.00% <0.00%> (ø)

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report in Codecov by Sentry.
📢 Do you have feedback about the report comment? Let us know in this issue.

@manato manato merged commit 49722f9 into autowarefoundation:main Mar 27, 2023
@manato manato deleted the feat/bytetrack branch March 27, 2023 03:12
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) type:documentation Creating or refining documentation. (auto-assigned)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants