Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(intrinsic_camera_calibrator): multiple camera models & ceres integration #208

Conversation

amadeuszsz
Copy link
Contributor

@amadeuszsz amadeuszsz commented Nov 6, 2024

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.

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.

@amadeuszsz amadeuszsz self-assigned this Nov 6, 2024
@codecov-commenter
Copy link

codecov-commenter commented Nov 11, 2024

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.

@amadeuszsz amadeuszsz marked this pull request as ready for review November 13, 2024 06:04
@amadeuszsz amadeuszsz requested a review from knzo25 November 13, 2024 06:04
@amadeuszsz amadeuszsz added the enhancement New feature or request label Nov 13, 2024
@knzo25
Copy link
Collaborator

knzo25 commented Nov 18, 2024

@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
Copy link
Collaborator

knzo25 commented Nov 18, 2024

@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

height = detections[0].get_image_height()
width = detections[0].get_image_width()

camera_model = CameraModel()
camera_model = make_camera_model(camera_model_type=CameraModelEnum.OPENCV)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Addressed in 73eb77b

@knzo25
Copy link
Collaborator

knzo25 commented Nov 20, 2024

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

knzo25 commented Nov 25, 2024

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?

Copy link
Collaborator

@knzo25 knzo25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@amadeuszsz
Copy link
Contributor Author

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

@amadeuszsz amadeuszsz requested a review from knzo25 November 25, 2024 07:47
@knzo25
Copy link
Collaborator

knzo25 commented Nov 25, 2024

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

knzo25 commented Nov 26, 2024

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
Copy link
Contributor Author

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

Copy link
Collaborator

@knzo25 knzo25 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@amadeuszsz amadeuszsz merged commit 1621154 into tier4:tier4/universe Nov 26, 2024
6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants