search

ros-perception / opencv_apps

http://wiki.ros.org/opencv_apps
64 stars 70 forks source link

[face_detection_nodelet.cpp] fix: face/eye origin #39

Closed furushchev closed 6 years ago

furushchev commented 7 years ago

In current code, face / eye rectangles origin are each center points. Since face and eyes message is defined as rectangle, these should be the origin of each components.

$ rosmsg show opencv_apps/Face
opencv_apps/Rect face
  float64 x
  float64 y
  float64 width
  float64 height
opencv_apps/Rect[] eyes
  float64 x
  float64 y
  float64 width
  float64 height
furushchev commented 7 years ago

irrelevant error?

cd /home/travis/catkin_ws/build/opencv_apps/test && rosbag reindex vslam_tutorial.bag --output-dir /home/travis/catkin_ws/src/opencv_apps/test
ERROR reading vslam_tutorial.bag: [Errno 2] No such file or directory: 'vslam_tutorial.bag'
k-okada commented 7 years ago

Yes, you're right, but at same time, people usually uses 'center of face' , and do not care about rectangle, so for such people, publishing the 'origin' of rectangle seems error code. How about publich face_centers too? plesase add comment within the code and explain faces is the rectangle and x,y is the left-top (?) of the rectangle. reindex vslam_tutorial.bag is downloaded during compile time, so it is just failed to download, I think

furushchev commented 7 years ago

@k-okada OK, I added face_center and eyes_center to Face message with explanation. Please review.

furushchev commented 7 years ago

rebased origin/indigo (see #45)

furushchev commented 7 years ago

@k-okada kindly ping to a maintainer.

furushchev commented 7 years ago

@k-okada Any comment?

k-okada commented 7 years ago

this is API breaking PR, so I'll merge with other braking PRs

◉ Kei Okada

2016-10-14 21:08 GMT+09:00 Furushchev notifications@github.com:

@k-okada https://github.com/k-okada Any comment?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/ros-perception/opencv_apps/pull/39#issuecomment-253782460, or mute the thread https://github.com/notifications/unsubscribe-auth/AAeG3Pg2d2eBKPIeUgYcit5XaaFLXVZ1ks5qz3CpgaJpZM4KF6CD .

furushchev commented 7 years ago

@k-okada Now I see. Please add a label for that.

furushchev commented 6 years ago

Closed. Now face recognition feature was merged and is used by some packages. I think it's rather better to keep this unchanged.