sdf/camera.sdf: fields for projection matrix

[ihasdapie]

🎉 New feature

Summary

This PR adds sdf fields to allow for configuration of the camera projection matrix.

Related PR in ign-sensors3: gazebosim/gz-sensors#249

Test it

Make a camera with sdf tags specifying these new fields.

Checklist

• Signed all commits for DCO
• Added tests
• Added example and/or tutorial
• Updated documentation (as needed)
• Updated migration guide (as needed)
• Consider updating Python bindings (if the library has them)
• codecheck passed (See contributing)
• All tests passed (See test coverage)
• While waiting for a review on your PR, please help review another open pull request to support the maintainers

[chapulina]

Thank you for the PR! The C++ classes in SDFormat should reflect the XML spec. Do you mind adding the new elements here, with their respective tests?

sdformat/sdf/1.9/camera.sdf

Lines 175 to 193 in 3dd02cc

	<element name="intrinsics" required="0">
	<description>Camera intrinsic parameters for setting a custom perspective projection matrix (cannot be used with WideAngleCamera since this class uses image stitching from 6 different cameras for achieving a wide field of view). The focal lengths can be computed using focal_length_in_pixels = (image_width_in_pixels * 0.5) / tan(field_of_view_in_degrees * 0.5 * PI/180)</description>
	<element name="fx" type="double" default="277" required="1">
	<description>X focal length (in pixels, overrides horizontal_fov)</description>
	</element>
	<element name="fy" type="double" default="277" required="1">
	<description>Y focal length (in pixels, overrides horizontal_fov)</description>
	</element>
	<element name="cx" type="double" default="160" required="1">
	<description>X principal point (in pixels)</description>
	</element>
	<element name="cy" type="double" default="120" required="1">
	<description>Y principal point (in pixels)</description>
	</element>
	<element name="s" type="double" default="0.0" required="1">
	<description>XY axis skew</description>
	</element>
	</element> <!-- End Intrinsics -->
	</element> <!-- End Lens -->

Also, note that we use CamelCase for function names, so the function names should be updated not to use _p. Maybe SetLensProjection*?

[ihasdapie]

Thanks for the feedback! I'll make those changes

[ahcorde]

Do you mind to add tests for the new methods ?

[chapulina]

Thank you for adding the elements! I'm wondering if it's best to put these inside a new <projection> element that's a sibling of intrinsics. Or maybe a child of intrinsics? I'd have to refresh my computer vision knowledge.

[scpeters]

yeah, I don't know enough about these parameters to have a useful opinion

[ihasdapie]

Thank you for adding the elements! I'm wondering if it's best to put these inside a new <projection> element that's a sibling of intrinsics. Or maybe a child of intrinsics? I'd have to refresh my computer vision knowledge.

IIUC the projection matrix is the cumulative effect of intrinsic and extrinsic parameters. So unless a xml element can have two parents (which I don't think is possible) I think @chapulina's suggestion to put these inside a new sibling <projection> element is best. I've done this in de4e536

References

• https://courses.cs.washington.edu/courses/cse576/17sp/notes/CamerasStereo17.pdf
• https://en.wikipedia.org/wiki/Camera_matrix

[ihasdapie]

Do you mind to add tests for the new methods ?

Yup, I missed it earlier. They've been added in a26db0e

[codecov]

Codecov Report

Merging #1088 (896f028) into sdf9 (2e891c2) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##             sdf9    #1088      +/-   ##
==========================================
+ Coverage   87.75%   87.81%   +0.05%     
==========================================
  Files          64       64              
  Lines       10055    10099      +44     
==========================================
+ Hits         8824     8868      +44     
  Misses       1231     1231              

Impacted Files	Coverage Δ
src/Camera.cc	83.13% <100.00%> (+1.96%)	⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

[scpeters]

Do you mind to add tests for the new methods ?

Yup, I missed it earlier. They've been added in a26db0e

to improve test coverage, let's add these new fields to the sensors.sdf test file and add corresponding test expectations to link_dom.cc

• https://github.com/ihasdapie/sdformat/blob/brianc/sdf9/configure_proj_mat/test/sdf/sensors.sdf#L23-L78
• https://github.com/ihasdapie/sdformat/blob/brianc/sdf9/configure_proj_mat/test/integration/link_dom.cc#L261-L304

[ihasdapie]

to improve test coverage, let's add these new fields to the sensors.sdf test file and add corresponding test expectations to link_dom.cc

@scpeters I've done so in 47bb73a.

C++ classes now match the spec

[scpeters]

@ahcorde @chapulina you had requested changes, but I believe they have been addressed, so I dismissed your reviews. I'll merge on Monday unless I hear otherwise