-
Notifications
You must be signed in to change notification settings - Fork 64
feat: enable interactive calibrator to specify file intrinsics/extrinsics on runtime #271
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: enable interactive calibrator to specify file intrinsics/extrinsics on runtime #271
Conversation
Gemini ReviewSummaryThis Pull Request (PR) significantly enhances the The core changes involve:
Visual OverviewThe PR introduces new ways to input and output calibration data, moving from potentially ROS-topic-only or internal management to file-based interaction. The following diagram illustrates the high-level data flow changes for calibration parameters. graph TD
subgraph "Old Flow"
A["ROS Topics (CameraInfo, TF)"] --> B[("Interactive Calibrator")]
B --> C["Save to JSON (TF only)"]
end
subgraph "New Flow"
D["ROS Topics (CameraInfo, TF)"] --> E[("Interactive Calibrator")]
F["User Selected Intrinsics File (YAML)"] --> E
G["User Selected Extrinsics File (YAML)"] --> E
E --> H["Display Current Intrinsics/Extrinsics"]
E --> I["Save Pairs and Results (YAML)"]
end
style A fill:#f9f,stroke:#333,stroke-width:2px
style B fill:#bbf,stroke:#333,stroke-width:2px
style C fill:#f9f,stroke:#333,stroke-width:2px
style D fill:#9cf,stroke:#333,stroke-width:2px
style E fill:#bbf,stroke:#333,stroke-width:2px
style F fill:#9cf,stroke:#333,stroke-width:2px
style G fill:#9cf,stroke:#333,stroke-width:2px
style H fill:#f9f,stroke:#333,stroke-width:2px
style I fill:#f9f,stroke:#333,stroke-width:2px
Explanation: The old flow primarily relied on ROS topics and saved only TF data to JSON. The new flow adds the ability to load intrinsics and extrinsics from user-selected YAML files, displays these values in the UI, and saves comprehensive calibration results (including pairs and final transforms) to YAML. Detailed ChangesLogic ChangesThe core logic changes revolve around how calibration parameters are managed and presented in the GUI. The removal of The
The Architecture ImpactThe PR introduces a clearer separation of concerns by moving file I/O and UI-specific logic into classDiagram
class ImageViewRosInterface {
+image_frame: string
+lidar_frame: string
+parent_frame: string
+child_frame: string
+should_reverse_transform: bool
+init(node_name: string)
+image_callback(msg: Image)
+pointcloud_callback(msg: PointCloud2)
+camera_info_callback(msg: CameraInfo)
}
class ImageViewUI {
+message_camera_info: CameraInfo
+optimized_camera_info: CameraInfo
+source_camera_info: CameraInfo
+initial_transform: TransformStamped
+current_transform: TransformStamped
+calibrated_transform: TransformStamped
+init(ros_interface: ImageViewRosInterface)
+setup_ui()
+load_intrinsics_from_file()
+load_extrinsics_from_file()
+save_calibration_results()
}
class InteractiveCalibratorUI {
+init(ros_interface: ImageViewRosInterface)
+float_representer(dumper, value)
+list_representer(dumper, value)
}
ImageViewRosInterface <|-- ImageViewUI : uses
ImageViewUI <|-- InteractiveCalibratorUI : inherits
Explanation: Code Quality EvaluationThe PR demonstrates good practices in several areas:
Potential areas for minor enhancement or consideration:
Does it accomplish what it claims?Yes, the PR successfully accomplishes what it claims.
The changes in the launch files also confirm the new parameterization for What can be enhanced beyond this PR?
Is there any things not considered well in this PR?
|
|
@paulsohn From the review, there are several things I think reasonable:
|
6d32e97 to
4df5595
Compare
|
1: Done 5201af8 . One inconvenience is that if you want to select json, you should select json from the dropdown menu (the default is set to yaml) There are lots of camera distortion model names sharing the same pinhole distortion formula (as previously addressed in #268 ), and I've seen more invalid intrinsics which have diverged focal length (e.g. 10^9 scale) or diverged distortion parameters, from intrinsics calibration actually crashing the view, than those which have camera matrix having 10 entries. Diverged results are very tricky to verify with a universal criteria so I didn't include it, but at least it will crash with log from now on 4df5595 2: Might be too much for a single PR. Could be the next task, but I won't do it here since this PR is already huge. |
Owen-Liuyuxuan
left a comment
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.
@shmpwk @KYabuuchi @yabuta I believe the function is improved well. I am thinking about how to broadcast/formulate the new UI descriptions/new workflows into confluence documentations.
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.
Pull request overview
This PR significantly enhances the interactive camera-lidar calibrator by enabling runtime specification of camera intrinsics and extrinsics from files. The changes improve the calibration workflow by allowing users to load and compare different calibration sources, with real-time visualization updates and standardized YAML output formats.
Key Changes:
- Added file loading capabilities for both camera intrinsics and extrinsics with support for multiple formats (YAML/JSON, rpy/quaternion)
- Introduced new frame parameters (image_frame, lidar_frame, parent_frame, child_frame, should_reverse_transform) to support flexible transform hierarchies
- Changed output format from JSON to YAML with two-stage transform output (tf.yaml for raw calibration, tf_postprocessed.yaml for final transforms)
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| sensor_calibration_manager/launch/xx1_gen2/tag_based_pnp_calibrator.launch.xml | Added new frame parameters and updated parameter passing to calibrator nodes |
| sensor_calibration_manager/launch/xx1_15/tag_based_pnp_calibrator.launch.xml | Added new frame parameters and reorganized variable definitions |
| sensor_calibration_manager/launch/xx1/tag_based_pnp_calibrator.launch.xml | Added new frame parameters with duplicate lidar_frame assignment issue |
| sensor_calibration_manager/launch/x2/tag_based_pnp_calibrator.launch.xml | Added new frame parameters with camera-specific parent_frame configurations |
| sensor_calibration_manager/launch/rdv/tag_based_pnp_calibrator.launch.xml | Added new frame parameters with duplicate lidar_frame assignment issue |
| sensor_calibration_manager/launch/default_project/tag_based_pnp_calibrator.launch.xml | Updated to use image_frame parameter but missing parent/child frame parameters |
| common/tier4_calibration_views/tier4_calibration_views/image_view_ui.py | Added file loading functions, source selection UI, and status display with transform/intrinsics visualization |
| common/tier4_calibration_views/tier4_calibration_views/image_view_ros_interface.py | Added transform conversion methods and frame validation with parameter-based frame configuration |
| calibrators/interactive_camera_lidar_calibrator/interactive_camera_lidar_calibrator/ros_interface.py | Removed save_calibration_tfs method (moved to UI layer) |
| calibrators/interactive_camera_lidar_calibrator/interactive_camera_lidar_calibrator/interactive_calibrator.py | Updated UI text, removed optimized intrinsics checkbox, added dual YAML output for transforms |
| calibrators/interactive_camera_lidar_calibrator/interactive_camera_lidar_calibrator/image_view.py | Deleted duplicate file (functionality exists in tier4_calibration_views) |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
common/tier4_calibration_views/tier4_calibration_views/image_view_ui.py
Outdated
Show resolved
Hide resolved
common/tier4_calibration_views/tier4_calibration_views/image_view_ui.py
Outdated
Show resolved
Hide resolved
| d = {} | ||
| d["image_width"] = self.optimized_camera_info.width | ||
| d["image_height"] = self.optimized_camera_info.height | ||
| d["camera_name"] = self.optimized_camera_info.header.frame_id.split("/")[0] # TEMP |
Copilot
AI
Dec 2, 2025
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.
The comment "TEMP" on line 380 suggests this is temporary code that extracts camera name by splitting the frame_id. This implementation could be fragile if frame_id format changes. Consider either removing the comment if this is the final implementation, or implementing a more robust solution.
| d["camera_name"] = self.optimized_camera_info.header.frame_id.split("/")[0] # TEMP | |
| import re | |
| frame_id = self.optimized_camera_info.header.frame_id | |
| match = re.match(r"([^/]+)", frame_id) | |
| d["camera_name"] = match.group(1) if match else frame_id |
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.
Explicit camera_name parameter introduced in 2ac84a0 (defaults to "camera") and to be tested testing done.
| use_rpy = True | ||
|
|
||
| calibrated_tf = { | ||
| "x": self.calibrated_transform[0, 3].item(), | ||
| "y": self.calibrated_transform[1, 3].item(), | ||
| "z": self.calibrated_transform[2, 3].item(), | ||
| } | ||
| if use_rpy: | ||
| rpy = transforms3d.euler.mat2euler(self.calibrated_transform[0:3, 0:3]) | ||
| calibrated_tf["roll"] = rpy[0] | ||
| calibrated_tf["pitch"] = rpy[1] | ||
| calibrated_tf["yaw"] = rpy[2] | ||
| else: | ||
| quat = transforms3d.quaternions.mat2quat(self.calibrated_transform[0:3, 0:3]) | ||
| calibrated_tf["qx"] = quat[1] | ||
| calibrated_tf["qy"] = quat[2] | ||
| calibrated_tf["qz"] = quat[3] | ||
| calibrated_tf["qw"] = quat[0] |
Copilot
AI
Dec 2, 2025
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.
Unused variable use_rpy. This variable is set to True on line 409 but never changes. The code structure suggests it was intended to allow switching between rpy and quaternion output formats, but currently only the rpy path is active. Consider either removing this variable and the unused quaternion code (lines 421-426), or implementing the switching logic if both formats are needed.
| use_rpy = True | |
| calibrated_tf = { | |
| "x": self.calibrated_transform[0, 3].item(), | |
| "y": self.calibrated_transform[1, 3].item(), | |
| "z": self.calibrated_transform[2, 3].item(), | |
| } | |
| if use_rpy: | |
| rpy = transforms3d.euler.mat2euler(self.calibrated_transform[0:3, 0:3]) | |
| calibrated_tf["roll"] = rpy[0] | |
| calibrated_tf["pitch"] = rpy[1] | |
| calibrated_tf["yaw"] = rpy[2] | |
| else: | |
| quat = transforms3d.quaternions.mat2quat(self.calibrated_transform[0:3, 0:3]) | |
| calibrated_tf["qx"] = quat[1] | |
| calibrated_tf["qy"] = quat[2] | |
| calibrated_tf["qz"] = quat[3] | |
| calibrated_tf["qw"] = quat[0] | |
| calibrated_tf = { | |
| "x": self.calibrated_transform[0, 3].item(), | |
| "y": self.calibrated_transform[1, 3].item(), | |
| "z": self.calibrated_transform[2, 3].item(), | |
| } | |
| rpy = transforms3d.euler.mat2euler(self.calibrated_transform[0:3, 0:3]) | |
| calibrated_tf["roll"] = rpy[0] | |
| calibrated_tf["pitch"] = rpy[1] | |
| calibrated_tf["yaw"] = rpy[2] |
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.
Indeed the branching is currently unused, but I want to leave future option to choose either rpy or quaternion.
sensor_calibration_manager/launch/xx1/tag_based_pnp_calibrator.launch.xml
Outdated
Show resolved
Hide resolved
sensor_calibration_manager/launch/rdv/tag_based_pnp_calibrator.launch.xml
Outdated
Show resolved
Hide resolved
...active_camera_lidar_calibrator/interactive_camera_lidar_calibrator/interactive_calibrator.py
Outdated
Show resolved
Hide resolved
...active_camera_lidar_calibrator/interactive_camera_lidar_calibrator/interactive_calibrator.py
Show resolved
Hide resolved
...active_camera_lidar_calibrator/interactive_camera_lidar_calibrator/interactive_calibrator.py
Outdated
Show resolved
Hide resolved
….launch.xml Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…mera_lidar_calibrator/interactive_calibrator.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…mera_lidar_calibrator/interactive_calibrator.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
…iew_ui.py Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
shmpwk
left a comment
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
Owen-Liuyuxuan
left a comment
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
muraki-t4
left a comment
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
Description
Screencast.from.2025.11.28.10.09.24.webm
(Now the selections are
"ROS topic (initial)"and"ROS topic (current)"instead of"Initial /tf"and"Current /tf")Huge improvements and changes for interactive calibrator. This includes:
MAJOR:
"Save calibration"(which is now"Save pairs and results") button has changed:tf.yaml, a new filetf_postprocessed.yamlcontaining the postprocessed (see below) extrinsics is generated.optimized_camera_info.yaml) and sensor calibrator manager. For the extrinsics output, the format is[yaml root].<parent_frame>.<child_frame>containing keysx,y,z,roll,pitch,yaw.image_frame,lidar_frame,parent_frameandchild_framein thelaunch.xml.tf.yamlcontains transform fromimage_frame -> lidar_frame, andtf_postprocessed.yamlcontains transform fromparent_frame -> child_frame.should_reverse_transformshould be set to true (defualt) ifimage_frame -> lidar_frameandparent_frame -> child_frameis in the reverse direction, and false otherwise.parent_frameandchild_framefor each project is taken fromsensor_calibration_manager/calibrators/<project>/tag_based_pnp_calibrator.py. (One future work not addressed in this PR would be reducing redundancy of<project>/<calibrator>.pyand<project>/<calibrator>.launch.xml.)"Save pairs and results". For loading extrinsics, bothtf.yamlandtf_postprocessed.yamlare supported, and quaternion variants (keys arex,y,z,qw,qx,qy,qz) are supported as well as ordinary rpy.minor:
interactive_camera_lidar_calibrator/image_view.pywhich is a duplicate oft4_calibration_views/image_view.pyand in fact unused.Related links
Tests performed
Notes for reviewers
Since this PR is huge, each commit (roughly) corresponds to each feature in the commit message. You can review each commits one by one if necessary.
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.