From 4a3deeacdcdd83ac94c561b8b8fcf79c501851c1 Mon Sep 17 00:00:00 2001 From: Janick Martinez Esturo Date: Thu, 10 Sep 2026 10:19:02 +0200 Subject: [PATCH] fix(data): apply --start-time-sec to COLMAP camera frame timestamps ColmapCamera.timestamps_us was declared as a @property that also took a start_time_sec parameter. A property cannot receive arguments, so every access used the signature default of 0.0 and the configured offset never reached the camera frames. The offset did reach the declared sequence interval and the SfM point cloud's reference timestamp, so a non-zero --start-time-sec left frames at t=0 while the interval began at the offset. Since the writer asserts frame containment rather than clamping, conversion failed outright. Carry start_time_sec on ColmapCamera and read it from self. Adds a data-free regression test; the package had no test target before. --- tools/data_converter/colmap/BUILD.bazel | 15 ++++ tools/data_converter/colmap/converter.py | 7 +- tools/data_converter/colmap/converter_test.py | 86 +++++++++++++++++++ 3 files changed, 106 insertions(+), 2 deletions(-) create mode 100644 tools/data_converter/colmap/converter_test.py diff --git a/tools/data_converter/colmap/BUILD.bazel b/tools/data_converter/colmap/BUILD.bazel index d7e9c0dd..2588d53a 100644 --- a/tools/data_converter/colmap/BUILD.bazel +++ b/tools/data_converter/colmap/BUILD.bazel @@ -15,6 +15,7 @@ load("@ncore_pip_deps//:requirements.bzl", "requirement") load("@rules_python//python:defs.bzl", "py_binary", "py_library") +load("//bazel/pytest:defs.bzl", "pytest_test") # targets might be used in various places also outside of the repo, so make it public package(default_visibility = ["//visibility:public"]) @@ -57,3 +58,17 @@ alias( name = "colmap", actual = ":convert", ) + +# Data-free unit test for the synthetic timestamp generation (runs in CI). +pytest_test( + name = "pytest_converter", + srcs = ["converter_test.py"], + python_versions = ["3.11"], + deps = [ + ":pylib", + "@pycolmap", + requirement("numpy"), + requirement("universal_pathlib"), + "//ncore:pylib", + ], +) diff --git a/tools/data_converter/colmap/converter.py b/tools/data_converter/colmap/converter.py index aa238698..2cb28c7c 100644 --- a/tools/data_converter/colmap/converter.py +++ b/tools/data_converter/colmap/converter.py @@ -81,14 +81,15 @@ class ColmapCamera: image_names: list[str] = field(default_factory=list) T_ref_camera_list: list[np.ndarray] = field(default_factory=list) reference_frame: str = "world" + start_time_sec: float = 0.0 @property def n_images(self) -> int: return len(self.image_names) @property - def timestamps_us(self, start_time_sec: float = 0.0) -> np.ndarray: - return (1e6 * (start_time_sec + np.linspace(0.0, self.n_images - 1, self.n_images))).astype(np.uint64) + def timestamps_us(self) -> np.ndarray: + return (1e6 * (self.start_time_sec + np.linspace(0.0, self.n_images - 1, self.n_images))).astype(np.uint64) @property def T_camera_refs(self) -> np.ndarray: @@ -385,6 +386,7 @@ def populate_camera_data( camera_id=ncore_camera_id, colmap_camera=self.scene_manager.cameras[imdata[k].camera_id], image_path=parent_dir / images_dir, + start_time_sec=self.start_time_sec, ) cameras[ncore_camera_id].T_ref_camera_list.append(T_ref_camera) cameras[ncore_camera_id].image_names.append(imdata[k].name) @@ -409,6 +411,7 @@ def populate_camera_data( T_ref_camera_list=[np.eye(4)] * len(image_names), image_names=image_names, downsample_factor=downsample_factor, + start_time_sec=self.start_time_sec, ) return cameras diff --git a/tools/data_converter/colmap/converter_test.py b/tools/data_converter/colmap/converter_test.py new file mode 100644 index 00000000..d1cd850e --- /dev/null +++ b/tools/data_converter/colmap/converter_test.py @@ -0,0 +1,86 @@ +# SPDX-FileCopyrightText: Copyright (c) 2026 NVIDIA CORPORATION & AFFILIATES. All rights reserved. +# SPDX-License-Identifier: Apache-2.0 +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +"""Data-free unit tests for the COLMAP converter's synthetic timestamps. + +These run in CI (no external dataset needed). COLMAP scenes carry no capture +times, so the converter synthesizes them at one second per frame, offset by +``--start-time-sec``. That offset also determines the declared sequence +interval, so a camera whose timestamps ignore it produces frames outside the +interval and the writer rejects them. +""" + +from __future__ import annotations + +import unittest + +import numpy as np +import pycolmap + +from upath import UPath + +from tools.data_converter.colmap.converter import ColmapCamera + + +def _camera(n_images: int, start_time_sec: float) -> ColmapCamera: + """Builds a ColmapCamera with n_images frames. + + The camera model and image path are placeholders: the timestamps under test + depend only on the image count and the start time. + """ + return ColmapCamera( + camera_id="camera_0", + colmap_camera=pycolmap.Camera("PINHOLE", 640, 480, [500.0, 500.0, 320.0, 240.0]), + image_path=UPath("/nonexistent"), + image_names=[f"{i:04d}.jpg" for i in range(n_images)], + start_time_sec=start_time_sec, + ) + + +class TestColmapTimestamps(unittest.TestCase): + def test_timestamps_are_one_second_apart(self) -> None: + timestamps_us = _camera(n_images=5, start_time_sec=0.0).timestamps_us + np.testing.assert_array_equal(timestamps_us, np.arange(5, dtype=np.uint64) * 1_000_000) + + def test_start_time_sec_offsets_frame_timestamps(self) -> None: + """The offset must reach the camera frames, not only the sequence interval.""" + start_time_sec = 10.0 + timestamps_us = _camera(n_images=4, start_time_sec=start_time_sec).timestamps_us + + expected = int(1e6 * start_time_sec) + np.arange(4, dtype=np.uint64) * 1_000_000 + np.testing.assert_array_equal(timestamps_us, expected) + + def test_frames_fall_inside_the_declared_sequence_interval(self) -> None: + """Regression: frames used to start at 0 regardless of --start-time-sec. + + The converter declares the sequence interval as + ``[start_time_sec, start_time_sec + n_images)`` seconds. Frames outside + it are rejected by the writer rather than clamped. + """ + start_time_sec, n_images = 10.0, 4 + timestamps_us = _camera(n_images=n_images, start_time_sec=start_time_sec).timestamps_us + + interval_start_us = int(1e6 * start_time_sec) + interval_stop_us = int(1e6 * (start_time_sec + n_images)) + + self.assertGreaterEqual(int(timestamps_us[0]), interval_start_us) + self.assertLess(int(timestamps_us[-1]), interval_stop_us) + + def test_empty_camera_yields_no_timestamps(self) -> None: + self.assertEqual(len(_camera(n_images=0, start_time_sec=3.0).timestamps_us), 0) + + +if __name__ == "__main__": + unittest.main()