ChArUco and Aruco board based calibration #2
Conversation
… boards for A4, A3 and A2 formats
- added new param (square_lenght) - corrected unequal vertical and horizontal margin - generated example of boards for A4, A3 and A2 format - redefined some variables names to be more clear
…re elegant way - Updated aruco_parameters.yaml with ChArUco parameters - Enhanced extrinsic_calibrator_class.py to handle different calibration methods dynamically. - Introduced new classes for ArUco parameters to avoid mishmash - Introduced new camera classes which preform calibration. Each class inherits from base camera class which keeps some shared stuff - each of the class has its own calibration procedure and then the process of validating the calibration quality is common for each method
…intrinsic calibration, code cleanups
szknowak
changed the title
There was a problem hiding this comment.
Since we're storing large files, please create .gitattribute file with corresponding definition to avoid lfs in git history
There was a problem hiding this comment.
I've added pre-commit and git lfs
extrinsic_calibrator/package.xml
Outdated
| </export> | ||
| </package> | ||
|
|
||
| </package> |
There was a problem hiding this comment.
Let's add parameters validation. https://github.com/PickNikRobotics/generate_parameter_library?tab=readme-ov-file#built-in-validators
Some examples:
board_modewould benefit fromone_of<>validationmarker_lengthcan't be assigned a negative value- ...
There was a problem hiding this comment.
aruco_dict should be one_of<> as well
There was a problem hiding this comment.
done, thanks for that tip, super cool stuff
There was a problem hiding this comment.
Hmm do we really need it? Can't we just rely on Python classes created automatically by params generator?
There was a problem hiding this comment.
good point - I'll try to use them
| class BaseArucoParams(ABC): | ||
| def __init__(self, node:Node, params): |
There was a problem hiding this comment.
If we speak about implementation details
- Pydantic inheritance is go to
- Conceptually, it'd be better to not pass
Nodeobject reference here. If u need logging, param object should return only string message or json structure which you can properly log in client code (ExtrinsicCalibrator)
There was a problem hiding this comment.
Yup, pydantic would be better.
I've tried to switch to it from ABC inheritance, but I see that it wold require a lot of code refactoring, so maybe I could add this when I would have some free time?
|
|
||
| # iterate through each marker of the marker_transforms dictionary | ||
| for marker_id, transforms in self.marker_transforms.items(): | ||
| if len(transforms) == 30: |
| self.node = node | ||
| self.camera_name = camera_name | ||
| self.camera_id = camera_id | ||
| self.image_topic = "/robotic_platform/" + image_topic |
There was a problem hiding this comment.
I've deleted it - 'robotic_platform' prefix is needed only while I was working on data from simulation.
On bags from the lab its unneeded.
| # Visualize board pose | ||
| cv2.drawFrameAxes(cv_image, self.camera_matrix, self.dist_coeffs, rvec, tvec, self.board.getMarkerLength() * 2) | ||
|
|
||
| if 0 not in self.marker_transforms and 0 not in self.reliable_marker_transforms: |
There was a problem hiding this comment.
Orginal calibrator was estimating poses of many aruco markers and keeping it in self.marker_transforms and self.reliable_marker_transforms dicts.
As in board based calibration only one pose is estimated (boards pose) then only first element of
self.marker_transforms and self.reliable_marker_transforms is populated.
So - 0 here is just id of detected object transform (board). And there always be one element in self.marker_transforms and self.reliable_marker_transforms as long as we use one board.
I'll change it from self.marker_transforms -> self.board_transforms (simple list instead of dict with marker ids).
So this strange logic with 0 would not be needed.
There was a problem hiding this comment.
Unfortunately It is not possible to change this logic with '0' to something more reasonable, so I've changed it to class const.
I've tried to apply the logic I've proposed above but it would require a lot of changes in ExtrinsicCalibrator methods as it operate on self.reliable_marker_transforms which must be a dict with int keys.
|
|
||
| from extrinsic_calibrator_core.src.detectors.aruco_params import ArucoMarkerParams, ArucoBoardParams, ChArUcoBoardParams | ||
|
|
||
| class CameraBase(ABC): |
There was a problem hiding this comment.
I see architectural issues here
- Single responsibility violation - this class handles too many things like camera subscription, marker detection, pose tracking (a sing of such a violation is always multiple constructor arguments, let's say >3 :))
- Separation of concerns (as above)
- Misleading naming when we consider domain - it's more a detector than camera
- As inheritance is often unavoidable we should try to benefit from aggregation as well.
I'd suggest
- I don't see any benefit from moving subscription out of the original node - let's move all Node instantiation thingies back there
- Separate concerns by defining
PoseTracker - Simplify implementation by using
DetectorArucoDetector... class with a single responsibility - The above can operate on ROS-agnostic formats (input already converted with cv2.bridge and return generic type)
There was a problem hiding this comment.
Keep in mind this is a "minimal change" in respect to a third party fork. Some of your suggestions might not be valid here. I would vote for not doing any drastic architectural changes here
There was a problem hiding this comment.
I think it would be worth to implement image_callback in the base class with the swappable parts defined in the child classes. That way we can compare thirdparty Camera class to our CameraBase class.
There was a problem hiding this comment.
Ain't checked implementation - vibe coding I assume ;)
However:
- Let's add docs
- We should story it under
scriptdir or sth
There was a problem hiding this comment.
yup, save_svg fully vibe coded, generate_aruco_board in 90%, with some small corrections.
What I could improve is to automate saving file name to keep naming convention
There was a problem hiding this comment.
Docs added. Generator changed so it keeps file naming convention 🤓
| @@ -0,0 +1,51 @@ | |||
| aruco_params: | |||
There was a problem hiding this comment.
I think this should be an extrinsic_calibrator_params_definition.yaml file that defines parameters for extrinsic_calibrator node. So that you can lode it to the node in the launch file with substitutions
There was a problem hiding this comment.
aruco_dict should be one_of<> as well
| default_value: "DICT_6X6_1000" | ||
| marker_length: | ||
| type: double | ||
| default_value: 0.26 No newline at end of file |
| return True | ||
|
|
||
|
|
||
|
|
| numpy==1.26.4 | ||
| opencv-contrib-python==4.12.0.88 |
There was a problem hiding this comment.
I've faced this issue:
https://stackoverflow.com/questions/76781469/attributeerror-module-cv2-aruco-has-no-attribute-interpolatecornerscharuco
Contrib version is needed to use Board Pose Estimation methods
Yesterday during first tests in the lab during fresh requirments installing it turned out that opencv-contrib collides with numpy<2. But running calibrator with numpy>2 causes some errors, so probably in requirments I'd left:
numpy==2.2.6
opencv-contrib-python==4.12.0.88
and in dockerfile:
pip install "numpy<2"
|
|
||
| from extrinsic_calibrator_core.src.detectors.aruco_params import ArucoMarkerParams, ArucoBoardParams, ChArUcoBoardParams | ||
|
|
||
| class CameraBase(ABC): |
There was a problem hiding this comment.
Keep in mind this is a "minimal change" in respect to a third party fork. Some of your suggestions might not be valid here. I would vote for not doing any drastic architectural changes here
| self.parameters = cv2.aruco.DetectorParameters() | ||
| self.detector = cv2.aruco.ArucoDetector(self.aruco_dict, self.parameters) | ||
|
|
||
| super().log_initialization() |
There was a problem hiding this comment.
super() unnecessary if there is no override
| if 0 not in self.marker_transforms and 0 not in self.reliable_marker_transforms: | ||
| self.marker_transforms[0] = deque(maxlen=30) | ||
|
|
||
| if 0 not in self.reliable_marker_transforms: |
| if 0 not in self.marker_transforms and 0 not in self.reliable_marker_transforms: | ||
| self.marker_transforms[0] = deque(maxlen=30) | ||
|
|
||
| if 0 not in self.reliable_marker_transforms: |
|
|
||
| from extrinsic_calibrator_core.src.detectors.aruco_params import ArucoMarkerParams, ArucoBoardParams, ChArUcoBoardParams | ||
|
|
||
| class CameraBase(ABC): |
There was a problem hiding this comment.
I think it would be worth to implement image_callback in the base class with the swappable parts defined in the child classes. That way we can compare thirdparty Camera class to our CameraBase class.
| return position_range, rotation_range | ||
|
|
||
| def caluclate_marker_pose_variation(self, position_range : np.ndarray, rotation_range : float) -> float: | ||
| pose_variation = np.mean(position_range) + np.degrees(rotation_range) |
There was a problem hiding this comment.
I don't like the metric. It's a simple mean of ptp's instead of some root square sum. The rotation part is in degrees which is not comparable to meters which skews the whole metric.
I would recommend treating everything as vectors in a 6D space and combining it with vector norm. And reporting all three: position_error, rotation_error and combined_error all in meters.
ChatGPT and Gemini agree with me:
https://chatgpt.com/s/dr_690cbfe778b88191849f010d43b8363f
There was a problem hiding this comment.
very good point, I'll redefine metric calucations
There was a problem hiding this comment.
def calculate_error(self, positions: np.ndarray) -> float:
mean_position = np.mean(positions, axis=0)
normalized_positions = positions - mean_position
position_variation = np.linalg.norm(normalized_positions)
return position_variationI'll utilize it like this ^
- normalize by the mean
- vector norm
position error will be in meters
rotation in degrees
…docs for generator script
…y params enerator
self.reliable_object_transforms[object_id] = transforms[-1]prevoius solution (from forked repo) was using last collected transform as a reliable one (used for calibration). While we have nice logic for checking if all transforms are precise maybe it would be better to use mean value of all transforms, not the last element? |
ChArUco and Aruco board based calibration
extrinsic_calibrator_core:
Aruco parameters
To handle 3 different board types and avoid mishmash (as boards have different params) the structure of
camera_topics_parameters_schema.yamlwas followed.detectors/camera.pywas refactored to follow this standard.Detectors
To handle 3 different board types and avoid mishmash all board-type functionalities were moved from
extrinsic_calibrator_class.pytodetectors/camera.pyIn
extrinsic_calibrator_class.py:In
detectors/camera.py:CameraBaseclass which includes all common functionalities between camera_detectorsCameraAruco,CameraArucoBoard,CameraChArUcoBoard) which inherits fromCameraBaseandoverwrites
image_callbackwhere board or marker pose estimation is conducted.CameraAruco.image_callbackstays the same as forked code and second parts of eachimage_callbackis also the same(do not review :D)
generate_board.py:
Doc with instructions:
https://docs.google.com/document/d/1UvQvqR0VwnnRna9qVXk13PwAKk63E7QZ-09zgk_v6oU/edit?usp=sharing
Feel free to left some comments in the doc if something is unclear