ros / common_msgs

Commonly used messages in ROS. Includes messages for actions (actionlib_msgs), diagnostics (diagnostic_msgs), geometric primitives (geometry_msgs), robot navigation (nav_msgs), and common sensors (sensor_msgs), such as laser range finders, cameras, point clouds.
http://wiki.ros.org/common_msgs
177 stars 191 forks source link

[sensor_msgs] add PanoramaInfo.msg and update CMakeLists.txt #171

Open sktometometo opened 3 years ago

sktometometo commented 3 years ago

Add PanoramaInfo.msg to represent meta-information about panoramic images.

We are using panoramic images generated from fisher eye images.

And we are defining a meta-information message of these panoramic images. https://github.com/jsk-ros-pkg/jsk_recognition/pull/2579 This meta-information is necessary if calculating geometric information from panoramic images. And I think this is useful for other users of panorama images.

PanoramaInfo.msg contains information below

CC: @k-okada

peci1 commented 2 years ago

Hi, we are also using panoramic images in our projects. However, we fuse them from 4-6 90° FOV cameras, so our perspective is a bit different.

What I think could be fruitful is predefining an enum of possible projections (maybe take inspiration in Hugin?), with a field that could hold any custom info if e.g. PROJECTION_CUSTOM is specified. The field could hold either just the name of the custom projection, or parameters of a standardized one (some projections have parameters, e.g. Panini general). This could be a string field with a suggested way to parse the contents. Or a vector of doubles hoping that that is enough to capture all possible panorama projection params.

Also, the message definition should explicitly specify what should a 360° pano have in theta_min/theta_max. I assume it is -pi and +pi, but it would be good to explicitly say that in a comment (assuming the projection center is in the middle of the image).

Last, there is also the question whether a LUT (lookup table) should not be a part of this message (or an accompanying meta info message). Are you sure that given just the image, angular bounds and name of the projection you are able to construct the unprojection function?

Maybe @Martin-Oehler could be interested in this discussion as he is the author of https://github.com/tu-darmstadt-ros-pkg/image_projection .

Martin-Oehler commented 2 years ago

Hello everyone, @peci1 thanks for pointing me to this issue. I thought about creating a message for projections as well, so thank you @sktometometo for initiating this PR.

However, I share the sentiment of @peci1, that the message is too specific. Most projections have some kind of parameters. Even if you limit "Panorama projections" to cylindrical projections, you would also need the radius.

What I think could be fruitful is predefining an enum of possible projections (maybe take inspiration in Hugin?), with a field that could hold any custom info if e.g. PROJECTION_CUSTOM is specified.

I'm not sure, if this would be helpful. This makes it necessary to edit the message if you want to add a new projection type and the message can no longer be used for custom applications/projections. I think, we can take inspiration from the similar sensor_msgs/CameraInfo where the distortion model is just a string and parameters are a vector of doubles.

Also, the message definition should explicitly specify what should a 360° pano have in theta_min/theta_max. I assume it is -pi and +pi, but it would be good to explicitly say that in a comment (assuming the projection center is in the middle of the image).

Yes, agreed.

Last, there is also the question whether a LUT (lookup table) should not be a part of this message (or an accompanying meta info message).

Assuming, that the projection has been created by some kind of analytical function, I don't think, this is necessary. But there should definitely be a way to add additional parameters.

Furthermore, the current definition does not include image dimensions. Similar to CameraInfo, I think this is important information. Next, I would also add a pose to specify the projection center. Since projections can be created on-demand, the projection center does not necessarily coincide with an existing camera frame.

Lastly, I would like to raise the question, if a specific PanoramaInfo message is even necessary. Considering the message fields, it would be very similar to a potential more general ProjectionInfo that could also represent other projections like stereographic.

peci1 commented 2 years ago

What I think could be fruitful is predefining an enum of possible projections (maybe take inspiration in Hugin?), with a field that could hold any custom info if e.g. PROJECTION_CUSTOM is specified.

I'm not sure, if this would be helpful. This makes it necessary to edit the message if you want to add a new projection type and the message can no longer be used for custom applications/projections.

What I meant by enum was actually the .msg "enum", which is just a bunch of constants with shared prefix. It should probably be a string field with a few predefined constants that can provide its value. The idea was to have a unified set of basic projections where the name that is in the projection type parameter would be unambiguous. The way it is currently done in CameraInfo is weird, as it is a string field, but to find the possible basic values, you have to look in the C++ header file somewhere else in the project. An example could look like this:

string PROJECTION_STEREOGRAPHIC="stereographic"
string PROJECTION_CYLINDRICAL="cylindrical"
# ... more

string projection_type  # either one of the predefined PROJECTION_* constants, or a custom projection type

Last, there is also the question whether a LUT (lookup table) should not be a part of this message (or an accompanying meta info message).

Assuming, that the projection has been created by some kind of analytical function, I don't think, this is necessary. But there should definitely be a way to add additional parameters.

You're right that if you have the function definition, you can quite easily build the LUT. The idea is that to improve "generality" of bagfiles containing these images, if there were a LUT recorded, the user replaying the bag would not need to care about the particular projection function and would easily get a correctly projected panorama even for custom functions. I agree that a LUT can be quite large, so it would either deserve its own message type (and could be published as a latched topic alongside the panorama image), or it could be "hijacked" into an Image message (I'm not sure if any of the supported image representations would be enough for storing a LUT, but it might be). However, this hijacking would be against ROS principles.

Furthermore, the current definition does not include image dimensions. Similar to CameraInfo, I think this is important information. Next, I would also add a pose to specify the projection center. Since projections can be created on-demand, the projection center does not necessarily coincide with an existing camera frame.

Agree.

Lastly, I would like to raise the question, if a specific PanoramaInfo message is even necessary. Considering the message fields, it would be very similar to a potential more general ProjectionInfo that could also represent other projections like stereographic.

Sounds interesting. Would you be able to sketch a definition of such message?

Martin-Oehler commented 2 years ago

What I meant by enum was actually the .msg "enum", which is just a bunch of constants with shared prefix. It should probably be a string field with a few predefined constants that can provide its value.

Yes, I guess that approach makes sense. I don't like having the constants in a separate cpp either.

You're right that if you have the function definition, you can quite easily build the LUT. The idea is that to improve "generality" of bagfiles containing these images, if there were a LUT recorded, the user replaying the bag would not need to care about the particular projection function and would easily get a correctly projected panorama even for custom functions.

That's an interesting idea, but should probably be a separate message. You could use sensor_msgs::Image with TYPE_64FC2, but I agree that it would be cleaner to create a separate message type for that.

Sounds interesting. Would you be able to sketch a definition of such message?

Well, I guess it would be pretty similar to this PR, something like:

ProjectionInfo.msg
----
Header header 

uint32 height
uint32 width

geometry_msgs/PoseStamped projection_center_pose 

string projection_model
float64[] projection_parameters

float64 theta_min
float64 theta_max
float64 phi_min
float64 phi_max
peci1 commented 2 years ago

geometry_msgs/PoseStamped projection_center_pose

  1. Does it make sense to have Stamped here?
  2. Should the interpretation be "offset of the 3D projection center from header.frame_id"? Or something else?