Skip to content

Conversation

@paulsohn
Copy link
Contributor

@paulsohn paulsohn commented Nov 28, 2025

Description

image
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:

    • Now camera intrinsics and camera-lidar extrinsics can be read from files, and the image view is adjusted from the current selection of intrinsics / extrinsics.
    • The current selected value of extrinsics and intrinsics are directly displayed on the textbox. (For intrinsics, only the relevant fields are displayed - distortion coefficient(D) and camera matrix(K))
    • Output format of "Save calibration" (which is now "Save pairs and results") button has changed:
      • Now the markup is in yaml instead of json.
      • In addition to tf.yaml, a new file tf_postprocessed.yaml containing the postprocessed (see below) extrinsics is generated.
      • The output formats are consistent with the output of our intrinsic calibrator (optimized_camera_info.yaml) and sensor calibrator manager. For the extrinsics output, the format is [yaml root].<parent_frame>.<child_frame> containing keys x,y,z,roll,pitch,yaw.
    • Interactive calibrator now accepts image_frame, lidar_frame, parent_frame and child_frame in the launch.xml. tf.yaml contains transform from image_frame -> lidar_frame, and tf_postprocessed.yaml contains transform from parent_frame -> child_frame. should_reverse_transform should be set to true (defualt) if image_frame -> lidar_frame and parent_frame -> child_frame is in the reverse direction, and false otherwise.
      • parent_frame and child_frame for each project is taken from sensor_calibration_manager/calibrators/<project>/tag_based_pnp_calibrator.py. (One future work not addressed in this PR would be reducing redundancy of <project>/<calibrator>.py and <project>/<calibrator>.launch.xml.)
    • Can load extrinsics/intrinsics from the exact output of "Save pairs and results". For loading extrinsics, both tf.yaml and tf_postprocessed.yaml are supported, and quaternion variants (keys are x,y,z,qw,qx,qy,qz) are supported as well as ordinary rpy.
    • Calibration of extrinsics are performed on top of the currently selected intrinsics, so "Use optimized intrinsics" checkbox for extrinsics calibration are removed.
  • minor:

    • (non-user-facing) Removed interactive_camera_lidar_calibrator/image_view.py which is a duplicate of t4_calibration_views/image_view.py and in fact unused.
    • Obscure text UIs are updated (e.g. "Save calibration" / "Load calibration" -> "Save pairs and result" / "Load pairs")

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.

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

@Owen-Liuyuxuan Owen-Liuyuxuan self-requested a review November 28, 2025 02:16
@Owen-Liuyuxuan
Copy link
Contributor

Gemini Review

Summary

This Pull Request (PR) significantly enhances the interactive_camera_lidar_calibrator tool by enabling users to specify camera intrinsics and camera-lidar extrinsics from files at runtime. Previously, these parameters were likely sourced only from ROS topics or hardcoded values. The PR also introduces a more user-friendly display of the currently selected intrinsics and extrinsics directly within the GUI and changes the output format for saving calibration results from JSON to YAML.

The core changes involve:

  1. Refactoring: The image_view.py file, which contained the core image rendering logic, has been removed, implying its functionality was integrated or refactored into image_view_ui.py or other components.
  2. File-based Configuration: Introduction of QFileDialog and YAML parsing/writing capabilities to load and save calibration parameters (intrinsics and extrinsics) from/to files.
  3. Enhanced UI Display: The GUI now directly displays the active intrinsics (distortion coefficients and camera matrix) and extrinsics in textboxes, providing immediate feedback to the user.
  4. ROS Parameterization: The ROS interface (image_view_ros_interface.py) is updated to declare and use new parameters (image_frame, lidar_frame, parent_frame, child_frame, should_reverse_transform), making the calibrator more configurable via launch files.
  5. Launch File Updates: All relevant launch files (tag_based_pnp_calibrator.launch.xml for default_project, rdv, x2, xx1, xx1_15, xx1_gen2) are updated to pass these new parameters to the calibrator nodes.

Visual Overview

The 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
Loading

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 Changes

Logic Changes

The core logic changes revolve around how calibration parameters are managed and presented in the GUI. The removal of image_view.py suggests a consolidation of rendering logic, likely into image_view_ui.py or a more integrated approach within the ImageViewUI class.

