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

new face_detection package with dlib library #36

Closed
wants to merge 7 commits into from

Conversation

Lizca
Copy link

@Lizca Lizca commented Feb 24, 2016

No description provided.

)

install(DIRECTORY include
DESTINATION ${CATKIN_GLOBAL_INCLUDE_DESTINATION}
Copy link
Member

Choose a reason for hiding this comment

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

CATKIN_PACKAGE_INCLUDE_DESTINATION

@floweisshardt
Copy link
Member

@Lizca: Please fix @ipa-fxm's comments and check if it is building on travis

set_target_properties(data_file PROPERTIES LINKER_LANGUAGE CXX)
target_link_libraries(data_file ${BZIP2_LIBRARIES})

add_executable(dlib_face_detection src/dlib_face_detection.cpp include)
Copy link
Member

Choose a reason for hiding this comment

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

remove the include from the add_executable tags...they are not needed

@Lizca
Copy link
Author

Lizca commented Feb 25, 2016

It seems like the build problems are solved...

<name>dlib_lib</name>
<version>0.1.0</version>
<description>
This package wrapes the external c++ library dlib (http://dlib.net/) in a ros package, so other packages can use it. It downloads the source code of it and then builds it, corresponding to the webpage of it, using cmake. The build library is a static library and will be copied to the created common/lib folder. The neccessary header files will be copied to the common/include folder. The include folder needs to exist befor building the package, because in the CMakeLists you have to specify where the headers are and if the folder doesn't exist, catkin won't be able to build the package. For further descriptions and tutorials see the Makefile.tarball and http://dlib.net/ .
Copy link
Member

Choose a reason for hiding this comment

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

wraps


cloud_sub_ = new message_filters::Subscriber<cloud_msgT>(nh, "pointcloud_in", 10);
direct_face_sub_ = new message_filters::Subscriber<rect_msgT>(nh, "/dlib_face_detection/direct_face_detections", 10);
head_based_face_sub_ = new message_filters::Subscriber<rect_msgT>(nh, "/dlib_face_detection/head_based_face_detections", 10);
Copy link
Member

Choose a reason for hiding this comment

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

avoid global namespaces for pubisher and subscriber

@fmessmer
Copy link
Member

some of my comments apply to several code files, e.g. "global namespaces" or "odom_combined"...did not want to put it everywhere...please have a look wherever it applies...

@ipa-nhg
Copy link
Member

ipa-nhg commented Sep 13, 2016

@ipa-rmb could you please review this PR?

@ipa-rmb
Copy link
Contributor

ipa-rmb commented Sep 14, 2016

Do we use dlib? Do you know whether there is any face detection or recognition function delivered by dlib which is superior to ours? If you do not know, I will check that. If there is no additional benefit or need for dlib, I would not like to add it to the repository as maintenance would be more intricate given this external lib. If we do not merge it to the offical main branch, we can still "save" the dlib integration work in a separate branch.

@floweisshardt
Copy link
Member

we do not use dlib at the moment. I'm not aware of any mayor differences. Saving this in a separate branch is ok for me.
@ipa-rmb: it's your decision

@ipa-rmb
Copy link
Contributor

ipa-rmb commented Sep 16, 2016

Yes, then let's have a separate branch for this. How can I change the target branch of the pull request?

@floweisshardt
Copy link
Member

you can't. We'll have to close this PR, clone the repository from Lizca, push to a branch on your fork and then open a new PR.

Let me know if you need help

@ipa-rmb
Copy link
Contributor

ipa-rmb commented Sep 19, 2016

redirected into a separate archive branch in PR #50 .

@ipa-rmb ipa-rmb closed this Sep 19, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants