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.

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

[jsk_pcl_ros] Add multi euclidean clustering #2463

Merged
merged 20 commits into from
Sep 3, 2020
Merged

[jsk_pcl_ros] Add multi euclidean clustering #2463

merged 20 commits into from
Sep 3, 2020

Conversation

iory
Copy link
Member

@iory iory commented Nov 3, 2019 •
edited
Loading

Add multi euclidean clustering as a jsk_pcl/EuclideanClustering's option.
This PR enables processing jsk_recognition_msgs/ClusterIndices's input at the same time.

The results of multi euclidean clustering as follows.
with multi euclidean clustering
https://drive.google.com/file/d/1ZMKwmq1TPtEdJ8mgWrSWlxiP58xU8QOu/view?usp=sharing

without multi euclidean clustering
https://drive.google.com/open?id=1vcRwDHxEnc2NbWidBeiEZau-nZGl2MXi

Those videos are in the same situation.

@iory iory requested a review from YutoUchimi November 3, 2019 00:01
@iory iory requested review from knorth55 and taichiH November 3, 2019 00:01
knorth55
knorth55 requested changes Nov 3, 2019
View reviewed changes
doc/jsk_pcl_ros/nodes/euclidean_clustering.md Outdated Show resolved Hide resolved
doc/jsk_pcl_ros/nodes/euclidean_clustering.md Outdated Show resolved Hide resolved
jsk_pcl_ros/src/euclidean_cluster_extraction_nodelet.cpp Outdated Show resolved Hide resolved
@iory iory force-pushed the multi-euclidean branch 2 times, most recently from 7de4478 to cfb2e8b Compare November 4, 2019 01:14
@iory
Copy link
Member Author

iory commented Nov 4, 2019

Thanks for your review! I modified code, so please check.

@knorth55
Copy link
Member

knorth55 commented Nov 4, 2019

I'm wondering if enum should be int or string.
IMO, string would be easier to understand, but what do you think?

@iory
Copy link
Member Author

iory commented Nov 4, 2019 •
edited
Loading

String is flexible but it's too flexible. Usually, users do not know the all valid options for string as input. So, users should read the documents of each node. If users read the document, users can understand the functions. And enum is too. There are no advantages of string with respect to enum.

Moreover, in ros's dynamics reconfigure, there is an enum function for users like the following image. This is useful for users to select cluster_filter options.
Selection_004

@knorth55
Copy link
Member

knorth55 commented Nov 4, 2019

OK, LGTM.

taichiH
taichiH reviewed Nov 4, 2019
View reviewed changes
Copy link

@taichiH taichiH left a comment

Choose a reason for hiding this comment

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

I added a comment

jsk_pcl_ros/src/euclidean_cluster_extraction_nodelet.cpp Outdated Show resolved Hide resolved
@taichiH taichiH mentioned this pull request Nov 4, 2019
Merged
YutoUchimi
YutoUchimi requested changes Nov 4, 2019
View reviewed changes
doc/jsk_pcl_ros/nodes/euclidean_clustering.md Outdated Show resolved Hide resolved
doc/jsk_pcl_ros/nodes/euclidean_clustering.md Outdated Show resolved Hide resolved
doc/jsk_pcl_ros/nodes/euclidean_clustering.md Outdated Show resolved Hide resolved
jsk_pcl_ros/sample/include/play_rosbag_hsr_multiobjects_in_shelf.xml Outdated Show resolved Hide resolved
jsk_pcl_ros/sample/sample_multi_euclidean_cluster_extraction.launch Outdated Show resolved Hide resolved
jsk_pcl_ros/cfg/EuclideanClustering.cfg Outdated Show resolved Hide resolved
jsk_pcl_ros/sample/sample_multi_euclidean_cluster_extraction.launch Outdated Show resolved Hide resolved
jsk_pcl_ros/src/euclidean_cluster_extraction_nodelet.cpp Outdated Show resolved Hide resolved
doc/jsk_pcl_ros/nodes/euclidean_clustering.md Outdated Show resolved Hide resolved
doc/jsk_pcl_ros/nodes/euclidean_clustering.md Show resolved Hide resolved
@iory
[jsk_pcl_ros/euclidean_clustering] Fixed typo (multi -> ~multi)
da578b2 
[jsk_pcl_ros/euclidean_clustering] Fixed typo (synchornizes -> synchronizes)

[jsk_pcl_ros/euclidean_clustering] Fixed typo (approximate_sync_ -> approximate_sync)

[jsk_pcl_ros/euclidean_clustering] Fixed size of maximum cluster size

[jsk_pcl_ros/euclidean_clustering] Delete duplicated value downsample_enable

[jsk_pcl_ros/euclidean_clustering] Fixed indent

[jsk_pcl_ros/euclidean_clustering/cfg] Fixed indent
@iory
[jsk_pcl_ros/euclidean_clustering] Use capital for arguments
39a6a90
@iory
[jsk_pcl_ros/euclidean_clustering] Rewrite document of `cluster_filte…
c34c7f0 
…r` option

[jsk_pcl_ros/euclidean_clustering] Modified document of ~multi option
@iory
[jsk_pcl_ros/euclidean_clustering] Update install data for data compr…
024e022 
…ession

[jsk_pcl_ros/euclidean_clustering] Update sample bag file player for data compression
@iory
Copy link
Member Author

iory commented Nov 4, 2019

@YutoUchimi
Thanks for your review!
I added a commit to fix pointed out problems.
Please check.

@YutoUchimi
Copy link
Contributor

Oh, I forgot. Please add this test to CMakeLists.txt.

@iory
Copy link
Member Author

iory commented Nov 4, 2019

OK. I added.

YutoUchimi
YutoUchimi approved these changes Nov 5, 2019
View reviewed changes
Copy link
Contributor

@YutoUchimi YutoUchimi left a comment

Choose a reason for hiding this comment

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

Great job!

@Affonso-Gui
Copy link
Member

Cool! Looking forward for this one 👍

@Affonso-Gui Affonso-Gui mentioned this pull request Nov 5, 2019
Merged
@iory
Moved bagfile for multi object detection
dbe9239 
Fixed path of play_rosbag xml

[jsk_pcl_ros_utils/install_sample_data.py] Make it multiprocess downloadable
@iory
[jsk_pcl_ros/multi_euclidean_clustering_sample] Fixed parameter clust…
7c94134 
…er_tolerance to tolerance
@taichiH taichiH mentioned this pull request Nov 6, 2019
Merged
@YutoUchimi YutoUchimi added this to the 1.2.11 milestone Nov 18, 2019
@iory iory mentioned this pull request Jun 10, 2020
Closed
@knorth55
Copy link
Member

@k-okada this node is useful for mask-rcnn and ssd.
also, it passed travis test.
can you check and merge this?

@k-okada k-okada merged commit 7822d2f into jsk-ros-pkg:master Sep 3, 2020
@iory iory deleted the multi-euclidean branch April 10, 2022 14:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@knorth55 knorth55 knorth55 approved these changes

@Affonso-Gui Affonso-Gui Affonso-Gui approved these changes

@taichiH taichiH taichiH approved these changes

@YutoUchimi YutoUchimi YutoUchimi approved these changes

Assignees
No one assigned
Labels
enhancement pkg/jsk_pcl_ros PR/MergeOK PR/WaitForMaintainerCheck
Projects
None yet
Milestone
1.2.16
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@iory @knorth55 @YutoUchimi @Affonso-Gui @taichiH @k-okada