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

SiamRPN fixes and ab3dmot ros1 node duplicate shortcut #378

Merged
merged 6 commits into from
Dec 19, 2022

Conversation

tsampazk
Copy link
Collaborator

@tsampazk tsampazk commented Dec 14, 2022

Very minor fix for siamrpn node path.

It wasn't caught as by using catkin_make the scripts are not installed, i.e. only the devel directory is created and needs to be sourced, so this didn't produce any issue.

This is an thing that was brought up before, some nodes, but not all, are in the CMakeLists.txt catkin_install_python(PROGRAMS ... ) for ROS1. As far as i can tell, these are used when you do catkin_make install and source the install/setup.bash to run the nodes.

Edit: Integrated another minor fix that i missed earlier for object tracking 3d ab3dmot ROS1 node. The duplicate shortcut in argparse stopped the node from running entirely.

@tsampazk tsampazk added bug Something isn't working test sources Run style checks test tools Test the toolkit methods labels Dec 14, 2022
@tsampazk tsampazk self-assigned this Dec 14, 2022
ad-daniel
ad-daniel previously approved these changes Dec 14, 2022
Copy link
Collaborator

@ad-daniel ad-daniel left a comment

Choose a reason for hiding this comment

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

nice catch, thank you

passalis
passalis previously approved these changes Dec 14, 2022
Copy link
Collaborator

@passalis passalis left a comment

Choose a reason for hiding this comment

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

Thank you!

@tsampazk tsampazk dismissed stale reviews from passalis and ad-daniel via 9e0f839 December 14, 2022 12:11
@tsampazk tsampazk changed the title Fix siamrpn script name in CMakeLists Fix siamrpn script name in CMakeLists and ab3dmot ros1 node duplicate shortcut Dec 14, 2022
ad-daniel
ad-daniel previously approved these changes Dec 14, 2022
@tsampazk
Copy link
Collaborator Author

I added another fix on this PR, for siamrpn ros1 node to include a built-in detector to initialize the tracker, so as not to open a different PR.

Now the node initializes the tracker using an object detector. Out of the bboxes detected, the one closer to the input image center is chosen for initialization.

We've discussed this with @vivinousi, and concluded that it is the easiest way to initialize the tracker and use the node out-of-the-box.

I've already made this change in ROS2.

Copy link
Collaborator

@passalis passalis left a comment

Choose a reason for hiding this comment

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

Thank you!

@tsampazk tsampazk changed the title Fix siamrpn script name in CMakeLists and ab3dmot ros1 node duplicate shortcut SiamRPN fixes and ab3dmot ros1 node duplicate shortcut Dec 19, 2022
Copy link
Collaborator

@ad-daniel ad-daniel left a comment

Choose a reason for hiding this comment

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

Thank you both, looks good to me as well!

@ad-daniel ad-daniel merged commit c603ee0 into develop Dec 19, 2022
@ad-daniel ad-daniel deleted the ros-cmakelist-fix branch December 19, 2022 16:29
lucamarchionni pushed a commit to lucamarchionni/opendr that referenced this pull request Jun 10, 2024
* Fix siamrpn script name in CMakeLists

* Fix duplicate argparse shortcut tracking3d

* SiamRPN node now uses built-in object detector

* Minor comment fix

Co-authored-by: ad-daniel <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working test sources Run style checks test tools Test the toolkit methods
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants