feat(intrinsic_camera_calibrator): multiple camera models & ceres integration

amadeuszsz commented 3 weeks ago

Description

Tests performed

Notes for reviewers

Pre-review checklist for the PR author

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

[x] I've confirmed the contribution guidelines.
[x] 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 commented 2 weeks ago

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 :warning:
...r/intrinsic_camera_calibrator/camera_calibrator.py	0.00%	17 Missing :warning:
...alibrator/intrinsic_camera_calibrator/parameter.py	0.00%	7 Missing :warning:
...ra_calibrator/intrinsic_camera_calibrator/utils.py	0.00%	6 Missing :warning:
...librator/src/ceres_camera_intrinsics_optimizer.cpp	0.00%	5 Missing :warning:
...tor/ceres_intrinsic_camera_calibrator/src/main.cpp	0.00%	4 Missing :warning:
...rator/src/ceres_intrinsic_camera_calibrator_py.cpp	0.00%	3 Missing :warning:
...ator/intrinsic_camera_calibrator/data_collector.py	0.00%	2 Missing :warning:

Additional details and impacted files

```diff @@ 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](https://app.codecov.io/gh/tier4/CalibrationTools/pull/208/flags?src=pr&el=flags&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tier4) | Coverage Δ | | |---|---|---| | [differential](https://app.codecov.io/gh/tier4/CalibrationTools/pull/208/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tier4) | `0.27% <0.00%> (?)` | | | [total](https://app.codecov.io/gh/tier4/CalibrationTools/pull/208/flags?src=pr&el=flag&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tier4) | `?` | | Flags with carried forward coverage won't be shown. [Click here](https://docs.codecov.io/docs/carryforward-flags?utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=tier4#carryforward-flags-in-the-pull-request-comment) to find out more.

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

knzo25 commented 1 week ago

@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 commented 1 week ago

@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

amadeuszsz commented 1 week ago

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 commented 1 week ago

@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 commented 1 week ago

@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 :sweat_smile: )

amadeuszsz commented 1 week ago

@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 https://github.com/tier4/CalibrationTools/pull/208/commits/494fcb46736c3c6c5d7062ac420678a76e18eb77

knzo25 commented 5 days ago

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?

amadeuszsz commented 5 days ago

@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 https://github.com/tier4/CalibrationTools/pull/208/commits/8d990f011f2b20de69de4e41d1e76b24c67bf7f2

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

Addressed in https://github.com/tier4/CalibrationTools/pull/208/commits/ef56973de8a4ba26221ed45ec697963445ab4532

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 https://github.com/tier4/CalibrationTools/pull/208/commits/4e3a48d88b47b017c617c1942c46ae8cf2745fd3 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 commented 5 days ago

@amadeuszsz Regarding https://github.com/tier4/CalibrationTools/commit/ef56973de8a4ba26221ed45ec697963445ab4532 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 commented 4 days ago

@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).

Screenshot from 2024-11-25 22-12-55

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 commented 4 days ago

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 commented 4 days ago

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

tier4 / CalibrationTools