-
Notifications
You must be signed in to change notification settings - Fork 45
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
feat(intrinsic_camera_calibrator): multiple camera models & ceres integration #208
Conversation
…ls & ceres integration Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
…ation settings Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
Codecov ReportAttention: Patch coverage is
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
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. |
…calibration for Ceres Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
… Ceres Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
...rinsic_camera_calibrator/include/ceres_intrinsic_camera_calibrator/coefficients_residual.hpp
Outdated
Show resolved
Hide resolved
...rinsic_camera_calibrator/include/ceres_intrinsic_camera_calibrator/coefficients_residual.hpp
Outdated
Show resolved
Hide resolved
Signed-off-by: amadeuszsz <[email protected]>
...rinsic_camera_calibrator/include/ceres_intrinsic_camera_calibrator/coefficients_residual.hpp
Outdated
Show resolved
Hide resolved
...ra_calibrator/ceres_intrinsic_camera_calibrator/src/ceres_intrinsic_camera_calibrator_py.cpp
Outdated
Show resolved
Hide resolved
...intrinsic_camera_calibrator/intrinsic_camera_calibrator/camera_models/opencv_camera_model.py
Show resolved
Hide resolved
.../intrinsic_camera_calibrator/intrinsic_camera_calibrator/camera_models/ceres_camera_model.py
Outdated
Show resolved
Hide resolved
@amadeuszsz |
...insic_camera_calibrator/intrinsic_camera_calibrator/intrinsic_camera_calibrator/parameter.py
Outdated
Show resolved
Hide resolved
@amadeuszsz 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 |
...ator/intrinsic_camera_calibrator/intrinsic_camera_calibrator/calibrators/ceres_calibrator.py
Outdated
Show resolved
Hide resolved
Signed-off-by: amadeuszsz <[email protected]>
.../intrinsic_camera_calibrator/intrinsic_camera_calibrator/board_detections/board_detection.py
Show resolved
Hide resolved
Signed-off-by: amadeuszsz <[email protected]>
.../intrinsic_camera_calibrator/intrinsic_camera_calibrator/board_detections/board_detection.py
Show resolved
Hide resolved
Signed-off-by: amadeuszsz <[email protected]>
...ator/intrinsic_camera_calibrator/intrinsic_camera_calibrator/calibrators/ceres_calibrator.py
Show resolved
Hide resolved
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) |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Addressed in 73eb77b
...mera_calibrator/intrinsic_camera_calibrator/intrinsic_camera_calibrator/camera_calibrator.py
Outdated
Show resolved
Hide resolved
.../intrinsic_camera_calibrator/intrinsic_camera_calibrator/camera_models/ceres_camera_model.py
Outdated
Show resolved
Hide resolved
@amadeuszsz |
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
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.
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 |
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? |
There was a problem hiding this 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 🙏
Signed-off-by: amadeuszsz <[email protected]>
Signed-off-by: amadeuszsz <[email protected]>
…logging Signed-off-by: amadeuszsz <[email protected]>
…tion samples for Ceres calibrator Signed-off-by: amadeuszsz <[email protected]>
Addressed in 8d990f0
Addressed in ef56973
Addressed in 4e3a48d |
Signed-off-by: amadeuszsz <[email protected]>
@amadeuszsz regarding logging, only |
Could you show me number of post-rejection inliers? The value of
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 |
The error with If you can also set Note: I was getting very bad detection results for the chess board, but it seems it is unrelated to the PR |
Signed-off-by: amadeuszsz <[email protected]>
Done |
There was a problem hiding this 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
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.
Post-review checklist for the PR author
The PR author must check the checkboxes below before merging.
After all checkboxes are checked, anyone who has write access can merge the PR.