The interactive_calibrator.py file now handles:

  1. YAML Serialization/Deserialization: Custom float_representer and list_representer are added to yaml to ensure consistent and readable output for floating-point numbers and lists, which is crucial for calibration parameters.
  2. Camera Info Management: The ImageViewUI class now distinguishes between message_camera_info (from ROS topic), optimized_camera_info, and source_camera_info (potentially from file or initial state), allowing for more flexible handling of camera parameters.
  3. File Dialog Integration: The QFileDialog is used to allow users to select input files for intrinsics and extrinsics, and output files for saving results.

The ros_interface.py no longer contains the save_calibration_tfs method, as saving is now handled by the UI layer with the new YAML format.

Architecture Impact

The PR introduces a clearer separation of concerns by moving file I/O and UI-specific logic into interactive_calibrator.py and image_view_ui.py, while ros_interface.py focuses purely on ROS communication. The introduction of new ROS parameters in image_view_ros_interface.py makes the underlying ROS node more generic and configurable.

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
Loading

Explanation: ImageViewRosInterface now declares and manages more ROS parameters, making it more configurable. ImageViewUI (and by extension InteractiveCalibratorUI) now has dedicated fields for different sources of CameraInfo and handles file-based loading/saving of calibration parameters.

Code Quality Evaluation

The PR demonstrates good practices in several areas:

  • Modern Python: Usage of yaml for configuration, which is a common and robust choice in robotics. The custom representers for float and list ensure clean and consistent YAML output.
  • Modularity: The refactoring, especially the removal of image_view.py and the clear parameterization in image_view_ros_interface.py, improves modularity and maintainability.
  • User Experience: Displaying current intrinsics/extrinsics directly in the UI and allowing file-based input significantly enhances the usability of the calibrator.
  • Robustness: Explicitly declaring ROS parameters in image_view_ros_interface.py makes the node's dependencies clearer and allows for easier configuration via launch files.

Potential areas for minor enhancement or consideration:

  • Error Handling for File I/O: While QFileDialog handles file selection, robust error handling (e.g., for malformed YAML files, missing keys) should be thoroughly implemented in the loading functions (load_intrinsics_from_file, load_extrinsics_from_file).
  • Type Safety/Clarity: The message_camera_info, optimized_camera_info, source_camera_info variables are all of type CameraInfo. While clear in context, ensuring consistent naming and documentation for their specific roles is good.
  • Logging: Ensure appropriate logging levels are used for file operations (e.g., INFO for successful load/save, WARNING/ERROR for failures).

Does it accomplish what it claims?

Yes, the PR successfully accomplishes what it claims.

  • "Now camera intrinsics and camera-lidar extrinsics can be read from files": The patches show the introduction of QFileDialog and YAML parsing, along with new UI elements to facilitate this.
  • "the image view is adjusted from the current selection of intrinsics / extrinsics": While the direct adjustment logic isn't fully visible in the patches, the capability to load and manage these parameters is established, which is a prerequisite for such adjustment.
  • "The current selected value of extrinsics and intrinsics are directly displayed on the textbox": The import of QPlainTextEdit and the description strongly suggest this UI feature is implemented.
  • "Output format of "Save calibration" (which is now "Save pairs and results") button has changed": The removal of json imports and save_calibration_tfs method, coupled with the addition of yaml representers, confirms the shift to YAML output.
  • "Now the selections are "ROS topic (initial)" and "ROS topic (current)" instead of "Initial /tf" and "Current /tf"": This is a minor UI text change, confirmed by the description.

The changes in the launch files also confirm the new parameterization for image_frame, lidar_frame, parent_frame, child_frame, and should_reverse_transform, making the calibrator more flexible across different vehicle configurations (XX1, X2, Gen1/Gen2).

What can be enhanced beyond this PR?

  1. Configuration Management UI: For complex systems with many cameras/LiDARs, a more advanced configuration management UI could be beneficial. This might include:
    • A dropdown to select pre-defined sensor configurations.
    • Visual indicators for which parameters are loaded from file vs. ROS topics.
    • Batch loading/saving of multiple sensor calibrations.
  2. Validation of Loaded Parameters: Implement more rigorous validation for loaded intrinsics/extrinsics (e.g., checking matrix dimensions, positive focal lengths, valid distortion models) to prevent runtime errors from malformed files.
  3. Visualization of Parameter Changes: When loading new intrinsics or extrinsics, a visual diff or a clear indication of how the projection changes could help users understand the impact of their loaded files.
  4. Integration with Calibration API: The use_calibration_api parameter is set to false in the launch files. Exploring deeper integration with a centralized calibration API (if one exists or is planned) could streamline the calibration workflow for production systems.
  5. Undo/Redo Functionality: For interactive tools, an undo/redo stack for calibration adjustments or parameter loading could significantly improve usability and reduce user frustration.
  6. Support for other file formats: While YAML is good, supporting other common formats like JSON or even custom binary formats for performance could be considered if there's a use case.

