Hi Kevin,

I don't quite understand your feedback. I think you may have interpreted
the REP in a way very different from what I intended.

On Tue, Dec 6, 2011 at 6:07 PM, Kevin Walchko <kevin.walchko@gmail.com>wrote:

> -1
>
> It is a little hard to understand this trade due to lack of info, but I
> will justify my answer:
>
> - There is a major re-write of code (both core ROS and community provided
> code) that could take significant time and effort to perform. It would be
> useful if an estimate of this impact could be given (even a WAG). However,
> maybe I am wrong.
>

The proposed depth image representation is *exactly* the one currently used
by the ROS OpenNI driver. The intent of this REP is to codify existing
practice. What code would need to be rewritten?

- There appear to be no crucial information included in the new
> DisparityImage to really justify the change and the effort above. At least
> there is no clear, compelling argument in the REP.
>

sensor_msgs/DisparityImage is not new - it's been around since pre-Box
Turtle. Using sensor_msgs/Image to represent dense depth data is a
comparatively new practice, and the subject of the REP.

- I dislike the justification being because one vendor has set a precedent.
> What happens when the kinect 2 comes out with a different format, will we
> adapt that format as the next default? What happens if OpenNI goes away and
> Nintendo (just picked someone at random) invents a really cool depth sensor
> that works different? I think our depth image should be based on what is
> right for us and not what one vender provides today. Just remember, the
> Kinect is only a little over one year old. What will next year this time
> bring?
>
> I did like the idea of uint16 taking less bandwidth and potentially
> running on simpler hardware (ARM, netbook, etc). However, I find none of
> the arguments compelling.
>

The canonical format - following standard ROS practices - is to use floats,
as described in "Canonical Representation." Depth image consumers need only
support the float format.

So you are proposing:
> DisparityImage of type uint16
> - focal length, baseline, principal point
> - valid_window (seems unnecessary)
> - min/max disparity
> - delta_d
>

Not at all. I am arguing against DisparityImage, for the reasons described
in "Why Not sensor_msgs/DisparityImage."

I assume we would get rid of the need for CameraInfo when used with
> DisparityImage message if this is done.
>

Well, this is one of the issues with the old DisparityImage message. It
included focal length and baseline to allow converting disparities to
depths, but to properly compute a point cloud you need the rest of the
CameraInfo parameters anyway.

What is the impact of instead publishing a separate topic to fill in these
> couple of missing metadata pieces? This changes things from (Image,
> CameraInfo) pair to (Image, CameraInfo, ??) triplet … not sure that is
> super great, but the impact may be much less.
>

I think we're in agreement here - I'm in favor of a separate topic with the
missing metadata, should we eventually find it necessary.

Also, not to be too critical, but I had a problem following the logic of
> this entire REP. I am sure it makes complete sense to the author, however I
> found the logic to jump around.
>

The REP follows the standard structure (
http://www.ros.org/reps/rep-0001.html#what-belongs-in-a-successful-rep). If
some of my language was confusing, please let me know.

Cheers,
Patrick