gsplat-rust-renderer: move tools/ CLI logic into apis/ thin shims - #55
Merged
Conversation
Follow-up to #50. These tools/ scripts kept their Tyro Config + main() + helpers inline, so `beartype_this_package()` (dev-mode runtime type checking) never instrumented them — the heavy jaxtyping annotations were effectively decorative. Move the implementation of visualize_brush_training, log_splats_with_cameras, calibration_scene, and relog_check into `gsplat_rust_renderer/apis/<name>.py` and reduce each `tools/<name>.py` to a ~5-line Tyro entrypoint shim, matching the repo convention (e.g. `simplecv/tools/view_exoego.py`). Pure relocation, no logic change. Verified: ruff + pyrefly clean, 13 tests pass, all three Tyro CLIs `--help`, and the apis modules import under beartype (so the annotations are now enforced in dev). Addresses Copilot review comments #5-8 on #50. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…r helper Simplify pass over the relocated apis/ CLIs (parallel reuse/quality/efficiency review): - Extract the duplicated COLMAP binary readers, qvec->rotmat, NeRF-synthetic loader, and RGBA->RGB compositing into gsplat_rust_renderer/scene_io.py; both visualize_brush_training and log_splats_with_cameras import them. The copies had already drifted (only one supported OPENCV_FISHEYE) — scene_io uses the superset model table. - Replace the hand-rolled up_to_z_rotation Rodrigues with simplecv's existing rotation_matrix_between (numerically identical; simplecv is already a dep). - count_eval_views: bind colmap_sparse_dir once instead of calling it twice + asserting. load_colmap_cameras kept per-file (the two genuinely differ: nested-dir + downscaled thumbnails + distortion-tolerant slicing vs sparse/0 + full-res + 4-param), so behavior is unchanged; both now call the shared readers. Kept the hand-rolled qvec_to_rotmat (COLMAP wxyz vs scipy xyzw is a footgun, and it is not hot-path). visualize_brush_training 835->726, log_splats 284->178 lines. Verified: ruff + pyrefly clean, 13 tests pass, all 3 CLIs --help, apis + scene_io import under beartype. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…eviewed) Resolve the two deferred simplifications: - Unify load_colmap_cameras into scene_io with an `image_policy` param (visualize_brush_training="thumbnail", log_splats="full"). Removes the two drifted copies + vbt's local colmap_sparse_dir/thumbnail_image_path. Adds model-aware intrinsics parsing — fixes a latent bug: params[:4] is WRONG for single-focal COLMAP models (SIMPLE_PINHOLE/SIMPLE_RADIAL/RADIAL store f,cx,cy, not fx,fy,cx,cy). log_splats now also handles a nested colmap/sparse/0 layout. - qvec_to_rotmat: drop the hand-expanded formula for scipy.spatial.transform.Rotation (what simplecv already standardizes on), with an explicit COLMAP-wxyz -> scipy-xyzw reorder. Verified numerically identical to the old formula (max abs diff 1.1e-15 over 2000 unit quats). scipy is provided via the simplecv dependency. Behavior preserved for the normal PINHOLE/OPENCV + images/ case (log_splats "full" resolves images/ first; vbt "thumbnail" keeps the images_8/4/2 ladder). visualize_brush_training 726->677, log_splats 178->137. Verified: ruff + pyrefly clean, 13 tests, all 3 CLIs --help, scene_io imports under beartype. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
…LMAP models, ladder dedup) Consolidated from three adversarial reviews (Codex + two subagents): - [blocker] apis/relog_check.py: the tools/->apis/ move added a directory level, so the viewer-binary path (Path(__file__).parents[1]) silently pointed at gsplat_rust_renderer/ instead of the package root. parents[1] -> parents[2]. - [bug] scene_io.py: the COLMAP model table stopped at id 5, so any sparse model using FULL_OPENCV (6) / FOV (7) / *_FISHEYE (8,9) / THIN_PRISM_FISHEYE (10) — all produced by adjacent repo tooling (pysfm) — KeyError'd before logging. Complete the table (ids 0-10) and extend _colmap_fx_fy_cx_cy: single-focal models -> f,f,cx,cy, everything else -> params[0:4]. - [cleanup] Replace the 2-value image_policy enum (which just reversed a hardcoded ladder) with an explicit image_subdirs ladder on load_colmap_cameras / colmap_image_path. This folds visualize_brush_training's third copy of the ladder resolver (eval_image_path) into colmap_image_path — the same dedup this PR set out to do. Behavior preserved: thumbnail/eval/full ladders unchanged. Not changed (reviewed, deliberately): scipy stays (it's a first-order dep of the pinned lpips, not a fragile transitive; quat output verified identical to 1e-15); the up-vector helper swap (estimate_up -> simplecv.rotation_matrix_between) is mathematically equivalent. Verified: ruff + pyrefly clean, 13 tests, all 4 CLIs --help, all 11 COLMAP camera models parse correctly, relog BINARY resolves to the package-root target/release. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
From a Codex simplify-code review of the relocated apis/ + scene_io: - log_gaussian_ply: complete the thin-shim refactor — move LogPlyConfig, splat_blueprint, and main into apis/log_gaussian_ply.py (now beartype- instrumented like the other four tools); tools/ is a ~7-line shim. Caught the same parents[1]->parents[2] path trap as relog_check: DEFAULT_PLY now resolves to the package-root examples/chair.ply from its deeper apis/ location. - visualize_brush_training: extract ready_export_plys(), a generator that yields the unprocessed, size-stable export_*.ply checkpoints. Both watch loops duplicated the same walrus-glob scan + done_plys/awaiting_stable_size skips; the per-loop bodies (which differ) stay inline. - visualize_brush_training: make the eval-disabled (eval_split_every == 0) path an explicit if/else instead of a buried comprehension guard. Left alone per the review: scene_io grouping (cohesive), qvec_to_rotmat / _colmap_fx_fy_cx_cy (small but encode COLMAP conventions), brush_blueprint's nested cell helpers (keep a dense blueprint readable), and calibration_scene's in-package Tyro subcommand parse (moving it to the shim would drop it out of beartype instrumentation). Verified: ruff + pyrefly clean, 13 tests, all 5 CLIs --help, DEFAULT_PLY resolves. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR restructures the gsplat-rust-renderer Python CLIs so runtime type checking via beartype_this_package() applies to the Tyro Config + main() implementations by moving them from tools/ into gsplat_rust_renderer/apis/, leaving tools/ as thin entrypoint shims. It also consolidates shared scene-loading utilities (NeRF-synthetic + COLMAP parsing and RGBA compositing) into a new in-package scene_io.py module.
Changes:
- Moved heavy CLI implementations into
gsplat_rust_renderer/apis/*and reduced eachtools/*.pyto a minimal Tyro shim. - Added
gsplat_rust_renderer/scene_io.pyto deduplicate COLMAP/NeRF loaders, quaternion conversion, and alpha compositing used by multiple CLIs. - Updated orientation logic to use
simplecv.camera_orient_utils.rotation_matrix_betweenand simplified some repeated logic (e.g., COLMAP sparse dir handling).
Reviewed changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| packages/gsplat-rust-renderer/tools/visualize_brush_training.py | Converted to a thin Tyro shim invoking the beartype-instrumented API implementation. |
| packages/gsplat-rust-renderer/tools/relog_check.py | Converted to a thin wrapper delegating to apis.relog_check.main(). |
| packages/gsplat-rust-renderer/tools/log_splats_with_cameras.py | Converted to a thin Tyro shim delegating to the API module. |
| packages/gsplat-rust-renderer/tools/log_gaussian_ply.py | Converted to a thin Tyro shim delegating to the API module. |
| packages/gsplat-rust-renderer/tools/calibration_scene.py | Converted to a thin wrapper calling the API module’s CLI entrypoint. |
| packages/gsplat-rust-renderer/gsplat_rust_renderer/scene_io.py | New shared scene I/O utilities (COLMAP binary readers, NeRF-synthetic loader, RGBA compositing). |
| packages/gsplat-rust-renderer/gsplat_rust_renderer/apis/visualize_brush_training.py | New beartype-instrumented implementation for the Brush sidecar visualizer CLI. |
| packages/gsplat-rust-renderer/gsplat_rust_renderer/apis/relog_check.py | New beartype-instrumented re-log regression check implementation. |
| packages/gsplat-rust-renderer/gsplat_rust_renderer/apis/log_splats_with_cameras.py | New beartype-instrumented dataset camera + splat logging CLI implementation. |
| packages/gsplat-rust-renderer/gsplat_rust_renderer/apis/log_gaussian_ply.py | New beartype-instrumented Gaussian PLY logging CLI implementation. |
| packages/gsplat-rust-renderer/gsplat_rust_renderer/apis/calibration_scene.py | New beartype-instrumented calibration scene generator/checker implementation. |
| packages/gsplat-rust-renderer/gsplat_rust_renderer/apis/init.py | Introduces the apis package namespace for Tyro entrypoints. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+4
to
+7
| Imported by both ``apis/visualize_brush_training.py`` and | ||
| ``apis/log_splats_with_cameras.py`` — kept in the package (not ``tools/``) so | ||
| ``beartype_this_package()`` instruments it in dev. pycolmap is not in this env; | ||
| the COLMAP binary layout is stable and these parsers avoid a heavyweight dep. |
Comment on lines
+70
to
+74
| def qvec_to_rotmat(q: tuple[float, float, float, float]) -> Float64[ndarray, "3 3"]: | ||
| """COLMAP quaternion (w, x, y, z) → 3x3 rotation matrix.""" | ||
| w, x, y, z = q | ||
| xyzw: Float64[ndarray, "4"] = np.asarray([x, y, z, w], dtype=np.float64) # COLMAP wxyz -> scipy xyzw | ||
| return np.asarray(Rotation.from_quat(xyzw).as_matrix(), dtype=np.float64) |
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.
Follow-up to #50 (addresses Copilot review comments #5–8).
Why
tools/scripts are meant to be thin shims: the TyroConfig+main()+ logic live inside the package sobeartype_this_package()(dev-mode runtime type checking) instruments them. Four gsplat CLIs kept everything inline intools/, so their heavy jaxtyping annotations (Float64[ndarray, "3 3"],UInt8[ndarray, "h w 3"], …) were never enforced at runtime — decorative, not live.What
Moved the implementation of
visualize_brush_training,log_splats_with_cameras,calibration_scene, andrelog_checkintogsplat_rust_renderer/apis/<name>.py; eachtools/<name>.pyis now a ~5-line Tyro entrypoint shim (matchingsimplecv/tools/view_exoego.py). ~1,630 lines relocated.Pure relocation — no logic change. Entry-only
import tyro/import sysdropped from the modules where unused; kept incalibration_scene(used bymain()).Verification
ruff(lint) +pyrefly(typecheck): clean--helpwork through the new shimsapis/modules import under beartype in the dev env (annotations now enforced)Net: zero behavior change, but the jaxtyping/beartype runtime checks the author clearly intended are now actually live.
Follow-up commit: simplify the relocated code (parallel reuse/quality/efficiency review)
Now that the CLIs live in the package, deduplicated the loaders that had been copy-pasted (and drifted) between
visualize_brush_trainingandlog_splats_with_cameras:gsplat_rust_renderer/scene_io.pyholds the shared COLMAP binary readers,qvec_to_rotmat, NeRF-synthetic loader, and RGBA→RGB compositing (superset model table — one copy had droppedOPENCV_FISHEYE).up_to_z_rotationwith simplecv's existingrotation_matrix_between(numerically identical).count_eval_views: bindcolmap_sparse_dironce.load_colmap_camerasdeliberately stays per-file (the two genuinely differ in image-path/distortion handling).visualize_brush_training835→726,log_splats284→178 lines. Verified: ruff + pyrefly clean, 13 tests, all CLIs--help.