ros-perception / opencv_apps

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

-1 for subscribing camera_info which is unnecessary in actual #55

Closed wkentaro closed 7 years ago

wkentaro commented 7 years ago

I'm not sure why some nodes in opencv_apps does subscribe camera_info even though it is unnecessary.

IMO, camera_info should not be subscribed as much as possible, because in most case it has higher frame rate compared to the images, and it causes the difficulty of synchronization.

For example:

AddingImages

It subscribes camera_info by default with use_camera_info:=true: https://github.com/ros-perception/opencv_apps/blob/indigo/src/nodelet/adding_images_nodelet.cpp#L116

And frame_id is passed to the do_work function: https://github.com/ros-perception/opencv_apps/blob/indigo/src/nodelet/adding_images_nodelet.cpp#L103

But the input_frame_from_msg is not used in actual: https://github.com/ros-perception/opencv_apps/blob/indigo/src/nodelet/adding_images_nodelet.cpp#L164

Even if we set the input_frame_from_msg to output msg, I wonder there are any cases where the output frame_id is different from both two image inputs. Cases like below are assumed by default?

k-okada commented 7 years ago

It is just a templates, to avoid discussion for each node, for same reason we basically have node if that is a part of opencv example.

Any problem with use_camera_info:=false? If you get used to other opencv_apps nodes, then you can easily imagine that the node has this option.

Or if you think default value of use_camera_info should be false, please send PR. https://github.com/ros-perception/opencv_apps/blob/indigo/cfg/AddingImages.cfg#L9

2017年3月20日(月) 20:19 Kentaro Wada notifications@github.com:

I'm not sure why some nodes in opencv_apps does subscribe camera_info even though it is unnecessary.

For example: AddingImages

It subscribes camera_info by default with use_camera_info:=true:

https://github.com/ros-perception/opencv_apps/blob/indigo/src/nodelet/adding_images_nodelet.cpp#L116

And frame_id is passed to the do_work function:

https://github.com/ros-perception/opencv_apps/blob/indigo/src/nodelet/adding_images_nodelet.cpp#L103

But the input_frame_from_msg is not used in actual:

https://github.com/ros-perception/opencv_apps/blob/indigo/src/nodelet/adding_images_nodelet.cpp#L164

I wonder there are any cases where the output frame_id is different from both two image inputs. Cases like below are assumed by default?

  • camera/rgb/camera_info, fraem_id=camera_rgb_optical_frame
  • image_publisher/output1, frame_id=dummy_camera_1
  • image_publisher/output2, frame_id=dummy_camera_2
  • adding_images/image camera_info:=camera/rgb/camera_info image1:=image_publisher/output1 image2:=image_publisher/output2, fraem_id=camera_rgb_optical_frame

— You are receiving this because you are subscribed to this thread. Reply to this email directly, view it on GitHub https://github.com/ros-perception/opencv_apps/issues/55, or mute the thread https://github.com/notifications/unsubscribe-auth/AAeG3L_5e_aDMBx1FPt46-JciDLNt9cEks5rnmB-gaJpZM4MiS6Y .

--

◉ Kei Okada

wkentaro commented 7 years ago

It is just a templates, to avoid discussion for each node, for same reason we basically have node if that is a part of opencv example.

I mean the template is not good, and wonder what is the merit of the templating.

Any problem with use_camera_info:=false? If you get used to other opencv_apps nodes, then you can easily imagine that the node has this option.

The package opencv_apps is relatively new, so I mean the design should be considered properly, seeing prior works. Of course, the use_camera_info:=false works, but I dislike to do things that is unnecessary. That is why I'm not willing to use and send PR to this package, before the re-designing.

wkentaro commented 7 years ago

Fixed via https://github.com/ros-perception/opencv_apps/pull/58

hh129sss5 commented 2 years ago

Open access on this approved M overview approved it

hh129sss5 commented 2 years ago

Open access on this approved M overview approved it

Granted