Repository navigation
Add ObservationManager - #2575
Merged
Merged
Conversation
sarlinpe
marked this pull request as ready for review
May 25, 2024 08:37
sarlinpe
commented
May 25, 2024
|
|
||
| const double tri_angle = CalculateTriangulationAngle( | ||
| proj_center1, proj_center2, point3D.xyz); | ||
| EXPECT_GE(tri_angle, DegToRad(kMinTriAngleDeg)); |
Member
Author
There was a problem hiding this comment.
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
enabled auto-merge (squash)
May 25, 2024 17:07
ahojnnes
reviewed
May 26, 2024
ahojnnes
reviewed
May 26, 2024
ahojnnes
reviewed
May 26, 2024
ahojnnes
reviewed
May 26, 2024
ahojnnes
reviewed
May 26, 2024
ahojnnes
reviewed
May 26, 2024
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? |
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
disabled auto-merge
May 27, 2024 07:04
sarlinpe
enabled auto-merge (squash)
May 27, 2024 07:05
Contributor
|
Perfect, thanks a lot. |
ahojnnes
approved these changes
May 27, 2024
fringehush-gif
added a commit
to fringehush-gif/colmap
that referenced
this pull request
Nov 9, 2026
## Summary Update the visibility pyramid when a feature gains its first triangulated correspondence or loses its last. Additional correspondences change support but leave the visible feature set unchanged, so repeating every pyramid-level update does unnecessary work. ## Changes - Put SetPoint and ResetPoint beside the existing zero/one visibility-count transitions, preserving occupied-cell scores for colliding features. - Extend the existing visibility-score and AddImage tests to cover repeated support, collisions, partial/final removal, underflow and rebuilding the manager. ## Background The repeated updates are inherited from fe8f82a, "Initial commit" (2016), in src/base/image.cc. The ObservationManager extraction, eaebe3c, "Add ObservationManager (colmap#2575)", retained them. ## Results Fixed public Add64/Delete32/Delete32 bookkeeping on a 16-image, 64-track, 120-pair fixture: | Platform | Before | After | | --- | ---: | ---: | | Linux/WSL | 1.395 ms | 0.640 ms | | Windows | 2.776 ms | 1.438 ms | Each value averages four fresh process means per arm, ABBAABBA, with two warmup and ten measured batches per process. Canonical inputs and outputs match within each platform. Whole-command Linux observations, including setup/oracles/serialization: median CPU 0.495 -> 0.443 s, median per-process peak RSS 414.50 -> 414.35 MiB, mean instructions 3.879 -> 3.782 billion. Windows median CPU 2.234 -> 2.211 s and median per-process peak working set 14.54 -> 14.33 MiB have overlapping process ranges. These broader counters describe the probe, not isolated pyramid operations. ## Validation - Current composition: all 18 maintained manager cases and two mapper lifecycle cases passed, without skips. Earlier matched arms passed the same selected counts on Linux and Windows. - clang-tidy-18 completed on the owner and maintained test with no displayed source diagnostics; generated/header warnings were suppressed. Changed-line formatting passed with the real source filename. - Incremental build: three translation units and two test links, reusing 46 production objects and 13 archives. Source, selected-link and provider postflight checks completed using the retained outputs. AI disclosure: Totally vibe coded
fringehush-gif
added a commit
to fringehush-gif/colmap
that referenced
this pull request
Nov 9, 2026
## Summary Update the visibility pyramid when a feature gains its first triangulated correspondence or loses its last. Additional correspondences change support but leave the visible feature set unchanged, so repeating every pyramid-level update does unnecessary work. ## Changes - Put SetPoint and ResetPoint beside the existing zero/one visibility-count transitions, preserving occupied-cell scores for colliding features. - Extend the existing visibility-score and AddImage tests to cover repeated support, collisions, partial/final removal, underflow and rebuilding the manager. ## Background The repeated updates are inherited from fe8f82a, "Initial commit" (2016), in src/base/image.cc. The ObservationManager extraction, eaebe3c, "Add ObservationManager (colmap#2575)", retained them. ## Results On the composed-fork version, fixed public Add64/Delete32/Delete32 bookkeeping on a 16-image, 64-track, 120-pair fixture: | Platform | Before | After | | --- | ---: | ---: | | Linux/WSL | 1.395 ms | 0.640 ms | | Windows | 2.776 ms | 1.438 ms | Each value averages four fresh process means per arm, ABBAABBA, with two warmup and ten measured batches per process. Canonical inputs and outputs match within each platform. Whole-command Linux observations, including setup/oracles/serialization: median CPU 0.495 -> 0.443 s, median per-process peak RSS 414.50 -> 414.35 MiB, mean instructions 3.879 -> 3.782 billion. Windows median CPU 2.234 -> 2.211 s and median per-process peak working set 14.54 -> 14.33 MiB have overlapping process ranges. These broader counters describe the probe, not isolated pyramid operations. ## Validation - Clean base and candidate: all 15 maintained manager cases passed in each arm, without skips. The composed-fork version also passed 18 manager and two mapper cases on Linux and Windows. - Configured clang-tidy-18 checked observation_manager.cc in both arms and the maintained test, with no displayed source diagnostics; generated/header warnings were suppressed. Changed-line formatting passed. - Incremental qualification reused retained pristine-upstream support and rebuilt the two selected RANSAC-dependent owners needed for the newer headers. Final source, selected-link and runtime-provider checks passed. AI disclosure: Totally vibe coded
ahojnnes
pushed a commit
that referenced
this pull request
Nov 10, 2026
## Summary Update the visibility pyramid only when an image feature gains its first triangulated correspondence or loses its last. Extra supporting correspondences leave occupied cells unchanged, so repeating every pyramid-level update does unnecessary work. This is a small inherited optimization, based directly on official COLMAP 3c1a1b9. ## Changes - Move SetPoint/ResetPoint into the existing zero/one visibility transitions. - Extend the maintained score test with repeated support, distinct colliding features, partial/final removal and underflow checks. - Extend AddImage with observation removal and reconstruction of the manager, avoiding unrelated fork registration behavior. ## Background The update pattern dates to fe8f82a, "Initial commit" (2016), in src/base/image.cc. It was retained by eaebe3c, "Add ObservationManager (#2575)". ## Results The source-equivalent transition change was measured in a composed-fork fixed bookkeeping probe: Add64/Delete32/Delete32 on 16 images, 64 tracks and 120 pairs. | Platform | Before | After | | --- | ---: | ---: | | Linux/WSL | 1.395 ms | 0.640 ms | | Windows | 2.776 ms | 1.438 ms | Each table value is the arithmetic mean of four fresh process means per arm (ABBAABBA). Each process mean averages ten measured batches after two warmups. Canonical inputs and outputs agree within each platform. These are fixed bookkeeping timings, not reconstruction throughput measurements. ## Validation - Independent source review accepted the clean-base production change and adapted lifecycle fixture. - Clean base and candidate each passed all 15 maintained manager cases, without skips. - Configured clang-tidy-18 checked observation_manager.cc in both arms and the maintained test with no displayed source diagnostics; generated/header warnings were suppressed. - Reused retained pristine-upstream support and rebuilt the two selected consumers of newer RANSAC headers; final source, link and provider checks passed. - Separately, the composed-fork version passed 18 manager and two mapper cases on Linux and Windows. AI disclosure: Totally vibe coded
fringehush-gif
added a commit
to fringehush-gif/colmap
that referenced
this pull request
Nov 10, 2026
## Summary Integrate the upstream visibility-transition optimization into the fork while retaining its observation-manager behavior and both lifecycle checks. ## Changes - Merge upstream 137f216, "SfM: Update visibility on feature transitions (CPU perf) (colmap#5006)", as the second parent; retain fork d896761 and its current production implementation. - Exercise the upstream partial and final observation-removal sequence and the fork registration and deregistration sequence with independently constructed state in the existing AddImage test. Preserve the remaining fork tests. - Attribute the inherited pattern to fe8f82a, "Initial commit", and eaebe3c, "Add ObservationManager (colmap#2575)". ## Validation - The selected Linux observation-manager suite passed 18/18 cases with no skips; the AddImage case executed both independent sequences. The changed test and current reconstruction owner were rebuilt, and selected archive members, source dependencies and runtime providers were checked. - Configured clang-tidy18 on the changed test: no displayed source diagnostics; generated/header warnings suppressed, consistent with retained baseline. Clang-format 18.1.8 left the affected AddImage range unchanged. AI disclosure: Totally vibe coded
This file contains hidden or 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
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.
This removes the filtering logic in
Reconstructionas well as the SfM-specific data inImage, such as the visibility pyramid. The logic is unchanged except one minor point: I removed thenum_observationscolumn in the GUI database image viewer because this field is now only available during reconstruction. It is anyway mostly redundant withnum_points2D.