Fix model_cropper not resetting image.num_points3D of cropped_rec #2557
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
ahojnnes
requested changes
May 8, 2024
src/colmap/scene/reconstruction.cc
Outdated
| @@ -452,8 +452,8 @@ Reconstruction Reconstruction::Crop( | |||
| for (const auto& image : images_) { | |||
| auto new_image = image.second; | |||
| new_image.SetRegistered(false); | |||
| for (auto& point2D : new_image.Points2D()) { | |||
| point2D.point3D_id = kInvalidPoint3DId; | |||
| for (point2D_t pid = 0; pid < new_image.NumPoints2D(); ++pid) { | |||
There was a problem hiding this comment.
Suggested change
| for (point2D_t pid = 0; pid < new_image.NumPoints2D(); ++pid) { | |
| const num_points2D = new_image.NumPoints2D(); | |
| for (point2D_t point2D_idx = 0; point2D_idx < num_points2D; ++point2D_idx) { |
There was a problem hiding this comment.
Added with type specifier in 6f716b8
src/colmap/scene/reconstruction.cc
Outdated
| for (auto& point2D : new_image.Points2D()) { | ||
| point2D.point3D_id = kInvalidPoint3DId; | ||
| for (point2D_t pid = 0; pid < new_image.NumPoints2D(); ++pid) { | ||
| new_image.ResetPoint3DForPoint2D(pid); |
There was a problem hiding this comment.
Suggested change
| new_image.ResetPoint3DForPoint2D(pid); | |
| new_image.ResetPoint3DForPoint2D(p oint2D_idx); |
There was a problem hiding this comment.
Changed and removed white space in 6f716b8
ahojnnes
approved these changes
May 8, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
While investigating the model_splitter issue for the current revision 1f95f3e described in #2096 I found a code regression introduced with commit 67029ad
This affects model_cropper and model_splitter tools.
As the previous implementation used the Image::ResetPoint3DForPoint2D method inside the loop, the num_points3D member was reset to 0 correctly.
colmap/src/colmap/scene/image.cc
Lines 97 to 103 in 1f95f3e
When directly assigning the kInvalidPoint3DId value, new_image keeps the value of the source num_points3D representing an incorrect state of the newly created cropped_reconstruction and finally leads to a failing check at
colmap/src/colmap/scene/reconstruction.cc
Line 184 in 1f95f3e
Fixing this by adding the call to Image::ResetPoint3DForPoint2D inside the loop again as it was first implemented in 99c7904.
Debugger screenshots:
Current Revision 1f95f3e :
Old implementation:
