feat(intrinsic_camera_calibrator): multiple camera models & ceres integration

[amadeuszsz]

Description

Related links

Tests performed

Notes for reviewers

Pre-review checklist for the PR author

The PR author must check the checkboxes below when creating the PR.

• I've confirmed the contribution guidelines.
• The PR follows the pull request guidelines.

In-review checklist for the PR reviewers

The PR reviewers must check the checkboxes below before approval.

• The PR follows the pull request guidelines.
• The PR has been properly tested.
• The PR has been reviewed by the code owners.

Post-review checklist for the PR author

The PR author must check the checkboxes below before merging.

• There are no open discussions or they are tracked via tickets.
• The PR is ready for merge.

After all checkboxes are checked, anyone who has write access can merge the PR.

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 0% with 104 lines in your changes missing coverage. Please review.

Project coverage is 0.27%. Comparing base (bab5400) to head (774dd90).
Report is 27 commits behind head on tier4/universe.

Files with missing lines	Patch %	Lines
...rinsic_camera_calibrator/coefficients_residual.hpp	0.00%	60 Missing ⚠️
...r/intrinsic_camera_calibrator/camera_calibrator.py	0.00%	17 Missing ⚠️
...alibrator/intrinsic_camera_calibrator/parameter.py	0.00%	7 Missing ⚠️
...ra_calibrator/intrinsic_camera_calibrator/utils.py	0.00%	6 Missing ⚠️
...librator/src/ceres_camera_intrinsics_optimizer.cpp	0.00%	5 Missing ⚠️
...tor/ceres_intrinsic_camera_calibrator/src/main.cpp	0.00%	4 Missing ⚠️
...rator/src/ceres_intrinsic_camera_calibrator_py.cpp	0.00%	3 Missing ⚠️
...ator/intrinsic_camera_calibrator/data_collector.py	0.00%	2 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                @@
##           tier4/universe    #208      +/-   ##
=================================================
- Coverage            0.93%   0.27%   -0.67%     
=================================================
  Files                 270      30     -240     
  Lines               21339    2535   -18804     
  Branches              383     263     -120     
=================================================
- Hits                  200       7     -193     
+ Misses              20982    2528   -18454     
+ Partials              157       0     -157     

Flag	Coverage Δ
differential	0.27% <0.00%> (?)
total	?

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[knzo25]

@amadeuszsz
remember to link the related tickets and data needed to reproduce (so I can use the same data and independent one if possible)

[knzo25]

@amadeuszsz
Regarding:
intrinsic_camera_calibrator/intrinsic_camera_calibrator/intrinsic_camera_calibrator/board_detections/board_detection.py,

The several statistics related to the pose, angles, etc that require a camera model, should use "the best camera model available", which is not the single shot. Single shot works well for these only in the case that the camera is not distorted.

Single shot should be used to detect outliers based on the reprojection error. E.g., if a model can not fit even one sample, it is highly probable that that sample is an outlier

[knzo25]

This seems to be a problem from before, but can you check if all .value accesses are done in a locked context?
Applies to the calibration related methods

[amadeuszsz]

I just checked it and it seems .value for all parameters from GUI are reflected here.

[knzo25]

That is not what I meant.
The parameters are accessed both in the GUI threads and the calibration ones. This raises potential dataraces.
So far I had been making sure (or at least tried) that all accesses to .value were done in a with self.lock: context.

I just checked, and this one seems is not. Can you address these cases?
Alternatively, the lock part of ParameterizedClass could be made to be applied to the Parameter class

[amadeuszsz]

Addressed in 73eb77b

[knzo25]

@amadeuszsz
Regarding how the distortion coefficients are treated inside the ceres calibrator.
Kind of got the impression there would be errors if radial coefficients < 3 and rational cofficients > 0.
Could you check? If seems to much a trouble, we could add a restriction that makes radial coefficients == 3 if we want to use rational coefficients (personally, I would prefer this 😅 )

[amadeuszsz]

@knzo25

Regarding how the distortion coefficients are treated inside the ceres calibrator. Kind of got the impression there would be errors if radial coefficients < 3 and rational cofficients > 0. Could you check? If seems to much a trouble, we could add a restriction that makes radial coefficients == 3 if we want to use rational coefficients (personally, I would prefer this 😅 )

Why? I checked and it works for any combination. Also I can see how reprojection error changes so I believe it is due to camera model configuration.

Regarding:

intrinsic_camera_calibrator/intrinsic_camera_calibrator/intrinsic_camera_calibrator/board_detections/board_detection.py,

The several statistics related to the pose, angles, etc that require a camera model, should use "the best camera model available", which is not the single shot. Single shot works well for these only in the case that the camera is not distorted.

Single shot should be used to detect outliers based on the reprojection error. E.g., if a model can not fit even one sample, it is highly probable that that sample is an outlier

This may be too big for this PR since also affects some of sergio's work. In the meantime, can you turn all data collection filters that use 3D information ? (pose, 3d rotation, etc). As explained before, using single shot models to estimate these values with highly distorted cameras will end up with trashy values

Yup, use recent best model brings me some issues and we would need to redesign some part of the code to assign camera model configuration to board detections. Your temporary workaround is addressed in 494fcb4

[knzo25]

Following occam's razor principle, I would like to leave the default distortion model fixed at radial coefficients=2 with no rational coefficients.

For ceres, the max calibration samples, currently capped at 80, should be increased to a large number, 500 or 1000 for example

Final comment. Not sure it this happened now or before (at least when I first implemented this, it did not happen), but in the current code, the loggings are not really working. Can you make sure that the ones in the calibration routine are flushed (shown in the console) as soon as they are executed?

[knzo25]

There is one comment from before left, plus some minor changes related to parameters and logging 🙏

[amadeuszsz]

@knzo25

Following occam's razor principle, I would like to leave the default distortion model fixed at radial coefficients=2 with no rational coefficients.

Addressed in 8d990f0

For ceres, the max calibration samples, currently capped at 80, should be increased to a large number, 500 or 1000 for example

Addressed in ef56973

Final comment. Not sure it this happened now or before (at least when I first implemented this, it did not happen), but in the current code, the loggings are not really working. Can you make sure that the ones in the calibration routine are flushed (shown in the console) as soon as they are executed?

Addressed in 4e3a48d
Now we use same logging severity as it was set for glog (Ceres c++ bindings). Default is 2 (error) and can be easily controlled via CLI. Please, let me know if you would like to change this default value.

[knzo25]

@amadeuszsz
Regarding ef56973
I mean that number as the default parameter of what goes after the entropy subsampling, In my test even if,say, 200 samples passed the pre-rejection flter, only 80 were used for the final ceres calibration.

regarding logging, only cvUndistortPointsInternal is displayed whereas all the other relevant logging does not appear

[amadeuszsz]

@knzo25

Regarding ef56973 I mean that number as the default parameter of what goes after the entropy subsampling, In my test even if,say, 200 samples passed the pre-rejection flter, only 80 were used for the final ceres calibration.

Could you show me number of post-rejection inliers? The value of 80 was occurring only in addressed commit and after update I can see more calibration samples used for calibration with default parameter (500).

regarding logging, only cvUndistortPointsInternal is displayed whereas all the other relevant logging does not appear

I can see every single log using appropriate severity level, e.g.:

[camera_calibrator-1] INFO:root:Iteration 93: inliers: 123 | mean rms: 0.62 | min rms: 0.07 | max rms: 7.94                                                                                                                                                                        

Did you run launch file with custom severity?

ros2 launch intrinsic_camera_calibrator calibrator.launch.xml severity:=0

[knzo25]

The error with cvUndistortPointsInternal was due to a custom opencv I had installed. Apologies for that one
Regarding the max_calibration_samples, I misunderstood its current meaning (now there are 3 limits), but can you please delete the self.max_calibration_samples.value = 500?

If you can also set <arg name="severity" default="0"/> we can merge this PR!

Note: I was getting very bad detection results for the chess board, but it seems it is unrelated to the PR

[amadeuszsz]

but can you please delete the self.max_calibration_samples.value = 500?

If you can also set <arg name="severity" default="0"/> we can merge this PR!

Done

[knzo25]

LGTM !
Thanks for the cleanest PR I have seen in a while ahaha