Conversation
…ich scans is used in this refinement
samcts-auki
added
enhancement
tatiesmars
left a comment
tatiesmars
left a comment
There was a problem hiding this comment.
can we add a README.md for the new runner? Currently we do not really maintain a standard for it but I think it would be nice to have it with expected input files, expected output files (like we did for the splatter https://github.com/aukilabs/splatter-server/blob/develop/README.md#required-files)
|
reminder to add the new capability |
There was a problem hiding this comment.
Pull request overview
Adds an update refinement capability end-to-end (Python pipeline + Rust runner) to merge new refined scans into an existing canonical global reconstruction, including geometry pruning and updated artifact outputs.
Changes:
- Introduces voxel raycast pruning + model merge utilities to update an existing COLMAP model from new scan observations.
- Adds
update_main.pyentrypoint and moves PLY post-processing intoutils.point_cloud_utils. - Adds a new Rust runner crate (
runner-reconstruction-update) to materialize inputs, invoke the Python pipeline, and upload update outputs.
Reviewed changes
Copilot reviewed 15 out of 19 changed files in this pull request and generated 18 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/voxel_raycast_utils.py | New voxel raycast-based carving to remove outdated reference geometry. |
| utils/point_cloud_utils.py | Adds shared post_process_ply used by global/update pipelines. |
| utils/io.py | Adds model consistency validation, similarity transform, and merge helpers. |
| utils/dataset_utils.py | Adds update_helper, adjusts path initialization, integrates update refinement flow. |
| utils/data_utils.py | Extends manifest writing to include previous scan IDs; adds manifest parsing helper. |
| update_main.py | New CLI entrypoint for update refinement jobs. |
| global_main.py | Switches to shared post_process_ply utility. |
| server/rust/runner-reconstruction-update/src/workspace.rs | Defines on-disk workspace layout including refined/update. |
| server/rust/runner-reconstruction-update/src/strategy.rs | Output naming helpers + zip extraction helper. |
| server/rust/runner-reconstruction-update/src/python.rs | Runs python pipeline with streamed logs + cancellation handling. |
| server/rust/runner-reconstruction-update/src/output.rs | Uploads update refinement artifacts to Domain. |
| server/rust/runner-reconstruction-update/src/input.rs | Materializes refined scan zips + global colmap + manifest inputs. |
| server/rust/runner-reconstruction-update/src/lib.rs | Registers capability + orchestrates workspace/input/python/upload. |
| server/rust/runner-reconstruction-update/Cargo.toml | Adds new Rust crate + dependencies. |
| server/rust/bin/src/main.rs | Registers update runner in the compute node binary. |
| server/rust/bin/tests/bin_wiring.rs | Ensures registry wiring includes the update runner capabilities. |
| server/rust/bin/Cargo.toml | Adds dependency on the new update runner crate. |
| server/rust/Cargo.toml | Adds update runner crate to the workspace members. |
| server/rust/Cargo.lock | Locks new crate dependencies. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
samcts-auki
left a comment
samcts-auki
left a comment
There was a problem hiding this comment.
made some changes in later commits
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 17 out of 21 changed files in this pull request and generated 10 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| portal_r, refined_files_r = parse_info_from_manifest(paths.reference_path / "refined_manifest.json") # return a dict of portal_id -> (R, t, size) | ||
| portal_sizes = {pid: portal[2] for pid, portal in portal_r.items()} | ||
| portal_r = {pid: pycolmap.Rigid3d(pycolmap.Rotation3d(portal_r[pid][0]), portal_r[pid][1]) for pid in portal_r.keys()} |
There was a problem hiding this comment.
parse_info_from_manifest reads portal poses from refined_manifest.json, which save_manifest_json stores in OpenGL coordinates. Here those OpenGL poses are wrapped directly into pycolmap.Rigid3d and then used to compute alignment_mat that is applied to COLMAP camera/point coordinates. This mixes coordinate frames (OpenGL vs COLMAP/OpenCV) and will yield an incorrect alignment. Suggest converting manifest poses back to COLMAP coords (e.g., via convert_pose_opengl_to_colmap on (t, q) before building Rigid3d) and keeping the alignment transform in the same frame as the models you transform.
| portal_r, refined_files_r = parse_info_from_manifest(paths.reference_path / "refined_manifest.json") # return a dict of portal_id -> (R, t, size) | |
| portal_sizes = {pid: portal[2] for pid, portal in portal_r.items()} | |
| portal_r = {pid: pycolmap.Rigid3d(pycolmap.Rotation3d(portal_r[pid][0]), portal_r[pid][1]) for pid in portal_r.keys()} | |
| portal_r_raw, refined_files_r = parse_info_from_manifest(paths.reference_path / "refined_manifest.json") # return a dict of portal_id -> (R, t, size) in OpenGL coords | |
| portal_sizes = {pid: portal[2] for pid, portal in portal_r_raw.items()} | |
| portal_r = {} | |
| for pid, (R_gl, t_gl, _size) in portal_r_raw.items(): | |
| # Convert portal pose from OpenGL to COLMAP coordinates before constructing Rigid3d | |
| R_col, t_col = convert_pose_opengl_to_colmap(R_gl, t_gl) | |
| portal_r[pid] = pycolmap.Rigid3d(pycolmap.Rotation3d(R_col), t_col) |
| gl_tvec, gl_qvec = convert_pose_colmap_to_opengl(portal.tvec, portal.qvec) | ||
| portals_u_list.append({ | ||
| "short_id": portal.short_id, | ||
| "tvec": gl_tvec, | ||
| "qvec": gl_qvec, | ||
| "image_id": portal.image_id, | ||
| "size": portal.size, | ||
| "corners": portal.corners, | ||
| "pose": pycolmap.Rigid3d(pycolmap.Rotation3d(np.array(gl_qvec)), np.array(gl_tvec)) |
There was a problem hiding this comment.
load_qr_detections_from_local_refinement converts portal poses from portals.csv (COLMAP coords) into OpenGL (convert_pose_colmap_to_opengl) before building pycolmap.Rigid3d. Those poses are later used to compute alignment_mat that is applied to COLMAP camera/point coordinates. Unless you also convert the COLMAP model into OpenGL first, this frame conversion here will make the computed alignment matrix incompatible with the model coordinates. Consider keeping portals from portals.csv in COLMAP coords (no conversion) and only converting when writing manifests/PLYs.
| gl_tvec, gl_qvec = convert_pose_colmap_to_opengl(portal.tvec, portal.qvec) | |
| portals_u_list.append({ | |
| "short_id": portal.short_id, | |
| "tvec": gl_tvec, | |
| "qvec": gl_qvec, | |
| "image_id": portal.image_id, | |
| "size": portal.size, | |
| "corners": portal.corners, | |
| "pose": pycolmap.Rigid3d(pycolmap.Rotation3d(np.array(gl_qvec)), np.array(gl_tvec)) | |
| # Keep portal poses in COLMAP coordinates here; conversion to OpenGL, | |
| # if needed, should be done only at export time (e.g., manifests/PLYs) | |
| colmap_tvec, colmap_qvec = portal.tvec, portal.qvec | |
| portals_u_list.append({ | |
| "short_id": portal.short_id, | |
| "tvec": colmap_tvec, | |
| "qvec": colmap_qvec, | |
| "image_id": portal.image_id, | |
| "size": portal.size, | |
| "corners": portal.corners, | |
| "pose": pycolmap.Rigid3d(pycolmap.Rotation3d(np.array(colmap_qvec)), np.array(colmap_tvec)), |
| def parse_info_from_manifest(manifest_path: Path) -> Tuple[Dict[str, Tuple[np.ndarray, np.ndarray]], List[str]]: | ||
| """ | ||
| Returns dict: shortId -> (R_world_portal, t_world_portal) |
There was a problem hiding this comment.
parse_info_from_manifest docstring/type hint says it returns shortId -> (R_world_portal, t_world_portal), but the implementation stores (R, t, size) in out[sid]. Please update the docstring and return type annotation to match the actual 3-tuple (or drop size if it isn't meant to be returned).
| def parse_info_from_manifest(manifest_path: Path) -> Tuple[Dict[str, Tuple[np.ndarray, np.ndarray]], List[str]]: | |
| """ | |
| Returns dict: shortId -> (R_world_portal, t_world_portal) | |
| def parse_info_from_manifest(manifest_path: Path) -> Tuple[Dict[str, Tuple[np.ndarray, np.ndarray, object]], List[str]]: | |
| """ | |
| Returns: | |
| A tuple (portal_info, data_ids) where: | |
| - portal_info: dict mapping shortId -> (R_world_portal, t_world_portal, physical_size) | |
| * R_world_portal: 3x3 rotation matrix (np.ndarray) | |
| * t_world_portal: 3D translation vector (np.ndarray) | |
| * physical_size: value of the 'physicalSize' field from the manifest (may be None) | |
| - data_ids: list of data IDs (as strings) containing "refined_scan". |
| ray_dir = ray_vec / dist | ||
| t_max = dist - clearance_margin | ||
| t_vals = np.arange(clearance_margin, t_max, step_size) | ||
|
|
||
| if len(t_vals) == 0: | ||
| continue | ||
|
|
||
| ray_pts = C + t_vals[:, np.newaxis] * ray_dir | ||
| ray_voxels = np.floor(ray_pts / voxel_size).astype(int) | ||
|
|
||
| # 3. Collect old points that violate the new free space | ||
| for vox in ray_voxels: | ||
| vox_tuple = tuple(vox) | ||
| if vox_tuple in voxel_to_ref_pids: | ||
| violated_ref_pids.update(voxel_to_ref_pids[vox_tuple]) | ||
| # Remove from dict so we don't process it multiple times | ||
| del voxel_to_ref_pids[vox_tuple] |
There was a problem hiding this comment.
The ray-marching loop builds t_vals/ray_pts for every observed 3D point and then iterates every voxel along the ray. For large reconstructions this can become a major bottleneck (and allocate large temporary arrays). Consider optimizations like early-breaking once voxel_to_ref_pids is empty, deduplicating ray_voxels (e.g., np.unique(..., axis=0)), and/or limiting the number of rays/steps per image to keep runtime bounded.
| result = update_helper( | ||
| dataset_paths=dataset_paths, | ||
| job_root_path=args.data_path.parent.parent, | ||
| logger_name="update_refinement" | ||
| ) |
There was a problem hiding this comment.
update_helper writes outputs under job_root_path / refined / update, but this script runs post_process_ply(output_path, ...) where output_path comes from a separate CLI arg. If --output_path doesn't match job_root_path/refined/update, post-processing will target the wrong directory (or fail to find RefinedPointCloud.ply). Consider deriving output_path from job_root_path (or passing output_path into update_helper and using it consistently) and updating the CLI defaults/help accordingly.
| portals_opengl = {pid: convert_pose_colmap_to_opengl(portal.translation, portal.rotation.quat) for pid, portal in portal_r.items()} | ||
| portals = {pid: [pycolmap.Rigid3d(pycolmap.Rotation3d(np.array(pose[1])), np.array(pose[0]))] for pid, pose in portals_opengl.items()} |
There was a problem hiding this comment.
When writing the updated manifest, this code converts portal_r from COLMAP->OpenGL (convert_pose_colmap_to_opengl) and then wraps the result back into pycolmap.Rigid3d before calling save_manifest_json. save_manifest_json already performs the COLMAP->OpenGL conversion internally, so this will effectively apply the transform twice (and likely produce incorrect portal poses in the manifest). Prefer passing {pid: [portal_pose_in_colmap]} directly into save_manifest_json without pre-converting.
| portals_opengl = {pid: convert_pose_colmap_to_opengl(portal.translation, portal.rotation.quat) for pid, portal in portal_r.items()} | |
| portals = {pid: [pycolmap.Rigid3d(pycolmap.Rotation3d(np.array(pose[1])), np.array(pose[0]))] for pid, pose in portals_opengl.items()} | |
| portals = {pid: [portal] for pid, portal in portal_r.items()} |
| dataset_paths: List of paths to datasets to stitch | ||
| job_root_path: Path to the root folder for the update job | ||
| logger_name: Name of logger to use | ||
|
|
||
| Returns: | ||
| StitchingResult containing basic and refined reconstructions |
There was a problem hiding this comment.
The update_helper docstring says it returns a StitchingResult, but the function signature/implementation returns bool. Please update the docstring to reflect the actual return type/meaning (or return a richer result object if that was intended).
| dataset_paths: List of paths to datasets to stitch | |
| job_root_path: Path to the root folder for the update job | |
| logger_name: Name of logger to use | |
| Returns: | |
| StitchingResult containing basic and refined reconstructions | |
| dataset_paths: List of paths to datasets to stitch. | |
| job_root_path: Path to the root folder for the update job. | |
| logger_name: Name of logger to use. | |
| Returns: | |
| bool: True if stitching (or preparation when only one dataset is present) | |
| completes successfully, False otherwise (e.g., when no dataset paths are found). |
| @@ -549,6 +618,10 @@ def read_model(path, ext="", logger=None): | |||
| cameras = read_cameras_binary(os.path.join(path, "cameras" + ext)) | |||
| images = read_images_binary(os.path.join(path, "images" + ext)) | |||
| points3D = read_points3D_binary(os.path.join(path, "points3D") + ext) | |||
|
|
|||
| if validate_consistency: | |||
| validate_model_consistency(cameras, images, points3D, logger=logger) | |||
|
|
|||
There was a problem hiding this comment.
read_model(..., validate_consistency=True) now runs validate_model_consistency by default. That validation does multiple full passes over images/points and includes an any(...) scan per observation, which can make loading large reconstructions very slow and noisy (extra logs) in production paths (e.g., Model.read_model). Consider defaulting validate_consistency to False (or only enabling under DEBUG), and/or making validation cheaper (e.g., precomputed lookup tables) when enabled.
| refined_sfm_dir, | ||
| }); | ||
| } | ||
|
|
||
| Ok(scans) |
There was a problem hiding this comment.
This function returns Ok(scans) even if scans is empty (e.g., when all inputs_cids fail resolve_by_name and are silently skipped). Consider erroring (or at least logging skipped inputs) when scans.is_empty() so downstream failures are easier to diagnose.
| def post_process_ply(output_path, logger=None): | ||
| ply_path = output_path / "RefinedPointCloud.ply" | ||
| filter_ply(ply_path, ply_path, convert_opencv_to_opengl=True, logger=logger) | ||
|
|
||
| # Ensure ply fits in domain data | ||
| logger.info("Downsampling ply if needed to be under 20 MB file size...") | ||
| ply_path_reduced = output_path / "RefinedPointCloudReduced.ply" |
There was a problem hiding this comment.
post_process_ply allows logger=None but then calls logger.info(...) unconditionally, which will crash when no logger is passed. Please either initialize a default logger when logger is None or make logger a required argument.
Feature
update-refinementMinor Changes