Add OpenCV lens distortion rendering to Newton - #6851
Conversation
Use Newton's native fisheye ray generation and add full OpenCV pinhole distortion so calibrated cameras render consistently across backends.
Greptile SummaryThe PR adds OpenCV pinhole and fisheye ray generation to the Newton renderer, including calibrated intrinsics and coefficient muting.
Confidence Score: 4/5The PR appears safe to merge, with a non-blocking gap in coverage for the newly advertised rational and thin-prism pinhole coefficients. The renderer dispatch and tested projection paths are coherent, but regressions in k4-k6 and s1-s4 handling would not be detected because all added fixtures disable those terms. Files Needing Attention: source/isaaclab_newton/test/renderers/test_opencv_distortion_rays.py Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
C[Camera distortion config] --> M{OpenCV model}
M -->|opencvFisheye| F[Newton fisheye ray helper]
M -->|opencvPinhole| P[Warp fixed-point inversion kernel]
F --> R[Per-pixel camera rays]
P --> R
R --> N[Newton tiled renderer]
N --> O[Camera outputs]
Reviews (1): Last reviewed commit: "Add OpenCV lens distortion rendering to ..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| _PINHOLE_COEFFS = dict( | ||
| k1=0.1, k2=-0.05, k3=0.01, k4=0.0, k5=0.0, k6=0.0, p1=0.001, p2=-0.002, s1=0.0, s2=0.0, s3=0.0, s4=0.0 | ||
| ) |
There was a problem hiding this comment.
The shared nontrivial fixture leaves k4–k6 and s1–s4 at zero, so the round-trip tests never exercise the newly advertised rational-denominator or thin-prism inversion paths. Add nonzero, valid values for these terms to catch coefficient-ordering, sign, and numerical regressions that otherwise produce incorrect edge-of-frame rays without failing this suite.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
The Newton backend now consumes existing OpenCV camera calibration configs for pinhole and fisheye ray generation, but the pinhole render-grid mapping introduces a half-pixel calibration offset that should be corrected before merge.
- Design and architecture: The implementation appropriately keeps distortion handling within the Newton renderer, delegates fisheye rays to Newton’s native helper, and uses a dedicated Warp kernel for the full OpenCV pinhole model. The cached ray-field design matches the renderer’s existing camera-ray layout.
- API: No public camera configuration or renderer signatures are changed. Existing OpenCV calibration fields are consumed by Newton, including calibrated intrinsics and the existing apply_lens_distortion flag. The changelog fragment documents the resulting backend behavior change.
- Implementation: The coefficient handling, OpenCV-to-OpenGL basis conversion, ray-buffer allocation, and fixed-point inversion are coherent. However, mapping each render pixel to (px + 0.5) times the calibration scale without subtracting 0.5 shifts rays relative to OpenCV’s integer-centered pixel coordinates, including at native calibration resolution. Use a center-preserving mapping for both axes so the effective principal point matches the authored cx and cy.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
|
|
||
| # Map the render pixel onto the calibrated image grid, then to distorted normalized coordinates. | ||
| # OpenCV image y points down. | ||
| u = ((wp.float32(px) + 0.5) / wp.float32(width)) * image_width |
There was a problem hiding this comment.
🔵 Suggestion · Implementation — Half-pixel offset in calibrated pixel mapping
OpenCV intrinsics are expressed in a frame where integer pixel coordinates are pixel centers, but this maps render pixel px to (px + 0.5) * image_width / width, shifting every ray by half a calibrated pixel and biasing the effective principal point relative to the authored cx/cy. A center-preserving mapping such as (px + 0.5) * image_width / width - 0.5 (and likewise for v) keeps the rendered rays consistent with the calibration this feature exists to honor.
AntoineRichard
left a comment
AntoineRichard
left a comment
There was a problem hiding this comment.
AI-generated review (Codex).
Requesting changes for test validity, stale public documentation, and code hygiene. The renderer path is directionally sound, but the integration fixture is non-invertible over the tested frame, the new integration test emits deprecation warnings, and the public camera configuration still says Newton ignores this model. I also found avoidable documentation/test duplication and comments that narrate review history or unsupported rationale.
I did not repeat the two existing inline threads about full coefficient coverage and pixel-center mapping; both remain unresolved. Before changing the latter, reconcile it with Newton's native fisheye helper, which currently uses the same half-pixel mapping.
Verification at this SHA: ./isaaclab.sh -f passed in an isolated checkout; all 4 CPU ray tests passed; the 3 CUDA integration tests collected and skipped because CUDA is unavailable here. The PR's isaaclab_newton CI job passed; a separate isaaclab_tasks [3/3] job is failing.
| * Added OpenCV lens-distortion support to the Newton renderer: a camera cfg carrying an OpenCV | ||
| pinhole (``k1..k6``, ``p1``, ``p2``, ``s1..s4``) or fisheye (``k1..k4``) distortion model on | ||
| ``spawn.distortion`` is now rendered through the distortion instead of as a centered, square-pixel | ||
| pinhole. Fisheye cameras use Newton's native OpenCV fisheye ray helper, while pinhole cameras use | ||
| an Isaac Lab kernel supporting rational radial, tangential, and thin-prism distortion. Both paths | ||
| honor the calibrated ``fx/fy/cx/cy`` intrinsics (including non-square focal lengths and an | ||
| off-center principal point). With ``apply_lens_distortion=False`` the distortion coefficients are | ||
| muted while the intrinsics are still applied, matching the RTX/OVRTX behavior. |
There was a problem hiding this comment.
[AI-generated] Changelog — keep this user-facing
This repeats the implementation and PR description (kernel/helper choices and the coefficient inventory). The changelog guidelines explicitly exclude internal details. Condense it to the behavior, for example: Added OpenCV pinhole and fisheye lens-distortion rendering to the Newton backend, including calibrated intrinsics and coefficient muting through apply_lens_distortion.
| # Match the PR kernel's forward-facing camera hemisphere and avoid validating the | ||
| # OpenCV polynomial outside its physically meaningful calibration range. | ||
| max_fov=math.pi, |
There was a problem hiding this comment.
[AI-generated] Comment — make it functional
PR kernel is review-history language, and max_fov caps the accepted ray angle; it does not validate the polynomial. Replace both lines with the durable statement # Limit fisheye rays to the forward hemisphere.
| OpenCV lens distortion (``spawn.distortion``) needs no preparation here: it is consumed at | ||
| ray-generation time by :meth:`RenderData._build_distortion_rays`, which inverts the OpenCV | ||
| forward model per pixel to trace the distorted camera-space rays. |
There was a problem hiding this comment.
[AI-generated] Documentation — update the source of truth
This adds renderer-local implementation prose while the public docstrings for OpenCvDistortionCfg and PinholeCameraCfg.distortion in sensors_cfg.py still state that Newton does not apply the model. Remove this duplicated paragraph and update those public docstrings to describe Newton support; otherwise the documented API contradicts the implementation.
| """Warp ray generation for OpenCV pinhole lens distortion under the Newton renderer. | ||
|
|
||
| The Newton tiled camera renders by tracing an explicit per-pixel ray field of shape | ||
| ``(camera_count, height, width, 2)`` (``wp.vec3f``): index ``0`` holds the ray origin in camera | ||
| space (always ``wp.vec3f(0.0)``) and index ``1`` the normalized ray direction in camera space. | ||
| Newton uses the OpenGL camera convention (``+X`` right, ``+Y`` up, looking down ``-Z``). | ||
|
|
||
| To honor an OpenCV ``fx/fy/cx/cy`` + distortion-coefficient calibration, for each output pixel the | ||
| kernel below inverts the OpenCV forward distortion model to recover the *undistorted* normalized | ||
| image coordinates ``(x_u, y_u)`` (OpenCV image ``y`` points down), then emit the camera-space ray | ||
| ``normalize(vec3(x_u, -y_u, -1))`` -- the negation of ``y`` and ``z`` maps OpenCV camera space | ||
| (``+Z`` forward, ``+Y`` down) onto Newton's OpenGL camera space. | ||
|
|
||
| OpenCV fisheye ray generation is provided directly by Newton's | ||
| ``SensorTiledCamera.utils.compute_camera_rays_fisheye_opencv`` helper. |
There was a problem hiding this comment.
[AI-generated] Documentation — remove duplication
Keep the module docstring to a one-sentence scope. Ray layout and coordinate conversion are repeated in the kernel documentation/body, while fisheye delegation is not implemented in this module. These copies add maintenance surface without a second contract.
| # Number of fixed-point iterations used to invert the distortion model. OpenCV's own | ||
| # ``undistortPoints`` defaults to a similar iteration budget; the inversion converges well within | ||
| # this for realistic calibrations. | ||
| _INVERSION_ITERATIONS = 20 |
There was a problem hiding this comment.
[AI-generated] Comment — correct the iteration claim
OpenCV documents the default undistortPoints path as 5 iterations, so 20 is not a similar budget, and this PR has no convergence test supporting “well within.” Keep the comment functional (what the constant controls), or add evidence for the selected bound and failure behavior.
| def test_pinhole_off_center_principal_point_is_honored(): | ||
| """The ray through the principal-point pixel looks straight ahead (down -Z).""" | ||
| rays = _launch_pinhole({k: 0.0 for k in _PINHOLE_COEFFS}) | ||
| directions = rays[0, :, :, 1, :] | ||
| # pixel closest to the principal point on the render grid | ||
| px = int(round(_CALIB["cx"] / _IMAGE_W * WIDTH - 0.5)) | ||
| py = int(round(_CALIB["cy"] / _IMAGE_H * HEIGHT - 0.5)) | ||
| direction = directions[py, px] | ||
| assert direction[0] == pytest.approx(0.0, abs=2e-2) | ||
| assert direction[1] == pytest.approx(0.0, abs=2e-2) |
There was a problem hiding this comment.
[AI-generated] Test — remove redundant weak coverage
test_pinhole_zero_coeffs_matches_ideal_projection already checks sampled rays against the full off-center calibration. This test adds no independent oracle, and its 2e-2 tolerance cannot detect sub-pixel principal-point errors. Remove it, or use a calibration whose principal point lands exactly on a sampled pixel and assert the expected ray tightly.
| # Example real-world OpenCV pinhole calibration (fx != fy, off-center principal point). | ||
| _CALIB = dict(fx=339.26592887, fy=338.82010626, cx=323.55809091, cy=250.27360914) | ||
| _COEFFS = dict(k1=0.07702322, k2=-0.13605453, k3=0.05163219, p1=-0.00024938, p2=-0.00175006) | ||
| # scale the (mild) real coefficients so the barrel effect is unambiguous in the assertion | ||
| _K_SCALE = 15.0 | ||
| # OpenCV fisheye (equidistant) coefficients; the base fisheye projection alone differs strongly from pinhole | ||
| _FISHEYE_COEFFS = dict(k1=0.1, k2=-0.05, k3=0.0, k4=0.0) |
There was a problem hiding this comment.
[AI-generated] Important — use an invertible calibration fixture
Multiplying these radial coefficients by 15 makes the forward radial mapping non-monotonic inside this frame: its derivative first becomes negative near normalized radius 0.899, while the corners reach about 1.2. The inverse is therefore ambiguous, yet the test only requires a different image, so invalid rays can pass. Use a synthetic coefficient set that stays monotonic over the full frame and assert representative rays or render samples against an oracle.
| spawn=sim_utils.CuboidCfg( | ||
| size=(0.01, 0.01, 0.01), | ||
| rigid_props=sim_utils.RigidBodyPropertiesCfg(), | ||
| mass_props=sim_utils.MassPropertiesCfg(mass=0.001), | ||
| collision_props=sim_utils.CollisionPropertiesCfg(), | ||
| physics_material=sim_utils.RigidBodyMaterialCfg(), | ||
| visual_material=sim_utils.PreviewSurfaceCfg(diffuse_color=(0.0, 0.0, 0.0)), |
There was a problem hiding this comment.
[AI-generated] Test hygiene — do not add deprecated APIs
These RigidBodyPropertiesCfg, CollisionPropertiesCfg, and RigidBodyMaterialCfg constructors emit three deprecation warnings during collection. New tests should use the solver-common RigidBodyBaseCfg, CollisionBaseCfg, and RigidBodyMaterialBaseCfg equivalents (or omit properties the off-screen anchor does not need).
| assert distorted.shape == (HEIGHT, WIDTH, 1) | ||
| # both frames render geometry (the ground plane fills the frame) | ||
| assert np.isfinite(distorted).mean() > 0.9 | ||
| assert np.isfinite(reference).mean() > 0.9 | ||
| # the renderer applied the lens distortion: the distance maps warp well beyond render noise | ||
| mean_abs_diff = _mean_abs_distance_diff(distorted, reference) | ||
| assert mean_abs_diff > 0.05, f"distorted vs reference distance maps differ by only {mean_abs_diff:.4f} m" |
There was a problem hiding this comment.
[AI-generated] Comments — remove assertion narration
The two comments restate the assertions immediately below them. Remove them and let the test/helper names carry the contract; apply the same cleanup to the repeated comments in the fisheye test.
| def test_opencv_distortion_intrinsics_match_authored_newton(device): | ||
| """The Newton camera reports intrinsics matching the authored, non-square, off-center calibration.""" | ||
| _distance, k = _render_distance(_pinhole_distortion(True), device=device) | ||
|
|
||
| assert k[0, 0] == pytest.approx(_CALIB["fx"], abs=1e-2) | ||
| assert k[1, 1] == pytest.approx(_CALIB["fy"], abs=1e-2) | ||
| assert k[0, 2] == pytest.approx(_CALIB["cx"], abs=1e-2) | ||
| assert k[1, 2] == pytest.approx(_CALIB["cy"], abs=1e-2) | ||
| # not the stock fx == fy / centered-principal-point collapse | ||
| assert k[0, 0] != k[1, 1] | ||
| assert k[0, 2] != pytest.approx(WIDTH / 2) |
There was a problem hiding this comment.
[AI-generated] Test — remove unrelated duplicate integration coverage
The Newton renderer does not compute Camera.data.intrinsic_matrices; Camera._update_intrinsic_matrices does. source/isaaclab/test/sensors/test_opencv_distortion.py::test_readback_uses_authored_fx_fy_cx_cy already checks these same values without a CUDA scene. Remove this test and simplify _render_distance to return only data needed to verify the changed renderer path.
4 participants
Description
PR #6608 introduced renderer-agnostic OpenCV camera calibration configs and native RTX/OVRTX rendering. The Newton path was intentionally left as a documented no-op that emitted a warning and rendered using a centered, square-pixel pinhole projection.
This PR completes backend parity by making
NewtonWarpRendererconsume the same OpenCV calibration and generate distorted per-pixel camera rays.This PR adds native OpenCV lens-distortion rendering to Newton:
k1..k6), tangential (p1,p2), and thin-prism (s1..s4) distortion - conritbuted by @AntoineRichard in Add native OpenCV lens-distortion rendering for the Newton renderer liorbenhorin/IsaacLab#1compute_camera_rays_fisheye_opencvhelper rather than maintaining a duplicate Isaac Lab implementation.fx/fy/cx/cy, including non-square focal lengths and an off-center principal point.apply_lens_distortion=Falsemutes the coefficients while retaining the calibrated intrinsics, matching RTX/OVRTX behavior.This is a stacked follow-up to #6608. It does not change the public camera configuration or USD authoring path introduced there; it only adds Newton renderer consumption of the existing model.
Images
Checklist
pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there