Is there any things not considered well in this PR?

  1. Backward Compatibility for Saved Files: The PR changes the output format from JSON to YAML. While this is an improvement, it means older saved calibration files in JSON format might not be directly loadable by the new tool. A migration path or a clear warning/documentation for users with existing JSON files would be beneficial.
  2. User Guidance for File Formats: While YAML is used, the exact structure of the YAML files for intrinsics and extrinsics is not explicitly defined in the PR description. Providing example YAML files or a schema in the documentation would greatly assist users in creating valid input files.
  3. Impact of image_view.py Removal: The complete deletion of image_view.py is a major refactoring. While likely intended, it's important to ensure that all functionalities (e.g., specific rendering optimizations, threading models) from the deleted file have been correctly migrated and integrated into the new structure without regressions. A detailed explanation of this refactoring would be helpful.
  4. Thread Safety for UI Updates: Given that ImageViewUI and InteractiveCalibratorUI interact with ROS topics and potentially file I/O, careful consideration of thread safety for UI updates is crucial to prevent crashes or unresponsive behavior. While PySide2 handles some of this, explicit checks for cross-thread UI access are always good.
  5. Launch File Complexity: The launch files, especially for rdv, x2, xx1, xx1_15, xx1_gen2, have become quite verbose with many let and param definitions, including conditional logic. While necessary for flexibility, this can make them harder to read and maintain. Consider using more structured YAML configuration files for parameters if the complexity grows further, or consolidating common parameters.

@Owen-Liuyuxuan
Copy link
Contributor

@paulsohn From the review, there are several things I think reasonable:

  1. Validation of Loaded Parameters: Implement more rigorous validation for loaded intrinsics/extrinsics (e.g., checking matrix dimensions, positive focal lengths, valid distortion models) to prevent runtime errors from malformed files.
  2. Undo/Redo Functionality: For interactive tools, an undo/redo stack for calibration adjustments or parameter loading could significantly improve usability and reduce user frustration.
  3. Backward Compatibility for Saved Files: The PR changes the output format from JSON to YAML. While this is an improvement, it means older saved calibration files in JSON format might not be directly loadable by the new tool. A migration path or a clear warning/documentation for users with existing JSON files would be beneficial.

@paulsohn paulsohn force-pushed the feat/interactive-calib-runtime-intrinsics-extrinsics branch from 6d32e97 to 4df5595 Compare December 1, 2025 06:22
@paulsohn
Copy link
Contributor Author

paulsohn commented Dec 1, 2025

@Owen-Liuyuxuan

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)
3: Done 5201af8 , but I see this validation is too basic and would not filter out practical invalid intrinsics (at least it would prevent "opening extrinsics instead of intrinsics" cases, which likely have been raising exceptions already).

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.

Copy link
Contributor

@Owen-Liuyuxuan Owen-Liuyuxuan left a 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.

Copy link
Contributor

Copilot AI left a 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.

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

Copilot AI Dec 2, 2025

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.

Suggested change
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

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

@paulsohn paulsohn Dec 2, 2025

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.

Comment on lines +409 to +426
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]
Copy link

Copilot AI Dec 2, 2025

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.

Suggested change
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]

Copilot uses AI. Check for mistakes.
Copy link
Contributor Author

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.

shmpwk and others added 10 commits December 2, 2025 09:06
….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>
Copy link
Contributor

@shmpwk shmpwk left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Contributor

@Owen-Liuyuxuan Owen-Liuyuxuan left a comment

Choose a reason for hiding this comment

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

LGTM

@yabuta yabuta requested a review from muraki-t4 December 2, 2025 06:32
Copy link
Contributor

@muraki-t4 muraki-t4 left a comment

Choose a reason for hiding this comment

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

LGTM

@paulsohn paulsohn merged commit 5dcc5b1 into tier4/universe Dec 2, 2025
6 of 7 checks passed
@paulsohn paulsohn deleted the feat/interactive-calib-runtime-intrinsics-extrinsics branch December 2, 2025 06:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants