Skip to content

Support more distortion parameters #25

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

Merged
merged 1 commit into from
Mar 30, 2025
Merged

Support more distortion parameters #25

merged 1 commit into from
Mar 30, 2025

Conversation

RuibinMa
Copy link

@RuibinMa RuibinMa commented Mar 6, 2025

This pull request add a "Radial" camera model which has k1,k2 distortion parameters into camera.py. Jacobians of this camera are implemented.

Several changes are made in lm_optimizer.py, perspective_fields.py, utils.py, geocalib.py and cameras.py to support arbitrary number of distortion parameters, instead of only one k1 parameter.

The only thing I'm not sure about is the change in the Jacobian of the up vectors in perspective_fields.py. The old code seems to be heavily based on the assumption that there is only k1 and used some tricks to simplify the computation, like torch.diag_embed. My change is derived from Eq(9) from scratch, and it could produce the same result as the original code on the testing image.

However, when I compare the values of the computed Jacobian (J_dist) and the original Jacobian (J_k1), I noticed their values become different after the first iteration, the direction seems correlated, for example (1.2, 1.0) vs (1.0, 0.8). I'm not sure if this is because there is a constant different between them which was discarded in the original implementation.

Could you help take a look at my implementation? I think this change can benefit the user community who wants to use more complicated cameras (SimpleRadial couldn't handle the fisheye examples).

Thank you so much!

Ruibin

…ary number of distortion parameters. Add a 'Radial' camera model which has k1 and k2 parameters.
@RuibinMa RuibinMa mentioned this pull request Mar 6, 2025
@RuibinMa RuibinMa changed the title Generalize lm_optimizer.py and perspective_fields.py to handle arbitr… Support more distortion parameters Mar 6, 2025
@veichta
Copy link
Collaborator

veichta commented Mar 9, 2025

Hey, that is super nice! Thank you for the help.

I've done some digging and noticed a few things:

  1. The gradients seem to be broken for the simple_radial camera now. You can check them against autograd using python -m siclib.geometry.gradient_checker. Just make sure to import the modules from geocalib instead of siclib with your updates, and change the camera model to simple_radial.
  2. It would be nice to keep the code a general as possible. For example, it would be nice to return a Jacobian of shape (... N, 2, K) in this lines for all camera models to avoid the if statement.

I did not find the error for the jacobians but will continue to look into it when I find the time.

@RuibinMa
Copy link
Author

Hey, that is super nice! Thank you for the help.

I've done some digging and noticed a few things:

  1. The gradients seem to be broken for the simple_radial camera now. You can check them against autograd using python -m siclib.geometry.gradient_checker. Just make sure to import the modules from geocalib instead of siclib with your updates, and change the camera model to simple_radial.
  2. It would be nice to keep the code a general as possible. For example, it would be nice to return a Jacobian of shape (... N, 2, K) in this lines for all camera models to avoid the if statement.

I did not find the error for the jacobians but will continue to look into it when I find the time.

  1. I'll take a look.
  2. Totally agree. I think my code generalizes most places to use (..., N, 2, K)

@manuelknott
Copy link

Hi @veichta,

Related to this discussion:

I tried to run python -m siclib.geometry.gradient_checker for the Pinhole and SimpleRadial model but it fails with TypeError: unsupported operand type(s) for /: 'NoneType' and 'int' in the fov2focal function.

To me, this seems to be unrelated to the broken camera model you mentioned but rather a test implementation issue. Is this supposed to work and am I missing something?

Thx in advance.

@veichta
Copy link
Collaborator

veichta commented Mar 20, 2025

I believe the issue comes from this line. You can replace it with this and it should work:

f = param_dict.get("f") if vfov is None else fov2focal(vfov, h)

veichta added a commit that referenced this pull request Mar 30, 2025
@veichta veichta merged commit 1a8a825 into cvg:main Mar 30, 2025
@veichta
Copy link
Collaborator

veichta commented Mar 30, 2025

Hey @RuibinMa!

I've made some updates and merged the PR, which means we now support the radial camera model! 🎉

Really appreciate all your help with this!

@RuibinMa
Copy link
Author

@veichta Great!! My pleasure!

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.

3 participants