8000 Add ObservationManager by sarlinpe · Pull Request #2575 · colmap/colmap · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Add ObservationManager #2575

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 24 commits into from
May 27, 2024
Merged

Add ObservationManager #2575

merged 24 commits into from
May 27, 2024

Conversation

sarlinpe
Copy link
Member
@sarlinpe sarlinpe commented May 24, 2024

This removes the filtering logic in Reconstruction as well as the SfM-specific data in Image, such as the visibility pyramid. The logic is unchanged except one minor point: I removed the num_observations column in the GUI database image viewer because this field is now only available during reconstruction. It is anyway mostly redundant with num_points2D.

@sarlinpe sarlinpe marked this pull request as ready for review May 25, 2024 08:37
@sarlinpe sarlinpe requested review from ahojnnes and B1ueber2y May 25, 2024 08:39

const double tri_angle = CalculateTriangulationAngle(
proj_center1, proj_center2, point3D.xyz);
EXPECT_GE(tri_angle, DegToRad(kMinTriAngleDeg));
Copy link
Member Author

Choose a reason for hiding this comment

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

This test sometimes fails on mac and Windows with the previous value of 1°. It seems that the sampling of camera poses doesn't constrain them to have a sufficient angle.

@sarlinpe sarlinpe enabled auto-merge (squash) May 25, 2024 17:07
@ahojnnes
Copy link
Contributor

Looks great to me. I assume most of the code in the ObservationManager is untouched and just moved from Reconstruction/Image classes? Have you run through a dataset and checked that it produces the exact same number of final observations?

@sarlinpe
Copy link
Member Author

PTAL. I have indeed mostly moved around code without change of logic. I have checked that the result is identical in terms of reconstruction statistics.

@sarlinpe sarlinpe requested a review from ahojnnes May 26, 2024 21:46
@sarlinpe sarlinpe disabled auto-merge May 27, 2024 07:04
@sarlinpe sarlinpe enabled auto-merge (squash) May 27, 2024 07:05
@ahojnnes
Copy link
Contributor

Perfect, thanks a lot.

@sarlinpe sarlinpe merged commit eaebe3c into main May 27, 2024
15 checks passed
@sarlinpe sarlinpe deleted the sarlinpe/observation-manager branch May 27, 2024 07:27
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.

2 participants
0