Fix camera matrix correction method

[tuxbotix]

Why? What?

The current implementation of to_corrected() applies the new correction on top of existing corrected values. Since this method is used in camera calibration, the parameters obtained by the solver also reflects this issue.

Current state:

• T = C_o * T_m at construction (C_o from parameter file)
• T_n = C_n * T when using to_corrected()
• So the total correction C_t will be C_o + C_n
• Neither C_o, nor T_m are stored in CameraMatrix, so I'll have to read from parameter files and re-calculate this which is wonky IMO.

What we need:

• T = C_o * T_m at construction (C_o from parameter file)
• T_n = C_n * T_m when using to_corrected()

Changes:

• This PR fixes it by storing the transformation values as per the theoritical model T_m so that corrections can always be done from that state. It is also verified by the added unit test.
• Also introduces robot rotation values as extra corrections.

Fixes #

ToDo / Known Issues

• Discuss and evaluate if we can do this differently

Ideas for Next Iterations (Not This PR)

If there are some improvements that could be done in a next iteration, describe them here.

How to Test

Describe how to test your changes. (For the reviewer)

[oleflb]

I did not fully get your explanation. Why do you have to apply the correction differently now? Reading the test case it seems like you want the correction to be idempotent, is this what you are after?

[oleflb]

corrected_variant

[tuxbotix]

I did not fully get your explanation. Why do you have to apply the correction differently now? Reading the test case it seems like you want the correction to be idempotent, is this what you are after?

I'm sorry for my bad explanation, the reasoning starts from how it is constructed by camera matrix calculator Vs how get_corrected is used.

The parameters give the full amount of correction needed and camera matrix calculator applies it to the bead to camera matrix before building CameraMatrix. But there is no information about the correction nor the state before correction.

In the current state, get_corrected means whatever additional offsets needed, not the absolute correction. So the calibration solver using this function also returns the relative values.
That means I have to re-read the existing parameters and calculate the full amount of correction before writing to the parameters file.

What I want instead is to store the head to camera matrix before correction or less preferably the original correction value we added.
It is simply encapsulation of all the information instead of sourcing it from different places and build back.

[oleflb]

Hmm I feel like the CameraMatrix struct shouldn't contain all of this state. Ideally it'd would only consist of Extrinsinc and Intrinsic. This is because the calibrations are only applied once in the extrinsic chain and then all nodes using the CameraMatrix work with the calibrated matrix (and they would never want to use the uncalibrated one). If you want the uncalibrated CameraMatrix, I would favor just creating a second instance of the existing CameraMatrix struct (uncalibrated_camera_matrix)

[tuxbotix]

I just saw your reply, I was already thinking of how else we can do this and came to the same conclusion - have an uncalibrated camera matrix instance - since the camera matrix already contains a lot of information and I feel it is already overcomplicated.

As you said, in an ideal world we only need the two main matrices but our needs and design choices made it harder. Perhaps we can reconsider this design and see what can we do about it (i.e. we have many 180° rotations with recent axis changes).

I'll change the module and you can review again :)

[oleflb]

I also ran into this issue and fully understand your pain now :D I still think having an uncalibrated and a calibrated variant is the best solution to this problem. Did you implement something like this already?

[tuxbotix]

I partly did it but didn't finish it, but should be quite easy to do, so if you or someone else have time please go ahead 😅.

…

On Mon, 10 Mar 2025, 16:52 ole.flb, ***@***.***> wrote: I also ran into this issue and fully understand your pain now :D I still think having an uncalibrated and a calibrated variant is the best solution to this problem. Did you implement something like this already? — Reply to this email directly, view it on GitHub <#1650 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTOBXYWYWA57AHSTYBVLBL2TXURBAVCNFSM6AAAAABX3AOFB2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMJRGY3DSOJSGE> . You are receiving this because you authored the thread.Message ID: ***@***.***> [image: oleflb]*oleflb* left a comment (HULKs/hulk#1650) <#1650 (comment)> I also ran into this issue and fully understand your pain now :D I still think having an uncalibrated and a calibrated variant is the best solution to this problem. Did you implement something like this already? — Reply to this email directly, view it on GitHub <#1650 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/ABTOBXYWYWA57AHSTYBVLBL2TXURBAVCNFSM6AAAAABX3AOFB2VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDOMJRGY3DSOJSGE> . You are receiving this because you authored the thread.Message ID: ***@***.***>

[oleflb]

This is implemented now in #1690. Please close if that PR suits your use case as well

[tuxbotix]

Oh neat, that's the alternative I wanted to implement. So all good 😋