Repository navigation
Conversation
Remove the `CAMERA_MODEL_CASES`, `PERSPECTIVE_CAMERA_MODEL_CASES`, `SPHERICAL_CAMERA_MODEL_CASES`, `PERSPECTIVE_FISHEYE_CAMERA_MODEL_CASES` and `CAMERA_MODEL_SWITCH_CASES` X-macros from `sensor/models/base.h`. Add a type registry and a single runtime dispatch point in `sensor/models/runtime.h`: `CameraModelTag`, the `AllCameraModels`, `PerspectiveCameraModels`, `SphericalCameraModels` and `PerspectiveFisheyeCameraModels` tuples, `internal::DispatchCameraModel` (one explicit switch), `VisitCameraModelOrThrow`, `VisitCameraModelOrValue`, the category-restricted visitors, and the `ForEachCameraModel*` enumerators. Migrate every former expansion site in `runtime.h`, `models.cc`, the reprojection-error cost functions, ray fitting, the UI widgets, the pybind11 camera binding, and the sensor tests to the new dispatchers. The dispatch keeps the explicit switch so the compiler can emit a jump table and inline the selected model function, as the macro expansion did. There is no virtual or indirect call. The `*_DEFINITIONS` macros that declare each model's static members are unchanged. Adding a model now requires the tuple entries, the matching category tuple, and a case in `internal::DispatchCameraModel`, as documented in `sensor/models.h`; the new `CameraModelDispatch` test covers all models through both enumeration and dispatch. Microbenchmarks show parity for dispatch-only and fixed-model cases and for the synthetic bundle-adjustment and incremental-mapping workloads. A synthetic loop that alternates the camera model on every `CameraModelImgFromCam` call measures about 9% slower; see the pull request description for the numbers and the jump-table analysis. Test Plan: - `cmake --build build -j 11` succeeds on the prototype branch. - `ctest -R "^(sensor|estimators|calibration|scene)/"` passes (46 tests) and `ctest -R "^(sfm|controllers)/"` passes (21 tests). - `PYENV_VERSION=colmap scripts/format/c++.sh` passes and `git diff --check` is clean. - The pybind11 camera binding was syntax-checked only; pycolmap was not fully built.
…l directly Rebase resolution: commit 105817b hoisted camera model dispatch per row in image warps using the `CAMERA_MODEL_CASE` / `CAMERA_MODEL_SWITCH_CASES` macros that the previous commit removes. Migrate `CamFromImgRow` and `ImgFromCamRow` in `image/warp.cc` to `VisitCameraModelOrThrow`, keeping each scanline loop inside the visitor lambda so dispatch stays once per row. The helpers are kept (they are shared by both interpolation instantiations of `WarpBetweenCamerasImpl`); inlining them into the loop was measured and reverted, as it duplicated the dispatch and grew `warp.cc.o` by about 48% with no runtime benefit. CI fix: return the `optional` directly from `VisitCameraModelOrThrow` in `CameraModelImgFromCam`, `CameraModelImgFromCamWithJac`, `CameraModelCamFromImg`, and `CameraModelCamRayFromImg`, building it inside the lambda from a lambda-local `xy` / `uv` / `ray`. This fixes the GCC `-Wmaybe-uninitialized` error (Ubuntu Coverage job) on the `ray` variable in `CameraModelCamRayFromImg`, without zero-initializing the outputs. The ClangTidy job had been cancelled when the Coverage job failed, not failed on its own merits. Warping is bit-identical to the macro baseline and at parity in A/B measurements (median ratios 0.997-1.010 at 2048x1536 over 10 pairs and 0.995-1.017 at 4032x3024 over 6 pairs with 7 repeats). Test Plan: - `cmake --build build -j 11` succeeds on the rebased branch. - `ctest -R "^(sensor|estimators|calibration|scene|sfm|controllers|image)/"` passes (70 tests), including `Warp.CameraModelsMatchPerPixelProjection`. - `PYENV_VERSION=colmap scripts/format/c++.sh` passes and `git diff --check` is clean. - The pybind11 camera binding was syntax-checked only; pycolmap was not fully built.
ahojnnes
force-pushed
the
user/jsch/camera-model-dispatch
branch
from
November 2, 2026 07:45
3464070 to
f40127f
Compare
sarlinpe
requested changes
Nov 4, 2026
Comment on lines
+173
to
+193
| template <typename F> | ||
| void ForEachPerspectiveCameraModel(F&& f) { | ||
| internal::ForEachCameraModelImpl<PerspectiveCameraModels>( | ||
| std::forward<F>(f), | ||
| std::make_index_sequence<std::tuple_size_v<PerspectiveCameraModels>>{}); | ||
| } | ||
|
|
||
| template <typename F> | ||
| void ForEachSphericalCameraModel(F&& f) { | ||
| internal::ForEachCameraModelImpl<SphericalCameraModels>( | ||
| std::forward<F>(f), | ||
| std::make_index_sequence<std::tuple_size_v<SphericalCameraModels>>{}); | ||
| } | ||
|
|
||
| template <typename F> | ||
| void ForEachPerspectiveFisheyeCameraModel(F&& f) { | ||
| internal::ForEachCameraModelImpl<PerspectiveFisheyeCameraModels>( | ||
| std::forward<F>(f), | ||
| std::make_index_sequence< | ||
| std::tuple_size_v<PerspectiveFisheyeCameraModels>>{}); | ||
| } |
Member
There was a problem hiding this comment.
These seem unused, do we really need them?
| } | ||
|
|
||
| template <typename F> | ||
| auto VisitSphericalCameraModel(CameraModelId model_id, F&& f) |
| } | ||
|
|
||
| template <typename F> | ||
| auto VisitPerspectiveFisheyeCameraModel(CameraModelId model_id, F&& f) |
Member
There was a problem hiding this comment.
Only used for models_test.cc, can we remove it? Along with PerspectiveFisheyeCameraModels.
| // must be added here, to the matching category tuple in this registry, | ||
| // and to the switch in `internal::DispatchCameraModel` (see | ||
| // `sensor/models.h`). | ||
| using PerspectiveCameraModels = std::tuple<SimplePinholeCameraModel, |
Member
There was a problem hiding this comment.
Isn't this redundant with CameraModelIsPerspective? can we derive it from the latter?
This branch has not been deployed
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.
Replace camera model X-macros with Visit/ForEach dispatch
Summary
This removes the
CAMERA_MODEL_CASESfamily of X-macros and replaces themwith a typed, single-point runtime dispatch. The
*_DEFINITIONSmacrosthat declare each model's static members are intentionally unchanged.
src/colmap/sensor/models/base.h: deleteCAMERA_MODEL_CASES,PERSPECTIVE_CAMERA_MODEL_CASES,SPHERICAL_CAMERA_MODEL_CASES,PERSPECTIVE_FISHEYE_CAMERA_MODEL_CASES,CAMERA_MODEL_SWITCH_CASES,and
CAMERA_MODEL_DOES_NOT_EXIST_EXCEPTION.src/colmap/sensor/models/runtime.h: addCameraModelTag, theAllCameraModels/PerspectiveCameraModels/SphericalCameraModels/
PerspectiveFisheyeCameraModelstuples,internal::DispatchCameraModel(one explicit
switch),VisitCameraModelOrThrow,VisitCameraModelOrValue, the category-restricted visitors, and theForEachCameraModel*enumerators. Migrate the 11 runtime dispatchfunctions to them.
src/colmap/sensor/models.cc,src/colmap/estimators/cost_functions/reprojection_error.h,src/colmap/calibration/ray_fitting.cc: migrate every remainingswitch/ enumeration site to the new dispatchers.src/colmap/ui/database_management_widget.cc,src/colmap/ui/feature_extraction_widget.cc,src/pycolmap/scene/camera.cc: migrate list/enum enumeration toForEachCameraModel.src/colmap/sensor/models_test.cc: migrate the fisheye helper andthe model-id list, and add
CameraModelDispatch.CoversAllModels.src/colmap/sensor/models.h: update the "add a new camera model"steps to the tuple + switch registry.
Tuple order matches the old
CAMERA_MODEL_CASESorder, so UI lists andpybind11 enum registration order are unchanged. Adding a model now
requires the tuple entries, the matching category tuple, and a case in
internal::DispatchCameraModel; sizestatic_asserts (19 / 18 / 1 / 7)and the new dispatch test guard that registry against drift.
Design notes
Why a tag, not the model directly?
In C++17 a type cannot be a runtime argument, and a generic lambda's
template parameter is unnamed, so the body recovers the type from its
argument with
typename decltype(tag)::type. Passing a model valuedoes not work today: the models are not constructible outside
themselves (e.g.
BaseCameraModelhas a private default constructorwith only the model as
friend, and the same pattern holds for theother CRTP bases). Changing that would relax the
non-instantiability invariant and still leave
std::decay_t<decltype(model)>in bodies that need the type as atemplate argument (
CostFunctor<CameraModel>,is_base_of_vchecks).Direct naming is possible in C++20 with templated lambdas
(
[]<typename CameraModel>(std::type_identity<CameraModel>)), or inC++17 with a namespace-scope functor whose templated
operator()is invoked as
f.template operator()<CameraModel>()(local classescannot have member templates, so a functor cannot capture call-site
locals). Neither is worth it here: the standard bump is a
project-wide decision, and the functor form is far more boilerplate
than one alias line per lambda. The tag is an empty struct and is
optimised away; see the assembly section below.
Semantics preserved
Throwing vs. non-throwing behaviour is preserved per call site:
ExistsCameraModelWithId,CameraModelIdToName, andCameraModelIsPerspectiveFisheyereturn a fallback (false/ emptyname) for unknown ids, while the other predicates and runtime
functions throw
std::domain_error. Ray fitting keeps itslog-and-return path for unsupported models (it checks existence
first, since
CameraModelIsPerspectivethrows for unknown ids), andCameraModelCalibrationMatrixkeeps its perspective-only errormessage. One cosmetic loss: the
ImgFromCamWithJacstatic_assertno longer stringizes the failing model name.
Performance
I do not claim blanket parity. The dispatch itself and the real
workloads are at parity, but one synthetic alternating-model case
remains slower. Details and numbers below so reviewers can judge
the trade-off.
Method
Baseline: clean worktree at
8007d1a51(macro code). Visitor: thisbranch. Probe source, compiler flags (
-O3,-DNDEBUG, Apple Clang,arm64), and inputs are identical; only the header tree differs.
ImgFromCam/CamFromImginputs vary per iteration through aruntime
perturbvalue so the compiler cannot hoist the projectionor replace the loop with a closed form. macOS has no hard core
pinning, so each process raised its QoS to user-interactive, runs
were interleaved in baseline/visitor pairs with varying seeds, and
each reported value is the median of 7 repeats (min reported
alongside in the analysis). Checksums match exactly for the fixed
cases; alternating
ImgFromCamchecksums differ slightly betweenversions (about 2e-5 relative), consistent with floating-point
contraction differences in iterative models. Probe binaries were
built without LTO; the benchmark binaries below used the project's
CMake configuration (interprocedural optimisation enabled).
Jump table and assembly
Both dispatch functions emit a jump table: a range check against 18,
a jump-table load (
lJTI,ldrh), and an indirect branch (br),with the model projections inlined into the dispatch body. The
call footprint is the same (libm
atan/atan2/tanplus theexception path; no extra calls in the visitor version), except one
tail call to the
RadTanThinPrismFisheyeprojection in the visitorbuild. The hot loop makes one direct call per iteration in both
versions (the baseline loop calls
CameraModelImgFromCam; thevisitor loop calls
DispatchCameraModelafterCameraModelImgFromCamand the visitor wrapper inline away).Code size grows modestly: the dispatch body is 861 vs. 828 assembly
lines, and
__textfor the probe binary is 52,804 vs. 50,752 bytes(+2,052 bytes, +4.0%). The tail call is not the whole story for the
gap below: table-driven probes that skip
RadTanThinPrismFisheyeentirely still measure slower (median ratio 1.19), as do probes
that skip
OpenCV(1.08).Microbenchmarks (10 interleaved pairs)
Ratios are visitor / baseline on paired medians; < 1.0 means the
visitor is faster.
CameraModelNumParams, alternating, 100M callsCameraModelIsPerspective, alternating, 100M callsCameraModelImgFromCam, fixed pinholeCameraModelImgFromCam, fixed OpenCVCameraModelImgFromCam, alternating all 19 modelsCameraModelCamFromImg, alternating all 19 modelsCaveat: the
NumParams/IsPerspectiveloops use only aseed-varying id sequence with constant per-model results, so the
compiler may partially fold them; treat those two rows as weak
evidence. The
ImgFromCam/CamFromImgrows use runtime-varyinginputs and are the ones to weight. The alternating
ImgFromCamgap also appears in an earlier varying-input probe (visitor
171–202 ms vs. baseline 158–161 ms over 20M iterations) and in
table-driven probes (all models 1.11, skip-OpenCV 1.08,
skip-RadTan 1.19 over 6 pairs). An earlier experiment returning
the
optionaldirectly from the visitor lambda did not close thegap and was reverted.
Real workloads
Synthetic Google Benchmark workloads, built for baseline and visitor
with the project CMake configuration and run interleaved:
BM_BundleAdjustment/Ceres/5/1/1/10/10000, 5repetitions, 4 pairs): median real time 201–227 ms (baseline) vs.
198–221 ms (visitor); paired ratios 0.87, 1.05, 1.01, 1.06.
Average time per LM step is likewise overlapping
(about 29–33 ms vs. 29–30 ms).
3 pairs each):
PINHOLEmedians 1,024–1,054 ms (baseline) vs.1,041–1,056 ms (visitor);
OPENCVmedians 1,781–1,796 ms vs.1,766–1,781 ms. Reconstruction counts are identical (25 images,
1,000/1,001 points). These workloads use one camera model per
camera, so they do not stress the alternating pattern above, and
bundle-adjustment cost evaluation already bypasses runtime dispatch
via templated cost functors.
Verdict: use the visitor dispatch if the alternating-
ImgFromCamregression (about 9% in a synthetic loop that changes model on
every call, a pattern real reconstructions do not follow) is
acceptable, or investigate the dispatch code layout/inlining for
that specific function further before landing.
Tests
721e185dd, FlexGlue):cmake --build build -j 11succeeds.ctest -R "^(sensor|estimators|calibration|scene|sfm|controllers)/":67/67 tests pass after the rebase, including the new
CameraModelDispatch.CoversAllModelstest and the existingmodels_test,models_jacobian_test,reprojection_error_test,ray_fitting_test, andcamera_testsuites.PYENV_VERSION=colmap scripts/format/c++.shpasses andgit diff --checkis clean.not fully built. Reviewers should confirm pycolmap CI covers
src/pycolmap/scene/camera.cc.