API review

Proposer: Blaise Gassend

Present at review:

  • List reviewers

Question / concerns / comments

Enter your thoughts on the API and any questions / concerns you have here. Please sign your name. Anything you want to address in the API review should be marked down here before the start of the meeting.

Blaise

  • Is there any use case in which the target frame is distinct from the image frame?
    • BG: Haven't found one, but isn't hindering anything.
  • Should the node be outputting a new camera_info, or is this just for visualization?
    • BG: Changed output topic to rotated/image so that it will be easy to add camera_info at some point

Brian

  • (minor) I think that image should be a subscribed topic.

    • BG: Fixed

Stu

  • I would prefer target/frame_id, target/x, ...
    • BG: Won't work with dynamic_reconfigure.
  • I think the description of target_frame_id might not be correct
    • BG: Renamed reference_* to source_* after discussion with Stu and Wim.

Patrick

  • Should only subscribe to image if something is subscribed to image_rotated, like what other image_pipeline nodes do.

    • BG: Done.
  • You're using the frame_id in the image header, which is arguably dangerous. Some time ago Vijay, Jeremy and I hashed out some conventions on the exact meanings of frame_id in Image and CameraInfo. One conclusion we had was that image.header.frame_id doesn't have a well-defined meaning and that info.header.frame_id should always be used instead. We talked about having some arbitrary convention for image.header.frame_id but never actually implemented that in the drivers. So, I recommend using a CameraSubscriber and pulling the frame id out of the CameraInfo.

    • BG: I added a use_camera_info option that defaults to true to make your request the default. I also added an input_frame_id option that allows explicit setting of the input frame.
  • I also slightly prefer target/x, etc. But I'm guessing that won't play nice with dynamic_reconfigure.

    • BG: Won't work with dynamic_reconfigure.

Doc suggestions while I'm at it:

  • In the package description state why this is useful, e.g. for viewing images from a camera with arbitrary motion.
    • BG: Done in trunk.
  • Emphasize that image_rotated is intended only for visualization purposes.

    • BG: Done in trunk. I assume you mean here that it would make no sense to use the rotated image for computation, correct?
  • The description of ~output_image_size makes it sound like an enum, so say something about interpolating for fractional values.

    • BG: Done

Meeting agenda

To be filled out by proposer based on comments gathered during API review period

Conclusion

Package status change mark change manifest)

  • /!\ Action items that need to be taken.

  • {X} Major issues that need to be resolved


Wiki: image_rotate/Reviews/2010_04_15_API_Review (last edited 2010-05-18 23:30:31 by BlaiseGassend)