Skip to content
New issue

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

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

Already on GitHub? # to your account

[Fix] lidar2cam error in update_infos_to_v2.py for nus and lyft dataset. #2110

Merged

Conversation

lianqing11
Copy link
Collaborator

@lianqing11 lianqing11 commented Dec 7, 2022

Thanks for your contribution and we appreciate it a lot. The following instructions would make your pull request more healthy and more easily get feedback. If you do not understand some items, don't worry, just make the pull request and seek help from maintainers.

Motivation

lidar2cam error in update_infos_to_v2.py for nus and lyft dataset.

In the latest info version, we save the transformation from lidar to camera coordinate system with a unified transformation matrix, but the original implementation does not multiply the rotation matrix with the translation matrix, leading to errors in transformation matrices. This PR fixes this problem, and users should re-convert info with the new script.

This problem does not affect the currently supported models in 1.x but may yield potential errors for BEV-series detectors.

Modification

change the translation from -t to -R^{-1}t

@lianqing11 lianqing11 requested a review from ZCMax December 7, 2022 01:49
@VVsssssk
Copy link
Collaborator

VVsssssk commented Dec 8, 2022

Hi,thanks your PR. I think you need to modify lidar2sensor in lidar_sweeps too.

lidar2sensor[:3, 3] = -ori_sweep['sensor2lidar_translation']

@VVsssssk
Copy link
Collaborator

VVsssssk commented Dec 8, 2022

And lidar2sensor in image dict.

lidar2sensor[:3, 3] = -ori_info_dict['cams'][cam][

@lianqing11
Copy link
Collaborator Author

I have fixed the error in sweep.

@ZwwWayne ZwwWayne merged commit 4e28378 into open-mmlab:dev-1.x Dec 14, 2022
@Shiming94
Copy link

Shiming94 commented Aug 19, 2024

Hi, @lianqing11, @Tai-Wang , @ZwwWayne @VVsssssk, I still need to understand this change fully.

## The code snippet from the nuscenes converter

    R = (l2e_r_s_mat.T @ e2g_r_s_mat.T) @ (
        np.linalg.inv(e2g_r_mat).T @ np.linalg.inv(l2e_r_mat).T)
    T = (l2e_t_s @ e2g_r_s_mat.T + e2g_t_s) @ (
        np.linalg.inv(e2g_r_mat).T @ np.linalg.inv(l2e_r_mat).T)
    T -= e2g_t @ (np.linalg.inv(e2g_r_mat).T @ np.linalg.inv(l2e_r_mat).T
                  ) + l2e_t @ np.linalg.inv(l2e_r_mat).T

I think points_new = points @ R + T # here, R is already transposed makes sense, as it can be calculated from:
R_e2g @ (R_l2e @ points_new + T_l2e) + T_e2g = R_e2g_s @ (R_l2e_s @ points + T_l2e_s) + T_e2g_s.
so why the np.matul(R, T) is needed here?

Thank you a lot in advance. This question bothers me for a long time.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants