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]

@amadeuszsz
Confirmed basic functionalities, but got enough comments that I feel I would like to see them addressed or dicussed before further reviews

[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
The most important / unique data is for regularization, which isolates unstable coefficients issue. It was attached in this ticket INTERNAL LINK
Also updated link for all the data used in experiments in this ticket INTERNAL LINK

[knzo25]

@amadeuszsz
Regarding my previous comment:

@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

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

[knzo25]

Is this assumption valid? (I do not remember implementing shuffling 🤔 )

[amadeuszsz]

For initial calibration I assume it is better to take uniform distributed (from collecting sequence point of view) samples, e.g. If we have 80 samples and 40 samples limit for initial calibration, we take 1, 3, 5... instead of 1-40. We could take samples base on pose distribution as well, but I didn't find it necessary.
In general, I would prefer to remove this limit in favor of timeout parameter, but OpenCV API does not consider this. Separate thread for init calibration task would not work - breaking it will not result with recent intrinsic parameters. Maybe calibrating frame by frame here is an answer, but I doubt the performance.

[knzo25]

Even if we do not apply the full behavior regarding the 3d statistics that require a non-single shot calibration, the following should still be implemented in this PR:

Have:
get_reprojection_errors: uses an existing model
and
get_single_shot_reprojection_errors: forces calibration using a single sample. It still requires an input model to know which kind of model it should use (parameters ,etc)

Alternatively, use only get_reprojection_errors and implement the required behavior in the caller.

Currently, there are two kind of uses for self._cached_camera_model. Maybe the should be split.

[knzo25]

In this method, what happens if:
self._cached_camera_model is not None and model != self._cached_camera_model ?

[amadeuszsz]

get_reprojection_errors: uses an existing model

Using existing model seems working here after changes in camera model (using last camera matrix & coefficients), but after triggering calibration we obtain bad results. There are some assumptions in _calibrate within calibrator and mentioned change in camera model is breaking change. I would need to reconsider design here again.

Instead of refactor twice, I would prefer to address this issue with this at once in separate PR if it is not a problem.

In this method, what happens if:
self._cached_camera_model is not None and model != self._cached_camera_model ?

Model has been changed, so we want to update our reprojection error. Isn't it expected behavior? We can change it.

[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]

@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