Skip to content

fix: remove safeguards for parsing colmap data - #3613

Closed
Yubel426 wants to merge 2 commits into
nerfstudio-project:mainfrom
Yubel426:patch-2
Closed

fix: remove safeguards for parsing colmap data#3613
Yubel426 wants to merge 2 commits into
nerfstudio-project:mainfrom
Yubel426:patch-2

Conversation

@Yubel426

Copy link
Copy Markdown

Fix #3611. The safeguards introduced in #3381 makes datasets with non-zero k4 coefficient such as fisheye unusable. When reading camera distortion coefficients such as OPENCV camera, k4 is correctly assigned as the fourth-order radial distortion coefficient rather than the fourth coefficient. Therefore, the safeguards can be removed.

@Yubel426

Yubel426 commented Mar 15, 2025

Copy link
Copy Markdown
Author

@kerrj @jb-ye Would you mind reviewing this PR?

@f-dy

f-dy commented Mar 17, 2025

Copy link
Copy Markdown
Contributor

This is incorrect. #3612 is correct.
colmap's k4, k5 and k6 are still unsupported for the perspective model.
k4 should be supported for the fisheye model.

@Yubel426

Yubel426 commented Mar 18, 2025

Copy link
Copy Markdown
Author

Hi @f-dy, for the perspective models—"SIMPLE_PINHOLE", "PINHOLE", "SIMPLE_RADIAL", and "RADIAL"—their k4 parameter is correctly set to 0 when reading the parameters. I'm not sure what the purpose of this error check is; please let me know if I've misunderstood anything.

It’s possible that k4 doesn’t support fisheye either; a new PR is needed here.

@ishipachev

ishipachev commented May 7, 2025

Copy link
Copy Markdown

@Yubel426
You shouldn't set k4 to zero when you read camera parameters if it has k4 non zero.

Imagine you have some random function over [a;b] and you have a polynom which approximates it: y = a * x^2 + bx + c
You can't "approximate" it further to linear model just by dropping a and having something like y = bx + c.

Instead you need to build a proper new approximation, not just dropping one coefficient from the reacher one.

@Yubel426 Yubel426 closed this May 18, 2025
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.

Throw an error on non-zero k4, k5 or k6

3 participants