Skip to content
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

colmap2nerf: Use separate intrinsics if available #1357

Merged
merged 5 commits into from
Jun 6, 2023

Conversation

FlorisE
Copy link
Contributor

@FlorisE FlorisE commented May 22, 2023

Currently only the last images' intrinsics are being used.

Recently COLMAP added a checkbox to do automatic reconstruction with separate intrinsics per subfolder.

I'm using a rig with multiple cameras (same model), so I thought it would be nice to see if using intrinsics per camera would improve the reconstruction.

Actually there doesn't seem to be a qualitative improvement, but maybe my modification to the script is still useful when for example using non-homogeneous cameras/lenses.

Copy link
Collaborator

@Tom94 Tom94 left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me! I've got just two minor change requests before hitting merge

is_fisheye = False
camera = {}
camera_id = int(els[0])
camera['w'] = float(els[2])
Copy link
Collaborator

Choose a reason for hiding this comment

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

Could all the ''-strings be changed to ""-strings?

@@ -333,6 +348,23 @@ def closest_point_2_lines(oa, da, ob, db): # returns point closest to both rays
up += c2w[0:3,1]

frame = {"file_path":name,"sharpness":b,"transform_matrix": c2w}
if len(cameras) != 1:
camera = cameras[int(elems[8])]
frame["camera_angle_x"] = camera["angle_x"]
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be expressed more concisely using frame |= camera if the angle_x parameter is renamed to camera_angle_x.

@FlorisE
Copy link
Contributor Author

FlorisE commented Jun 5, 2023

Sorry it took a while, but I applied the suggested changes.

|= is only supported from Python 3.9 for dicts, is it a problem to increase the minimum Python version required to run the script?

@Tom94
Copy link
Collaborator

Tom94 commented Jun 5, 2023

Oh, I hadn't realized |= is so recent. Thanks for catching that -- I wouldn't want to force users to upgrade their Python just to save a couple of LoC. Please feel free to revert. (And note that indentation should use tabs and not spaces. I'm spotting a few spaces in the diff.)

@FlorisE
Copy link
Contributor Author

FlorisE commented Jun 5, 2023

I think we can get the best of both worlds by using update() instead of |=.

Also I accidentally changed aabb_scale to 64, so I changed it back to 32.

@Tom94
Copy link
Collaborator

Tom94 commented Jun 6, 2023

LGTM, thanks again!

@Tom94 Tom94 merged commit b8c66f4 into NVlabs:master Jun 6, 2023
fnysalehi pushed a commit to fnysalehi/instant-ngp-rendering that referenced this pull request May 14, 2024
colmap2nerf: Use separate intrinsics if available
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.

None yet

2 participants