-
Notifications
You must be signed in to change notification settings - Fork 200
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
Update leg_detector to OpenCV3 #53
Conversation
[face_detector][test common] Fix warning in .launch format. Fix namespace in testcases. Increase retry. .bag-based perceptional tests are hard to deterministic, so it makes sense to do this. Set param use_sim_time. [face_detector][test] hztest can be used for measureing no publishment. hzerror is not needed in that case. [face_detector][test] Specify test name. Pass all args. [face_detector][test] pass_all_args might require all args kept out of include tag. [face_detector][test] Specify test-name. [face_detector][test] Disable all tests other than rgbd. I couldn't figure out how to use launches other than face_detector.rgbdlaunch, after hours of trial on local machine. I leave that up to someone else. [face_detector][test] Detection using rgbd works with this config. [face_detector][test] Shorter test duration (bag files are less than 7 seconds). [face_detector][test] Shorter test_duration. We only need a second or so as long as we can confirm the designated topic is being published. [face_detector][test] rosbag play with clock and loop option.
Since the publish rate of the detected face varies every second because one of the persons in the bag file is moving, [publishtest](http://wiki.ros.org/rostest/Nodes#publishtest) should suit the best in this case. [face_detector][test common] Enable substitution in rosparam tag.
The cv::ml package has been rewritten. I just changed the code to accomodate the new api, maybe this does not conform to the way the new cv::ml should be used...
In opencv3 the predict_prob method was removed from cv::ml::RTrees.
The dependency on the people_msgs package should be enough. This removes warnings for the leg_detector and people_tracking_filter when building with catkin_make_isolated and catkin build.
This reverts commit 0bda254.
Isaac Saito has been working hard to make the Travis CI work with Kinetic. Removing his email from ravis.yml, to save his inbox from errors due to other issues.
Version changed to 3.2.0, unfortunately we need to hardcode the location for now.
Hi! Is there a plan for reviewing this PR? If not, is there any way I can help? e.g. code review, comparing performance on real data between this & 14.04, etc. |
Bump, any plan on moving this forward? |
So, in case it helps on deciding, I gave it a try to run it on the Both are running with the same laser topic, a real Hokuyo UTM-30LX-01 (from a PR2). Find a video of their behaviour in Rviz here: https://youtu.be/G-OxB-L14z0 Find also attached to this comment a rosbag with all the topics:
leg_detector_indigo_vs_kinetic_2018-04-20-19-05-57.bag.tar.gz I see even an improvement in kinetic. But generally, they do quite the same job. This was running (after checking out the repo and doing
With the only modification of changing the I'd like to have this package back in the ROS repos as it's very good to enable simple demos (like this video) to get people started with ROS and stuff to do with people perception. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Overall LGTM, just a few small comments.
@@ -82,15 +82,15 @@ if(CATKIN_ENABLE_TESTING) | |||
DESTINATION ${CATKIN_DEVEL_PREFIX}/${CATKIN_PACKAGE_SHARE_DESTINATION}/test | |||
MD5 59126117e049e69d577b7ee27251a6f8) | |||
|
|||
add_rostest(test/wide-stereo_true_rtest.xml) | |||
add_rostest(test/wide-stereo_false_rtest.xml) | |||
# add_rostest(test/wide-stereo_true_rtest.xml) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd prefer just deleting these instead of commenting them out, here and below
@@ -244,7 +244,8 @@ class LegDetector | |||
|
|||
int mask_count_; | |||
|
|||
cv::ml::RTrees forest; | |||
// cv::ml::RTrees forest; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: remove
I don't really have things setup to try this out now, so unless there are any objections from @DLu I'm going to go ahead and merge. I'll check back in a few days. |
@dlaz a little friendly bump in case anyone wants to take a final look or you want to merge it. |
I have updated the leg_detector to the new structure of OpenCV3. I actually had another PR #48 open but it failed in Travis. Isaac Saito fixed this in his PR #49 which I rebased this code onto, and now it passes on Travis!
Isaac also found that we need to hardcode the location of haarcascade_frontal_face_alt from OpenCV3 (see #52). Between the time he made #49 and now, OpenCV3 was updated to 3.2.0, so I went and updated the filepath.
This is all to say that, merging this branch also includes all the hard work of Isaac in PR #49.