[ROCm] Fix round 2: merge main and maintainer nits (colmap#4635) - #5
Merged
Merged
Conversation
Summary: - remove 56 unused includes identified with clangd and the compilation database - retain includes needed by conditional builds, platform-specific code, macro expansion, and compatibility paths Testing: - PYENV_VERSION=colmap scripts/format/c++.sh - cmake --build build --parallel 8 - ctest --test-dir build --output-on-failure --parallel 8 (160 tests passed)
## Summary - expose the existing `Mapper.load_all_images` setting through mapper CLI/config options - forward the setting to the database cache used by `image_registrator` - cover command-line parsing and project-file round trips for the option This keeps the default behavior unchanged while allowing externally registered images without two-view geometry to be loaded when requested. Fixes colmap#4602 ## Testing - `scripts/format/c++.sh` with the required clang-format 22.1.5 (3 changed files) - configured a CPU-only release build with tests enabled - built `colmap_controllers_option_manager_test` and `colmap_main` (371/371 build steps) - ran `option_manager_test` (14/14 passed) - ran `colmap image_registrator --help` and verified `--Mapper.load_all_images arg (=0)` is exposed ## AI assistance AI tools assisted with repository analysis and implementation. I reviewed the resulting diff, traced the existing database-cache option flow, and ran the tests and executable checks listed above. --------- Co-authored-by: Johannes Schönberger <jsch@meta.com>
Bugfix for colmap#4630 **Summary** `SpatialPairGenerator::ReadPositionPriorData` calculates the mean coordinate of the image position priors and subtracts this from the positions matrix to improve numerical precision. In the case where some images do not have a pose prior, their corresponding zero-valued rows were still included in calculation of the mean position, and this could significantly bias the calculated mean position. For pose priors using projected Cartesian coordinates in particular, where coordinate values might be on the order of 1e6, this could result in large numerical errors and cause subsequent spatial matching to fail. **Changes** - `SpatialPairGenerator::ReadPositionPriorData` has been updated to trim unpopulated position rows prior to calculation of mean coordinate - Debug logging has been added that prints the calculated offset at log level 1 - A test `CentersLargeCoordinatesWithMissingPosePrior` has been added to test for regressions Hopefully this PR is welcome and follows general expectations; let me know if there are issues.
…lmap#4620) ## Summary - **Propagate overlap before re-partitioning.** Overlapping images between sibling clusters are now added to their direct children *before* recursing, so the next level sees them directly instead of propagating overlap to all descendants after partitioning. - **Let inherited overlap participate in the next graph cut**. An overlap image inherited from the parent becomes a vertex in the next graph cut partition. - **Enforce the documented leaf bound.** A cluster stops partitioning once it reaches `leaf_max_num_images + image_overlap`, so every leaf actually respects the documented size bound (previously multi-level leaves could exceed it). ## Test The new `HierarchicalTwoLevelsWithOverlap` test uses the 8-image scene below (`branching=2`, `image_overlap=2`, `leaf_max_num_images=3`): ``` Original scene: 100 50 100 (0) -------- (1) -------- (2) -------- (3) | | 10 10 | | (7) -------- (6) -------- (5) -------- (4) 100 50 100 ``` Level 1 splits the scene into two clusters, each receiving the two best-matching images from the sibling cluster as overlap: ``` level 1: C1 = branch {0,1,2,3} ∪ overlap {4,7} = {0, 1, 2, 3, 4, 7} level 1: C2 = branch {4,5,6,7} ∪ overlap {0,3} = {0, 3, 4, 5, 6, 7} ``` Level 2 splits C1 and C2 again. The inherited overlap images `{4,7}` / `{0,3}` have no edges in the child subgraphs, so the graph cut cannot label them —this is where the behaviors diverge. **Before this PR**, the inherited overlap is copied into every leaf of the subtree (leaves with 5 images). - **leaves**: `{0,1,2,4,7} {1,2,3,4,7} {0,3,4,5,6} {0,3,5,6,7} ` ``` level 2 (before): inherited overlap is copied into every leaf of the subtree C11 = branch {0,1} ∪ overlap {2} ∪ inherited overlap {4,7} = {0,1,2,4,7} C12 = branch {2,3} ∪ overlap {1} ∪ inherited overlap {4,7} = {1,2,3,4,7} C13 = branch {4,5} ∪ overlap {6} ∪ inherited overlap {0,3} = {0,3,4,5,6} C14 = branch {6,7} ∪ overlap {5} ∪ inherited overlap {0,3} = {0,3,5,6,7} ``` **With this PR**, `7` follows anchor `0` and `4` follows anchor `3` into distinct sub-clusters, keeping each leaf at 4 images. - **leaves**: ` {0,1,2,7} {1,2,3,4} {3,4,5,6} {0,5,6,7} ` ``` level 2 (this PR): inherited overlap is assigned through its anchor C11 = branch {0,1,7} ∪ overlap {2} = {0,1,2,7} C12 = branch {2,3,4} ∪ overlap {1} = {1,2,3,4} C13 = branch {3,4,5} ∪ overlap {6} = {3,4,5,6} C14 = branch {0,6,7} ∪ overlap {5} = {0,5,6,7} ``` --------- Co-authored-by: Johannes Schönberger <jsch@meta.com>
…egression) (colmap#4634) Restore the row-by-row copy used up to commit 7d2058b (the parent of 8e014c5). Correct copy before the regression: https://github.com/colmap/colmap/blob/7d2058baaae1406160b5e0c7ae9b47582ade1b32/src/colmap/mvs/patch_match_cuda.cu#L1578-L1582 Regressed copy introduced by 8e014c5: https://github.com/colmap/colmap/blob/8e014c5bc70c7e01506f1d5ef7daaf25dc3190ae/src/colmap/mvs/patch_match_cuda.cu#L1577 Since commit 8e014c5 (colmap#3459) the bitmaps were copied with a single contiguous memcpy, displacing row r of every image narrower than max_width by r * (max_width - width) pixels. Example for a 2x2 image in a 3-wide layer: ``` bitmap [a b; c d] -> contiguous copy [a b c; d 0 0] (wrong) row-wise copy [a b 0; c d 0] (correct) ``` Workspaces where all source images share the same dimensions are unaffected (the two copies coincide there), which is why the standard single-camera case never showed the problem. Workspaces with mixed image sizes get silently corrupted photometric costs for every source image narrower than the widest one.
In accordance with a brief discussion with @ahojnnes at CVPR, here is a PR for adding the LoMa matcher. For reference on the LoMa matcher, we refer to the paper, [LoMa: Local Feature Matching Revisited](https://arxiv.org/abs/2604.04931), and the code, [https://github.com/davnords/loma](https://github.com/davnords/loma). The PR is basically a copy-paste of how the integration with ALIKED+LG already was made. **Note:** The ONNX files are currently hosted on my own GitHub storage project. Feel free to download them and add them as a release to colmap to align the download links with where the other weights are hosted. This PR is restricted to LoMa-B. I think this will benefit the community the most as the matcher is quick (same speed as LG) and lightweight while the detector / descriptor are a little slower but the full pipeline is very powerful (see below). Happy to fix anything that would improve the integration. ## Tests I used it for reconstruction on South Building (128 images) and it looked good. I also made sure the feature matches looked decent on a MegaDepth pair. <img width="1093" height="399" alt="Skärmavbild 2026-07-09 kl 14 57 30" src="https://github.com/user-attachments/assets/bae76061-e9e9-4219-b614-cb0385d7e9ba" /> To provide some background on the strength of LoMa: In an HLoc [fork](cvg/Hierarchical-Localization#495) we run Visual Localization on InLoc and get: ``` DUC1: 55.1 / 80.3 / 91.4 DUC2: 72.5 / 87.8 / 88.5 ``` This is similar to the results reported in the [paper](https://arxiv.org/abs/2604.04931) and an almost 20-point improvement on DUC2 (0.25m, 10°) compared to what is reported in the [LightGlue](https://arxiv.org/abs/2306.13643) paper. ## Acknowledgements Credit to [aliejabbari](https://github.com/aliejabbari) for providing the code for the ONNX export. / David --------- Co-authored-by: David Nordström <davnords@arrhenius1.hpc.arrhenius.naiss.se> Co-authored-by: Ali Jabbari <alijabbari.contact@gmail.com> Co-authored-by: Johannes Schönberger <jsch@meta.com> Co-authored-by: Johannes Schönberger <jsch@demuc.de> Co-authored-by: Shaohui Liu <b1ueber2y@gmail.com>
## Summary Global SfM keeps only the largest connected component of the view graph, so everything else in a disconnected capture is discarded. This PR reconstructs every component instead, producing one model per component. The process has three steps: 1. Extract the initial connected components of the view graph. 2. Run rotation averaging independently on each of them, on an isolated copy of the base reconstruction so that rig calibration cannot leak across components. This is what settles the final components: filtering edges by relative rotation error breaks apart clusters that were only held together by outlier edges, so one input component can yield several genuine ones. Doing the split here rather than on the raw pose graph matters because no later stage discards images. 3. Run the full global mapper on each final component, over its own `DatabaseCache` restricted to that component's images. The resulting models are sorted by registered-frame count, and those with fewer than `min_num_frames` frames are dropped. ## Options - `GlobalMapper.reconstruct_all_components` (default `true`): reconstruct every component versus only the largest one. - `GlobalMapper.min_num_frames` (default `3`): discard models with fewer registered frames. Both are exposed through `OptionManager` and pycolmap's `GlobalMapperOptions`. ## Behavior changes - Multi-component reconstruction is on by default, so runs on disconnected datasets now emit several models instead of one. - `min_num_frames` also applies to the single-component path, where any successful model was previously kept. - The reconstruction is no longer added to the `ReconstructionManager` before the mapper starts, so the GUI no longer renders intermediate states during global mapping; models appear once complete. ## Benchmark: IMC2025, all 13 scenes No covisibility filtering, same feature/match databases, `--random_seed 0 --num_threads 1`, measured at `b647561c`. `global (main)` and `global (single)` (`--reconstruct_all_components 0`) come out bit-identical, confirming the largest-component path is unchanged. Pooled over all scenes: | method | AUC@0.5° | AUC@1° | AUC@5° | AUC@10° | reg imgs / 1823 | #comp | |----------------|----------|--------|--------|---------|-----------------|-------| | incremental | 8.52 | 17.01 | 36.25 | 41.67 | 1345 | 34 | | global (main) | 7.45 | 11.53 | 19.22 | 21.14 | 634 | 12 | | global (multi) | 13.85 | 23.15 | 40.33 | 44.33 | 1544 | 68 | Per-scene average (mAA): | method | AUC@0.5° | AUC@1° | AUC@5° | AUC@10° | |----------------|----------|--------|--------|---------| | incremental | 10.80 | 21.24 | 43.00 | 48.55 | | global (main) | 7.71 | 12.59 | 22.78 | 25.04 | | global (multi) | 14.60 | 25.21 | 44.74 | 48.91 | Registered images more than double (634 to 1544 of 1823) and overall AUC@10° goes from 21.14 to 44.33, slightly ahead of the incremental mapper. Full per-scene tables are in [this comment](colmap#4589 (comment)). ## Test plan New end-to-end tests in `controllers/global_pipeline_test.cc`: - `MultiComponents`: two rigs with all cross-rig matches removed yield one reconstruction per component, each matching its ground-truth subset. - `MultiComponentsWithOutlierEdges`: two clusters bridged only by outlier edges are still recovered separately, since rotation averaging leaves large residuals on the bridges and filters them out. - `MultiComponentsWithOutlierEdgesUsingGravity`: even when every cross-cluster edge is an outlier, which the gravity-free solver cannot resolve because the inter-cluster gauge is free, gravity priors make the bridges detectable. - `MultiComponentsEmptyViewGraph`: a database with no matches produces no reconstructions. New tests in `scene/pose_graph_test.cc` cover the component ordering and the edgeless case. - [x] `ctest -R "controllers/global_pipeline_test|scene/pose_graph_test"` --------- Co-authored-by: Johannes Schönberger <jsch@meta.com> Co-authored-by: Johannes Schönberger <jsch@demuc.de>
Follow-up to review feedback in colmap#4641. ## Summary - restore the camera-ray overloads of pycolmap.estimate_relative_pose and pycolmap.refine_relative_pose - attach identity Jacobians to preserve the legacy Sampson-error normalization - add regression coverage for both compatibility overloads ## Testing - PYENV_VERSION=colmap scripts/format/c++.sh - PYENV_VERSION=colmap scripts/format/python.sh - PYENV_VERSION=colmap python -m pytest -q src/pycolmap/estimators/pose_test.py
## Summary - honor the requested Bitmap::Rescale filter - use an explicit antialiased triangle filter for bilinear resizing instead of OpenImageIO's expensive default Lanczos filter - explicitly retain box filtering where requested - add a regression test verifying that the two filter modes differ This addresses the severe slowdown when feature extraction downsizes images reported in colmap#4638. It does not claim to resolve the separate no-resize CUDA slowdown discussed there. ## Performance Using 30 copies of the 1920x1345 image from colmap#4638, resized to 960px with CPU SIFT configured to detect no features: - before: 8.08 s - after: 3.11 s - COLMAP 3.13.0: 2.13 s A focused resize microbenchmark measured OpenImageIO's default downsampling at about 234 ms per image and explicit triangle-filtered downsampling at about 52 ms per image. The triangle filter preserves low-pass filtering for downsampling, unlike ImageBufAlgo::resample. ## Testing - PYENV_VERSION=colmap scripts/format/c++.sh - ctest --test-dir build --output-on-failure -R sensor/bitmap_test\|controllers/feature_extraction_test
…colmap#4640) colmap#4420 adds ROCm, HIP support to colmap. But on an attempt of starting the stereo step, the dense reconstruction gui gives an error by only checking against the presence of CUDA, even if the user has built it with HIP. This commit fixes that. If user has HIP enabled, but not CUDA, the process can start.
Fixes colmap#4649. Pose-prior residuals could reintroduce fixed rig transforms as variable Ceres parameter blocks. Preserve their constant parameterization and add regression coverage. Test: bundle_adjustment_ceres_test
## Summary Consolidates backend-independent bundle adjustment tests into the shared interface test suite. - Parameterizes shared tests across Ceres and Caspar when Caspar is enabled - Moves common coverage for camera/point constraints, partial tracks, track-length filtering, intrinsics, and ignored points - Removes duplicated tests from the Ceres and Caspar implementation suites - Keeps backend-specific behavior and solver-internal tests in their respective suites - Fixes coverage where the previous Caspar constant-camera test accidentally constructed a Ceres adjuster ## Verification - `estimators/bundle_adjustment_test` - `estimators/bundle_adjustment_ceres_test` - Caspar-enabled compilation of all three affected test translation units - Formatting and `git diff --check`
## Summary - run the full configured pytest suite for Windows pycolmap wheels - execute pytest from the repository root so it uses the shared `pyproject.toml` configuration - preserve the Windows-specific Visual Studio and vcpkg environment setup - make native command failures terminate the PowerShell test script This closes the coverage gap identified while reviewing colmap#4648: Windows previously only imported `pycolmap`, so Python regression tests did not run on the platform affected by colmap#3798. ## Validation - `PYENV_VERSION=colmap python -m pytest` (948 passed) - `git diff --check`
## Summary - isolate CUDA and non-CUDA compiler caches - make ccache reusable across cibuildwheel temporary build directories - compile the macOS prerequisite build without sudo so it uses the configured cache - cap cache size and report fresh per-run statistics after wheel compilation ## Motivation Recent main builds restored compiler caches, but macOS recorded no hits and the Linux CUDA job could not save its updates because it shared a key with the non-CUDA job. The existing statistics also ran before the wheel builds and therefore did not show whether pycolmap compilation itself benefited. ## Validation - parsed build-pycolmap.yml with PyYAML - ran bash syntax checks on the modified Linux and macOS scripts - verified the new ccache environment settings with ccache --show-config - ran git diff --check
## Summary - replace per-run compiler cache keys with weekly immutable generations - restore caches in every build, but save only from trusted main/release runs - centralize weekly restore/save behavior in a reusable `compiler-cache` composite action - cap each configuration according to its observed working set - prune stale PyCOLMAP ccache entries and recompress retained entries at zstd level 6 before the weekly save - remove cross-run FetchContent caches, which consumed roughly 4 GB of the repository quota by themselves - preserve stable ccache path/compiler hashing and per-run statistics ## Motivation The repository had 11.06 GB of active GitHub Actions caches, above the standard 10 GiB limit. Run-specific compiler archives and PR-scoped copies were evicting recently saved main caches. The latest main build restored only 1 of 10 native compiler caches even though available caches produced approximately 99.3–99.9% hits. Pull requests now restore shared main caches read-only. One trusted build seeds each configuration for the current UTC week; subsequent builds restore the exact weekly key without producing more archives. At the next weekly epoch, the previous generation is used as a fallback and the refreshed generation is saved once. Before saving a new PyCOLMAP generation, ccache entries not used during the current full wheel build are evicted. The remaining entries are recompressed from ccache's default zstd level 1 to level 6. This work runs only on the first successful trusted build that creates a weekly cache. ## Cache budget Configured compiler-cache limits are based on the observed working sets: - native Docker/macOS: 100 MB each - native Ubuntu: 100–150 MB per configuration - native Windows: 200–300 MB per configuration - PyCOLMAP: 350 MB–1 GB per configuration One complete generation is capped at approximately 4.4 GB. During the seven-day overlap between weekly generations, the projected peak before save-time compaction is approximately 8.8–9.1 GB including cached compiler tools, leaving limited headroom below the 10 GiB repository quota. Existing FetchContent entries will age out because the workflows no longer restore them. ## Validation - parsed all workflow and composite-action YAML - verified only the shared composite action invokes `actions/cache/restore` and `actions/cache/save` - verified pull requests and manual PyCOLMAP runs cannot save compiler caches - ran an isolated save/restore smoke test: https://github.com/colmap/colmap/actions/runs/33055245598 - the reusable action evicted the deliberately stale entry - the retained entry survived recompression and archive restore as a cache hit - the stale entry produced the expected cache miss - `git diff --check` CI exercises restore behavior on all hosted runner platforms. The first trusted main run will seed the weekly keys; a subsequent run will demonstrate warm-cache hit rates.
## Summary - use Bash to compact ccache on Linux and macOS - retain PowerShell compaction on Windows - avoid requiring pwsh after the CUDA job frees disk space ## Context The PyCOLMAP CUDA job in https://github.com/colmap/colmap/actions/runs/33075656647 built and archived its wheels successfully, then failed while saving the compiler cache because pwsh was no longer available. ## Validation - actionlint passes for all workflows - all GitHub YAML files parse successfully - the Bash compaction sequence passes against an isolated ccache directory - git diff --check passes
## Summary - add `SequentialMatching.loop_detection_min_image_distance` to exclude nearby images from loop retrieval - apply candidate filtering before top-N selection so nearby frames do not consume the loop-detection budget - expose the option through the CLI, GUI, and PyCOLMAP and document its behavior The option defaults to zero to preserve existing behavior. ## Verification - `PYENV_VERSION=colmap scripts/format/c++.sh` - `ctest --output-on-failure -R "controllers/pairing_test|retrieval/visual_index_test"` - built `colmap_ui` and `colmap_main` - built the PyCOLMAP `_core` extension and verified the new property
Benchmarking results in the following.
## Summary - Add process-wide cooperative handling for SIGINT and SIGTERM, with a second signal forcing immediate termination. - Propagate cancellation through feature extraction/matching, incremental mapping, bundle adjustment, point triangulation, image registration, image undistortion/rectification, patch-match stereo, and stereo fusion. - Support graceful shutdown in `automatic_reconstructor` when using its incremental mapper; global and hierarchical mapper modes remain excluded. - Preserve and write usable in-progress reconstruction or fusion results before returning. - Expose a reusable `pycolmap.CancellationToken` across supported Python pipeline entry points and raise `InterruptedError` only after cleanup. - Keep guided geometric-verification database updates consistent across cancellation. - Document CLI and Python shutdown behavior. Global mapping is intentionally not included because its intermediate state cannot currently be resumed safely. ## Testing - `PYENV_VERSION=colmap scripts/format/c++.sh` - `PYENV_VERSION=colmap scripts/format/python.sh` - Targeted C++ build for the CLI and affected test targets. - Seven affected C++ test suites pass: - `controllers/bundle_adjustment_test` - `controllers/undistorters_test` - `estimators/global_positioning_test` - `exe/sfm_test` - `mvs/fusion_test` - `sfm/global_mapper_test` - `util/cancellation_test` - pycolmap shared-module build and stub generation. - `src/pycolmap/util/cancellation_test.py`: 7 passed. The full `install` build remains blocked by the pre-existing `util/cache_test` linker error for `testing::internal::GetWithoutMatchers()`; the successfully built libraries were installed directly for pycolmap verification. The implementation and follow-up fixes were independently reviewed with Claude Code.
## Summary - add draft release notes for COLMAP 4.2.0 - highlight multi-component global mapping, LoMa, ROCm/HIP PatchMatch, the browser viewer, and mapper API expansion - document performance improvements, bug fixes, and migration-relevant breaking changes ## Testing - `git diff --check` - `PYENV_VERSION=colmap make -C doc html SPHINXOPTS="-W --keep-going"`
## Summary - require complete function annotations and check all typed function bodies with mypy - annotate every tracked Python function, including tests, benchmarks, docs, examples, and the Caspar generator - include documentation and the Caspar generator in CI type checking - exempt generated pybind11 stubs from source annotation enforcement and narrowly suppress generated-binding diagnostics ## Testing - `PYENV_VERSION=colmap scripts/format/python.sh` - exact CI mypy commands against an installed wheel - `PYENV_VERSION=colmap pytest` (955 passed) - `PYENV_VERSION=colmap make -C doc html SPHINXOPTS="-W --keep-going"` - AST audit confirming no executable behavior changes beyond annotations
Fixes colmap#4672 FlatHashMap/FlatHashSet and their node variants are data members of classes in public headers, so COLMAP_HASH_MAP_BACKEND determines the layout of Reconstruction, CorrespondenceGraph, BundleAdjustmentConfig and others: sizeof(Reconstruction) is 320 bytes with STD and 280 with BOOST. Both backends are header-only, so a mismatch changes no mangled name and produces no link error, only memory corruption. The backend was auto-selected from the Boost found at configure time, which made that layout a property of the build machine rather than of the source and the CMake arguments. The published manylinux pycolmap wheels are built through vcpkg and got BOOST; an apt-based Ubuntu 24.04 build of the same commit gets STD. An extension linking its own COLMAP and sharing bound types with pycolmap in one interpreter then reads those objects at the wrong offsets, since pybind11 keys registration on type_index, which is identical for both layouts. Default to STD instead, and keep the old behavior available as an explicit AUTO that warns. BOOST stays an opt-in that has to be applied to everything in the process. colmap-config.cmake now pre-sets the value COLMAP was built with, so find_package() consumers reproduce that choice instead of re-deriving it from their own Boost, turning a mismatch into a configure-time error. The choice is also recorded in the binary as colmap::kHashMapBackend, reported by GetBuildInfo(), and exposed as pycolmap.__hash_map_backend__ for downstreams to assert on.
…colmap#4713) Replace the 28-line verbose BSD license header in all source files with a single `SPDX-License-Identifier: BSD-3-Clause` line, using the comment style of each language (//, #, %, rem), and add it to the ~230 source files that were missing a header. Also rename the license text to the conventional LICENSE filename and update the Qt resource embedding it (resources.qrc + MainWindow::License). Third-party code is untouched: src/thirdparty keeps its own licenses, and the Google-derived headers in util/glog_macros.h, optim/tiny_solver.h, and util/string.cc retain their attribution. Net effect: -17k lines of boilerplate. Stack: PR 1/3, followed by colmap#4714 and colmap#4716.
Stacked on colmap#4713 (PR 2/3, followed by colmap#4716). List ETH Zurich and UNC Chapel Hill as the 2016 copyright holders and The COLMAP Contributors for 2016-2026 in LICENSE, README.md, and doc/license.rst.
…#4716) Stacked on colmap#4714 (3/3). Instruct contributors and agents to start every new source file with the one-line SPDX header, in the file's comment style.
Remove the unmaintained Matlab utilities in `scripts/matlab` (model/depth-map/normal-map/PLY readers, writers, and plotting helpers) and update `doc/faq.rst` and `doc/format.rst` to point to pycolmap instead. Remaining case-insensitive mentions of "matlab" in the tree are only provenance comments in C++ sources (e.g. reference values generated with Matlab) and vendored VLFeat docs; no Matlab code remains.
## Summary When `os.cpu_count()` returns 1, panorama perspective rendering subtracts one reserved CPU and passes `max_workers=0` to `ThreadPoolExecutor`. This raises `ValueError: max_workers must be greater than 0` before any panorama is processed. Keep at least one rendering worker. The existing unknown-CPU fallback, reservation of one CPU on multicore hosts, and 32-worker cap remain unchanged. Add a regression test for reported CPU counts of 1, `None`, 4, and 64. It uses the real executor and verifies that every submitted panorama is processed, replacing only the image-processing work to avoid optional dependencies. ## Validation - Before the fix: the single-CPU regression raises the reported exception; the three comparison cases pass. - After the fix: all six tests in `python/pycolmap/panorama_test.py` pass. - A separate smoke test exercised actual rendering with `cpu_count=1`, two synthetic 16x8 panoramas, Pillow/OpenCV, and the native PyCOLMAP bitmap writer. It produced two rendered images and two masks, all 4x4. - Ruff check, Ruff format check, and `git diff --check` pass. The Python tests used this checkout's source with the native PyCOLMAP 4.2.0 Windows wheel (Python 3.13.5). COLMAP's C++ code and the full reconstruction pipeline were not built or tested. AI assistance: OpenAI Codex identified the edge case, implemented the fix, and ran the regression tests. --------- Co-authored-by: Paul-Edouard Sarlin <15985472+sarlinpe@users.noreply.github.com> Co-authored-by: Johannes Schönberger <jsch@meta.com>
Bump to the latest pybind11 release (3.1.0) and adopt two of its new features. Version bump: - `pyproject.toml`: build pin `3.0.4` -> `3.1.0` - `python/CMakeLists.txt`: `find_package` minimum `3.0.2` -> `3.1.0` Notable pybind11 3.1.0 changes vs 3.0.x: Python 3.8 and MSVC 2017 support dropped (no impact, pycolmap requires >=3.10), PEP 484 strict-mode numeric casts relaxed, plus `stl_bind` slice-deletion, `enum_` shutdown-crash, and `scoped_ostream_redirect` fixes. Feature adoption: - Custom `__str__` on `enum_` (pybind/pybind11#6078): `str()` on enums using `AddStringToEnumConstructor` now returns the plain member name (e.g. `str(Device.auto) == 'auto'`) and round-trips through the string constructor. `repr()` keeps the qualified `Type.MEMBER` form via its own function object (pybind11 chains overloads by mutating the shared record, so the old `__repr__ = __str__` alias would have picked up the custom `__str__` too). - Explicit `py::mod_gil_used()` module declaration (new 3.1.0 spelling for the default): documents that `_core` requires the GIL; no behavior change. Verified locally: full C++ rebuild + install, pycolmap incremental rebuild against pybind11 3.1.0 (incl. stub generation), and the full pytest suite (962 passed, incl. 2 new enum str/repr tests).
…che (colmap#4702) Stacked on colmap#4696. Makes `min_inlier_ratio` rejections effective end to end: rejected pairs are stored empty and skipped on load, instead of flowing into the mappers behind a DEGENERATE label. ## Producer: matcher rejects UNDEFINED estimates; skips are not stored The ratio recheck rejects pairs as DEGENERATE while keeping their inlier matches and models, and the verifier stored those rows verbatim (both `Match` and `Verify` output stages). Since downstream loaders only check the inlier count, rejected pairs silently fed the mappers. Now: - The verifier worker throws if a two-view estimator returns UNDEFINED, so UNDEFINED at the output stage provably means the pair was skipped (below-minimum matches or missing descriptors), never estimated. (Audited all estimator exits: uncalibrated, calibrated, spherical, shared/one-sided-focal, force-H, known-pose, multiple-models, rig — none can return UNDEFINED.) - Skipped (UNDEFINED) pairs are not stored at all: absence of a row means never verified. `Verify` still deletes any pre-existing row first, so re-verification self-heals stale rows. - Rejected (DEGENERATE) pairs keep their diagnosis with the payload (inlier matches, E/F/H, pose, cameras) cleared wholesale. The old inlier-count reset is removed: every estimator exit labels below-minimum results DEGENERATE, so clearing on the label alone is equivalent. The database interface itself stays permissive (UNDEFINED rows remain writable and legible for fixtures and legacy DBs); the contract is enforced at the matcher and at load. ## Consumer: cache skips UNDEFINED/DEGENERATE pairs and warns `DatabaseCache` rejects UNDEFINED/DEGENERATE pairs at load regardless of stored inlier matches (single `UseInlierMatchesCheck` choke point), which also repairs databases written before the producer-side fix. Additionally: - Load counts UNDEFINED rows and `LOG(WARNING)`s the count (outdated-producer rows; clear and re-verify to remove them). Note: truly-empty rows never reach the cache — `ReadTwoViewGeometries` already filters them — so the warning covers payload-carrying rows. - `CreateFromCache` now applies the same check when copying graph edges (previously verbatim), not just for image connectivity. ## Behavior-change notes - Incremental mapper: rejected pairs (previously loaded) are now excluded. Courtyard @ 0.25 re-verified with the final binary: 691 rows, 474 live pairs bit-identical to before, 217 DEGENERATE (all empty, incl. 210 previously stored as UNDEFINED), 0 UNDEFINED rows. - `skip_geometric_verification` and skipped pairs no longer leave UNDEFINED marker rows, so re-runs re-attempt them instead of skipping via the marker. - Global pipeline: view-graph calibration already marks inconsistent pairs DEGENERATE; those rows were previously still loaded and are now excluded too. No dedicated global-pipeline benchmark was run. - Rig estimator (returns no entries on rejection), guided verifier (funnels through the fixed output stage), synthetic fixtures (CALIBRATED/UNCALIBRATED only) are unaffected. ## Follow-ups (not in this PR) - `FeaturePairs` matcher and rig verification write estimator output verbatim without the worker assertion; the silent-drop hazard doesn't apply there (no skip logic), but the same assertion could be added for uniformity. - View-graph calibration writes re-estimated output and DEGENERATE markings verbatim; the consumer filter covers mapper ingestion, but stored rows keep stale inliers. ## Test plan - Existing `MatchClearsDegenerateGeometry` / `VerifyClearsDegenerateGeometry` (DEGENERATE-empty, raw matches preserved) still pass. - New `MatchSkippedPairsStoreNoTwoViewGeometry` / `VerifySkippedPairsStoreNoTwoViewGeometry`: skipped pairs store no TVG row. - `MatchSkipGeometricVerification` updated to the new contract (no row stored). - New `DatabaseCache.CreateFromCacheSkipsUndefinedGeometries`: UNDEFINED edges excluded (image dropped when isolated, edge dropped when endpoints survive); existing `SkipsUndefinedAndDegenerateGeometries` passes unchanged. - `controllers/` + `scene/`: 37/38 pass; the single failure (`scene/reconstruction_test` AddFrame/AddImage exception-text mismatch) is pre-existing on the base and unrelated (untouched `reconstruction.cc` code path; this branch only touches `database_cache.cc` in `scene/`). - E2E: legacy DB with 3 payload-carrying UNDEFINED rows makes the mapper log `3 two-view geometries with UNDEFINED configuration ...`; courtyard re-verify (above) shows identical live pairs with no estimator check firing. - `clang-format` clean on all touched files.
## Summary - Change the `compiler-cache` epoch from ISO week (`%G-%V`) to UTC day (`%F`), so changed objects are incorporated within a day instead of a week. - Add a post-save prune step that deletes superseded generations per family on the current ref, keeping the newest two (new `retain-generations` input, default `"2"`). Quota footprint stays at ~2 generations alive, same as the weekly scheme. - Restrict cache saves to `main` pushes and `release` events; `release/*` pushes, PRs, and manual runs restore read-only. This preserves the Docker `docker-release_true` family, which is only seeded by releases. - Grant the five calling build jobs `contents: read` + `actions: write` (pruning uses `gh cache list`/`delete` via `GITHUB_TOKEN`). ## Motivation After large refactors land mid-week, every CI run restores the pre-change weekly snapshot and recompiles the same new objects until the next weekly rotation. Daily rotation bounds that staleness to a day. Explicit pruning (rather than relying on GitHub's 7-day idle expiry) keeps the faster rotation within the 10 GiB quota: without it, ~7 overlapping daily generations would exceed quota and trigger cross-family LRU eviction. ## Test Plan - E2E self-test of the new action on this branch against an isolated `colmap-compiler-selftest-*` family (temporary workflow, removed before opening this PR; all test archives deleted afterwards): - run [34756730823](https://github.com/colmap/colmap/actions/runs/34756730823): confirmed save+prune correctly skip on a non-`main` push (Option B gate). - run [34756772404](https://github.com/colmap/colmap/actions/runs/34756772404) (with a temporarily widened gate): `Cache saved with key: ...-2000-01-03`, `Deleting superseded cache id ...`, verify confirmed exactly the two newest generations remain — success. - Prune-selection `jq` program tested against fixtures (keep-2, keep-1, single/empty/foreign-only listings) — 5/5 pass. - Same selection pipeline dry-run read-only against live production families: correct no-op on a 2-generation family. - All touched workflow/action YAML parsed; extracted prune script passes `bash -n`; `git diff --check` clean. - This PR's own CI exercises the restore path (daily key miss with fallback to the previous weekly archives) on all platforms.
This updates poselib to the latest commit improving build compatibility with newer versions of eigen. This was tested on Ubuntu 24.04 with Eigen 5.0.1. Co-authored-by: jmackay2 <jmackay2@gmail.com> Co-authored-by: Shaohui Liu <b1ueber2y@gmail.com>
…ap#4721) Add `SolveEpipolarConstraintMatrix` and `RaysFromCamRaysWithJac` to a new solvers/utils library. The former replaces two verbatim copies of the QR/SVD nullspace plus rank-2 enforcement in the eight-point fundamental and essential estimators; the latter replaces the file-local `UnpackCamRaysWithJac` helper. Part 2/5 of the solver-simplify stack (stacked on colmap#4720). Test Plan: - estimators_solvers/utils_test (new) - estimators_solvers/essential_matrix_test - estimators_solvers/fundamental_matrix_test
colmap#4722) Add `SolveHomographyFromConstraintMatrix` covering the minimal LU path and the over-determined SVD path, pinning the shared 1e-8 singularity threshold. Covered by the existing parameterized estimator tests. Part 3/5 of the solver-simplify stack (stacked on colmap#4721). Test Plan: - estimators_solvers/homography_matrix_test
…4723) Extend solvers/utils with `CalibratedRays` (replacing two verbatim copies), plus `PoseParamsFromRigid3d` and `Rigid3dFromPoseParams` for the 7-parameter TinySolver packing used by the essential and focal refine drivers. Part 4/5 of the solver-simplify stack (stacked on colmap#4722). Test Plan: - estimators_solvers/utils_test (extended) - estimators_cost_functions/sampson_error_test (incl. new round-trip test) - estimators_solvers/essential_matrix_test - estimators_solvers/relpose_one_sided_focal_test - estimators_solvers/relpose_shared_focal_test
Introduces shared HWC->CHW image conversion in feature/utils and uses it for the ALIKED input tensor and the LoMa detector and descriptor inputs. No behavior change. Independent PR (no dependencies). Test plan: `ctest -R 'feature/(utils_test|aliked_test|loma_test)'` passes.
…ors (colmap#4727) Add `ComputeImgReprojError` and use it in the four autodiff reprojection functors, replacing four verbatim copies of the project / subtract-observation / seam-wrap / zero-on-failure epilogue. Adds an analytical behind-camera zeroing test. Part 5/5 of the solver-simplify stack (stacked on colmap#4723). Test Plan: - estimators_cost_functions/reprojection_error_test
Removes `colmap::Clamp` from `math.h` (including its unit test) and converts all call sites plus all other nested `std::min`/`std::max` clamping patterns across the codebase to `std::clamp`. Converted (16 sites in 11 files): `image/undistortion`, `mvs/mesh_simplification`, `mvs/depth_map`, `mvs/texture_mapping`, `scene/reconstruction_pruning`, `scene/visibility_pyramid`, `retrieval/vote_and_verify`, `estimators/gravity_refinement`, `ui/colormaps`, `ui/model_viewer_widget`, and `math.h::TruncateCast`. Deliberately left alone: - `estimators/fundamental_matrix_degensac.cc`: mixes signed `int` and `size_t`; the inner `max(..., 1)` runs in the signed domain, so a naive `std::clamp<size_t>` would change behavior for negative inputs. - `estimators/covariance.cc`: `max(max(...), ...)` is two lower bounds, not a clamp. No behavior change; `std::clamp` generates identical codegen to the handcrafted nesting. Verified all converted sites have ordered bounds (`lo <= hi` is a `std::clamp` precondition). Test plan: built `colmap_ui`, `colmap_retrieval`, `colmap_estimators`, `colmap_mvs`, `colmap_math`; `math/math_test` and `retrieval/vote_and_verify_test` pass; files formatted with `scripts/format/c++.sh`.
Stack 1/2. Introduces CreateONNXTensor/CreateONNXScalarTensor in onnx_utils and uses them in the ALIKED extractor, LoMa extractor/matcher, and ONNX brute-force/LightGlue matchers. No behavior change. Next: colmap#4750 Test plan: `ctest -R 'feature/(onnx_utils_test|onnx_matchers_test|aliked_test|loma_test)'` passes.
Stack 2/2. Depends on colmap#4745. Introduces ImageFeatureCache in feature/utils, a per-image cache retaining the two most recently used entries, and uses it in the SIFT CPU matcher and the ONNX brute-force/LightGlue matchers, replacing hand-rolled per-image caching. Also drops the unused descriptor index prefetch in guided SIFT matching. No behavior change. Test plan: `ctest -R 'feature/(utils_test|sift_test|onnx_utils_test|onnx_matchers_test|aliked_test|loma_test)'` passes.
## Motivation Adding constraints to COLMAP's global-positioning Ceres problem currently requires hacking its implementation because `RunGlobalPositioning(...)` constructs, solves, and publishes the problem in one call. Following `CreateDefaultBundleAdjuster(...)`, this PR adds `GlobalPositioner::CreateDefault(...)`, which returns the prepared standard problem so callers can extend it before solving. `RunGlobalPositioning(...)` remains the stock one-shot path with unchanged behavior. ## Changes - Add `GlobalPositioner::CreateDefault(...)` and a prepared `GlobalPositioner` lifecycle. - Expose the prepared Ceres problem, temporary frame centers, and parameter-ordering extension. - Add an optional caller-provided loss. - Expose the construction and lifecycle workflow through PyCOLMAP. ## Why global positioning needs additional seams The bundle-adjustment workflow can expose a prepared problem through parameter blocks that are already externally addressable. Global positioning also owns temporary centers and observation scales internally, so its prepared problem needs additional seams. <details> <summary>[Click to expand for details]</summary> - **Temporary frame centers.** The solver does not optimize the pose translations stored in `Reconstruction` directly. It copies poses into temporary world-frame centers and converts the optimized centers back during finalization. `FrameCenterParameterBlock(frame_id)` exposes this temporary parameter to code adding constraints to the prepared Ceres problem. `Finalize()` performs the required publication back to `Reconstruction`. - **Internal observation scales.** BATA observation scales are stored inside `GlobalPositioner` rather than `Reconstruction`. - **Late parameter blocks.** Stock parameter ordering is created during construction. `ExtendParameterBlockOrdering()` adds caller-created blocks without rebuilding existing assignments and supports explicit group overrides. </details> <details> <summary>[Click to expand for a possible future refactor]</summary> A deeper refactor could eliminate the two global-positioning-specific state seams: - Optimize the translation blocks in reconstruction-owned `Rigid3d` poses instead of copied frame centers. - Make observation scales explicit input and output. `GlobalPositioner::CreateDefault(...)` could consume a scale map keyed by observation, allowing callers to provide initial values and inspect the optimized results instead of relying on hidden owner storage. </details> --------- Co-authored-by: Johannes Schönberger <jsch@demuc.de>
Follow-up to the investigation in colmap#4691: PR colmap#4304 made the inner refinement BAs inherit the full outer solver recipe (robust loss, tight tolerances), diverging from the cheap legacy/incremental recipe for the same shared refinement routine. This change: - Derives inner refinement BA options from the outer options, inheriting only variable-selection scope while restoring the cheap legacy recipe (TRIVIAL loss, gradient-only tolerance, capped iterations, no minimum track length). Solver/GPU selection stays inherited. - Restores the legacy track establishment cap (`track_max_num_views_per_track=100`, exposed via CLI/GUI/pycolmap). - Restores legacy outer BA tolerances (gradient 1e-10, parameter 1e-8). - Runs a second outer BA after re-filtering in retriangulation, as legacy did. Deliberately not restored: BA rig refinement stays enabled by default, since current rotation averaging preserves calibrated rigs (unlike legacy, which overwrote them) and fixing the rig in BA demonstrably breaks rig recovery (`GlobalMapper.WithoutNoiseWithNonTrivialKnownRig`). Test plan: - `global_mapper_test` 5/5, `global_pipeline_test` 13/13, `option_manager_test` 14/14 pass. - Formatted with `scripts/format/c++.sh`.
…map#4762) Fixes colmap#4761. pybind11 enables LTO for the `_core` module by default, but the Boost that COLMAP builds from source (when the system Boost is older than 1.84, e.g. Ubuntu 24.04) is installed as non-LTO static archives, and with GCC that mix fails at link time with `undefined reference to boost::bad_weak_ptr::what() const` from `libboost_thread.a`. This sets `CMAKE_INTERPROCEDURAL_OPTIMIZATION` to `OFF` for pycolmap only when compiling with GCC against a COLMAP that uses the pinned Boost, which matches the root build. Tested on Ubuntu 24.04 with `python -m pip install .` against an installed COLMAP. --------- Co-authored-by: Johannes Schönberger <jsch@meta.com>
Bring the branch up to current main (7019dcc) with a merge rather than a rebase, so the commits already published on this pull request keep their hashes and the review history stays attached to them. The resulting tree is identical to the maintainer's rebase of the same commits onto that main. Two files conflicted and both take the maintainer's resolution: - src/pycolmap/pipeline/mvs.cc: main restructured this file after the branch was cut and already carries the CUDA-or-HIP guard, so main's version is taken unchanged. - src/colmap/util/opengl_utils_test.cc: main's version, with the QApplication include and the existing tests kept under COLMAP_GUI_ENABLED and the RunsThreadBody test that checks RunThreadWithOpenGLContext runs the thread body in builds without an OpenGL context. This merge was prepared with assistance from an AI coding agent. Test Plan: ``` git diff HEAD 02bece1 # empty: same tree as the maintainer's rebase cmake -S . -B build-hip-gui -GNinja -DCUDA_ENABLED=OFF -DHIP_ENABLED=ON \ -DCMAKE_HIP_ARCHITECTURES=gfx90a -DCMAKE_BUILD_TYPE=Release \ -DTESTS_ENABLED=ON -DGUI_ENABLED=ON -DCGAL_ENABLED=OFF \ -DDOWNLOAD_ENABLED=OFF -DONNX_ENABLED=OFF cmake --build build-hip-gui xvfb-run -a build-hip-gui/src/colmap/feature/sift_test xvfb-run -a build-hip-gui/src/colmap/util/opengl_utils_test build-hip-gui/src/colmap/mvs/gpu_mat_test ```
- Mention HIP in the GPU matching Check() error and fix use_gpu typo. - Update stale CUDA-version comments to compute-backend wording. - Drop unused cuda_to_hip.h include from feature_matching_utils.cc. - Clarify ROCM_PATH cache-over-environment precedence in installed config. - Rename CreateSiftGPUMatcherCUDA test to CreateSiftGPUMatcherCompute. - Report missing PBO interop from the CuTexImage ctor on HIP builds.
Collaborator
Author
|
To approve this fix round, leave a comment containing this line by itself: To send it back to the porter instead: Approving covers the commits on this branch and the section under '## Upstream reply' in the body, which is posted verbatim as a comment on the upstream pull request after the merge, behind a standing line disclosing that it was drafted by an AI assistant. |
Collaborator
Author
|
/moat approve |
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
16 participants
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.
Upstream reply
Thanks! Your changes are in, with the resulting tree identical to #4. I merged main into the branch rather than rebasing so the existing review history stays intact, and your commit is included as authored.
Re-tested on AMD GPUs at the new head: MI250X (gfx90a) and Radeon Pro W7800 (gfx1100), full ctest 162/162 on each, with the GPU SIFT tests (32/32) confirmed running on the GPU. The CUDA build still compiles and links cleanly.
Round summary (not posted upstream)
The maintainer reviewed colmap#4635, said it looks good, and opened #4 (base moat-port) rebasing our three commits onto colmap main 7019dcc plus his own nit commit e30393b. moat-port is frozen and never rebased, so this round reproduces his result as a merge.
[ROCm] Merge main into the HIP SIFT branch: merges 7019dcc into the published tip 0af9a2d; the conflicts in opengl_utils_test.cc and pycolmap/pipeline/mvs.cc take the maintainer's resolution. The mvs.cc and dense_reconstruction_widget.cc port hunks are gone because upstream already carries them (Update conditional compilation for CUDA and HIP for dense reconstruct colmap/colmap#4640).Review: passed, no findings. Validation at cfe7336: linux-gfx90a 162/162, linux-gfx1100 162/162 (wave64 and wave32 gates); windows remains under its approved waiver. CUDA 12.8 compile-and-link check clean. Details in projects/colmap/notes.md on port/colmap.
After merge, close #4 noting its content landed tree-identical.