New CameraSensor Implementation - #3627
Open
daniela-hase wants to merge 5 commits into
Open
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a shape-backed ChangesCamera sensor rendering
Estimated code review effort: 5 (Critical) | ~120 minutes Sequence Diagram(s)sequenceDiagram
participant Importer
participant CameraSensor
participant ModelBuilder
participant Model
participant RenderContext
Importer->>CameraSensor: generate camera rays
Importer->>ModelBuilder: add_shape_camera
ModelBuilder->>Model: finalize and bind camera sensor
CameraSensor->>RenderContext: update transforms and render outputs
RenderContext-->>CameraSensor: return camera images
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (4)
newton/_src/sensors/camera_sensor_renderer/render_context.py (1)
337-341: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKey the kernel cache on the value tuple, not its hash.
kernel_cacheis keyed by the rawhash(...)integer, so two distinct(config, state, clear_data)triples that collide would silently reuse the wrong compiled megakernel. Since all three areeq=Truedataclasses, the tuple is directly usable as a collision-safe dict key.♻️ Proposed change
- kernel_cache_key = hash((config, self.state, clear_data)) + kernel_cache_key = (config, self.state, clear_data)Update the annotation accordingly:
- self.kernel_cache: dict[int, wp.Kernel] = {} + self.kernel_cache: dict[tuple, wp.Kernel] = {}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor_renderer/render_context.py` around lines 337 - 341, Update the kernel cache logic around kernel_cache_key to use the value tuple (config, self.state, clear_data) directly as the dictionary key instead of hashing it first, and adjust the kernel_cache annotation accordingly. Preserve the existing lookup, creation, and storage behavior while ensuring distinct tuples cannot collide.newton/_src/sensors/camera_sensor_renderer/utils.py (2)
135-139: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueResolve the leftover reviewer question before merge. This
NOTE(reviewers)block poses an open design decision (clamp/saturate vs. the legacy wrap-on-overflow behavior for out-of-range normals). Decide the intended semantics, then drop the question so it doesn't ship as a standing comment.Want me to open a tracking issue capturing the clamp-vs-wrap decision, or draft the resolved comment once you confirm the preferred behavior?
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor_renderer/utils.py` around lines 135 - 139, Resolve the open clamp-versus-wrap decision in the normal image conversion logic, preserving the intended saturating clamp behavior for out-of-range normals. Remove the NOTE(reviewers) question and replace it with a concise non-question comment only if needed to document the finalized semantics.Source: Path instructions
791-791: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReturn annotation
-> wp.array()is malformed. This invokes the array constructor in annotation position rather than naming a type (it only avoids a runtime error becausefrom __future__ import annotationskeeps it as an unevaluated string). Use a proper type; the method returns a(buffer, worlds_per_row)pair.♻️ Suggested annotation
- ) -> wp.array(): + ) -> tuple[wp.array, int]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor_renderer/utils.py` at line 791, Update the return annotation on the surrounding method to describe its actual two-element `(buffer, worlds_per_row)` result using a proper typing annotation, and remove the constructor call from `wp.array()`. Preserve the existing return behavior.newton/_src/sim/builder.py (1)
7186-7238: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDocstring doesn't mention that a supplied
cfgis also forced into a site.The
cfgdoc only describes thecfg is Nonefallback ("a non-colliding, invisible, zero-density site configuration is used"), but the code unconditionally callscfg.mark_as_site()and setsis_visible = Falseeven when the caller passes an explicitcfg(Lines 7218-7224). Any collision/visibility settings on a caller-suppliedcfgare silently discarded. Worth a one-line clarification so callers aren't surprised theircfggets overridden.📝 Proposed docstring clarification
cfg: Shape configuration. If ``None``, a non-colliding, - invisible, zero-density site configuration is used. + invisible, zero-density site configuration is used. If provided, + it is copied and forced into a site configuration + (``mark_as_site()`` plus ``is_visible=False``); any + collision/visibility settings on the supplied ``cfg`` are + overridden.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sim/builder.py` around lines 7186 - 7238, Update the cfg argument documentation in add_shape_camera to state that a supplied configuration is copied, forced to site behavior, and made invisible before use. Keep the existing description of the cfg=None fallback and align the wording with the unconditional mark_as_site and is_visible assignment.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/sim/model.py`:
- Line 30: Update the RenderContext import used by Model.render_context to come
from the active camera_sensor_renderer module instead of warp_raytrace, keeping
the annotation aligned with the RenderContext instances created by
CameraSensor._get_render_context().
In `@newton/_src/utils/import_usd.py`:
- Around line 324-328: Fix the docstring indentation for the load_sites,
load_visual_shapes, load_cameras, and hide_collision_shapes argument entries so
they align with sibling parameters such as skip_mesh_approximation and
force_show_colliders. Keep each argument’s description nested beneath its own
entry, preserving the existing text and behavior.
---
Nitpick comments:
In `@newton/_src/sensors/camera_sensor_renderer/render_context.py`:
- Around line 337-341: Update the kernel cache logic around kernel_cache_key to
use the value tuple (config, self.state, clear_data) directly as the dictionary
key instead of hashing it first, and adjust the kernel_cache annotation
accordingly. Preserve the existing lookup, creation, and storage behavior while
ensuring distinct tuples cannot collide.
In `@newton/_src/sensors/camera_sensor_renderer/utils.py`:
- Around line 135-139: Resolve the open clamp-versus-wrap decision in the normal
image conversion logic, preserving the intended saturating clamp behavior for
out-of-range normals. Remove the NOTE(reviewers) question and replace it with a
concise non-question comment only if needed to document the finalized semantics.
- Line 791: Update the return annotation on the surrounding method to describe
its actual two-element `(buffer, worlds_per_row)` result using a proper typing
annotation, and remove the constructor call from `wp.array()`. Preserve the
existing return behavior.
In `@newton/_src/sim/builder.py`:
- Around line 7186-7238: Update the cfg argument documentation in
add_shape_camera to state that a supplied configuration is copied, forced to
site behavior, and made invisible before use. Keep the existing description of
the cfg=None fallback and align the wording with the unconditional mark_as_site
and is_visible assignment.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: fbcae228-f701-4324-8944-3aaaf5e2c599
📒 Files selected for processing (34)
CHANGELOG.mddocs/api/newton.rstdocs/api/newton_geometry.rstdocs/api/newton_sensors.rstnewton/__init__.pynewton/_src/geometry/__init__.pynewton/_src/geometry/inertia.pynewton/_src/geometry/types.pynewton/_src/geometry/utils.pynewton/_src/sensors/__init__.pynewton/_src/sensors/camera_sensor.pynewton/_src/sensors/camera_sensor_renderer/__init__.pynewton/_src/sensors/camera_sensor_renderer/camera_utils.pynewton/_src/sensors/camera_sensor_renderer/gaussians.pynewton/_src/sensors/camera_sensor_renderer/lighting.pynewton/_src/sensors/camera_sensor_renderer/raytrace.pynewton/_src/sensors/camera_sensor_renderer/render.pynewton/_src/sensors/camera_sensor_renderer/render_context.pynewton/_src/sensors/camera_sensor_renderer/textures.pynewton/_src/sensors/camera_sensor_renderer/tiling.pynewton/_src/sensors/camera_sensor_renderer/types.pynewton/_src/sensors/camera_sensor_renderer/utils.pynewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/utils/import_mjcf.pynewton/_src/utils/import_usd.pynewton/_src/viewer/viewer.pynewton/examples/sensors/example_camera_sensor.pynewton/geometry.pynewton/sensors.pynewton/tests/test_camera_sensor.pynewton/tests/test_examples.pynewton/tests/test_import_mjcf_cameras.pynewton/tests/test_import_usd_cameras.py
daniela-hase
force-pushed
the
dev/camera-sensor
branch
from
July 24, 2026 01:02
04b5bf9 to
e80f106
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/_src/sensors/camera_sensor_renderer/utils.py (1)
791-791: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueFix the return annotation
-> wp.array():.
wp.array()is a call expression, not a type. It's inert today only becausefrom __future__ import annotationskeeps it unevaluated, but it misleads readers and type checkers. This helper returns a(buffer, worlds_per_row)tuple.Proposed annotation fix
- ) -> wp.array(): + ) -> tuple[wp.array[wp.uint8], int]:🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor_renderer/utils.py` at line 791, Update the return annotation of the helper ending near `wp.array()` to describe its actual `(buffer, worlds_per_row)` tuple result using valid type syntax; remove the call expression and preserve the existing implementation behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/sensors/camera_sensor_renderer/utils.py`:
- Around line 135-139: Resolve the clamp-versus-wrap behavior in the camera
normalization logic and remove the reviewer-directed NOTE(reviewers) comment.
Preserve the existing clamp behavior, and if needed replace it with at most a
concise one-line rationale; do not leave an open design question or
tracking-request text in the kernel body.
---
Nitpick comments:
In `@newton/_src/sensors/camera_sensor_renderer/utils.py`:
- Line 791: Update the return annotation of the helper ending near `wp.array()`
to describe its actual `(buffer, worlds_per_row)` tuple result using valid type
syntax; remove the call expression and preserve the existing implementation
behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 078f39e2-e5bc-4bb5-9dd2-db3be4aa9297
📒 Files selected for processing (34)
CHANGELOG.mddocs/api/newton.rstdocs/api/newton_geometry.rstdocs/api/newton_sensors.rstnewton/__init__.pynewton/_src/geometry/__init__.pynewton/_src/geometry/inertia.pynewton/_src/geometry/types.pynewton/_src/geometry/utils.pynewton/_src/sensors/__init__.pynewton/_src/sensors/camera_sensor.pynewton/_src/sensors/camera_sensor_renderer/__init__.pynewton/_src/sensors/camera_sensor_renderer/camera_utils.pynewton/_src/sensors/camera_sensor_renderer/gaussians.pynewton/_src/sensors/camera_sensor_renderer/lighting.pynewton/_src/sensors/camera_sensor_renderer/raytrace.pynewton/_src/sensors/camera_sensor_renderer/render.pynewton/_src/sensors/camera_sensor_renderer/render_context.pynewton/_src/sensors/camera_sensor_renderer/textures.pynewton/_src/sensors/camera_sensor_renderer/tiling.pynewton/_src/sensors/camera_sensor_renderer/types.pynewton/_src/sensors/camera_sensor_renderer/utils.pynewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/utils/import_mjcf.pynewton/_src/utils/import_usd.pynewton/_src/viewer/viewer.pynewton/examples/sensors/example_camera_sensor.pynewton/geometry.pynewton/sensors.pynewton/tests/test_camera_sensor.pynewton/tests/test_examples.pynewton/tests/test_import_mjcf_cameras.pynewton/tests/test_import_usd_cameras.py
🚧 Files skipped from review as they are similar to previous changes (28)
- docs/api/newton_geometry.rst
- docs/api/newton_sensors.rst
- newton/_src/sensors/init.py
- docs/api/newton.rst
- newton/_src/geometry/init.py
- newton/_src/geometry/utils.py
- newton/_src/sensors/camera_sensor_renderer/init.py
- newton/sensors.py
- newton/geometry.py
- newton/_src/geometry/inertia.py
- newton/_src/geometry/types.py
- newton/_src/sensors/camera_sensor_renderer/lighting.py
- newton/_src/sensors/camera_sensor_renderer/tiling.py
- newton/tests/test_examples.py
- newton/_src/sensors/camera_sensor_renderer/textures.py
- newton/_src/viewer/viewer.py
- CHANGELOG.md
- newton/tests/test_import_mjcf_cameras.py
- newton/_src/sensors/camera_sensor_renderer/types.py
- newton/tests/test_camera_sensor.py
- newton/tests/test_import_usd_cameras.py
- newton/_src/sensors/camera_sensor_renderer/raytrace.py
- newton/_src/sensors/camera_sensor_renderer/render.py
- newton/_src/sensors/camera_sensor_renderer/gaussians.py
- newton/_src/sensors/camera_sensor_renderer/render_context.py
- newton/_src/sensors/camera_sensor.py
- newton/_src/sim/builder.py
- newton/_src/sensors/camera_sensor_renderer/camera_utils.py
daniela-hase
force-pushed
the
dev/camera-sensor
branch
from
July 24, 2026 22:16
e80f106 to
b719f82
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (13)
newton/_src/sensors/camera_sensor.py (3)
621-624: 🚀 Performance & Scalability | 🔵 Trivial | 💤 Low valueFail fast before initializing the render context.
init_render_context()builds the shared context and loads textures on first call; if this sensor was never finalized,_update_transformsraises immediately afterwards, so that work is wasted. Validate first.♻️ Proposed reorder
- render_context = model.init_render_context() self._update_transforms(model, state) + render_context = model.init_render_context() if world_render_flags is None: world_render_flags = self._all_world_render_flags🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor.py` around lines 621 - 624, Reorder the logic in the sensor rendering method so self._update_transforms(model, state) runs before model.init_render_context(). Preserve the existing world_render_flags defaulting behavior, and ensure transform validation fails before render-context initialization and texture loading.
232-232: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPublic ray-generation APIs lack Google-style
Args:blocks and SI units. All five static factories onCameraSensorcarry a single summary line, so callers get no documented units for angles, intrinsics, or aperture parameters — the shared root cause is missing parameter documentation on the newly public surface.
newton/_src/sensors/camera_sensor.py#L232-L232: documentcamera_fov[rad],focal_length/aperture/offset params [mm],out_rays,device, and that aperture params are all-or-nothing.newton/_src/sensors/camera_sensor.py#L287-L287: documentcamera(UsdGeom.Camera),time(USD time code),out_rays,device.newton/_src/sensors/camera_sensor.py#L320-L320: documentfx/fy/cx/cyandimage_width/image_height[px],k1–k4(dimensionless),max_fov[rad].newton/_src/sensors/camera_sensor.py#L371-L371: documentoptical_center_x/optical_center_y[px],image_*/nominal_*[px] with the must-match rule,k0–k4,max_fov[rad].newton/_src/sensors/camera_sensor.py#L420-L420: documentoptical_center_*[px],image_*/nominal_*[px],k0–k3,max_fov[rad].As per coding guidelines: "Use Google-style docstrings, keep types in annotations, format
Args:entries asname: description" and "Include SI units for physical quantities in public API docstrings".🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor.py` at line 232, Expand the Google-style docstrings for all five public static ray-generation factories on CameraSensor with Args: entries, keeping types in annotations. At newton/_src/sensors/camera_sensor.py lines 232-232, document camera_fov [rad], focal_length/aperture/offset parameters [mm], out_rays, device, and the all-or-nothing aperture requirement; at lines 287-287, document camera as UsdGeom.Camera, time as a USD time code, out_rays, and device; at lines 320-320, document fx/fy/cx/cy and image dimensions [px], dimensionless k1-k4, and max_fov [rad]; at lines 371-371, document optical centers and image/nominal dimensions [px] including their must-match rule, k0-k4, and max_fov [rad]; and at lines 420-420, document optical centers and image/nominal dimensions [px], k0-k3, and max_fov [rad].Source: Coding guidelines
118-118: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueUse an explicit empty array constructor.
wp.array([], dtype=wp.int32, ...)is not a clearly supported way to create a shape-free Warp array; allocate the finalshape_indicesarray explicitly with size 0 instead.♻️ Proposed change
- self.shape_indices = wp.array([], dtype=wp.int32, device=self.rays.device) + self.shape_indices = wp.zeros(0, dtype=wp.int32, device=self.rays.device)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor.py` at line 118, Update the shape_indices initialization in the camera sensor setup to use Warp’s explicit zero-length array allocation with int32 dtype and the existing rays device, rather than constructing it from an empty Python list.newton/_src/sensors/camera_sensor_renderer/utils.py (2)
434-435: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winValidate the
wp.arraydepth_rangelike the tuple path.The kernel indexes elements 0 and 1, so a shorter array or one on another device produces an out-of-bounds device read or a launch failure, while the tuple branch below is fully validated.
♻️ Suggested guard
elif isinstance(depth_range, wp.array): + if depth_range.shape != (2,) or depth_range.dtype != wp.float32 or depth_range.device != device: + raise ValueError( + "to_rgba_from_depth: depth_range array must have shape (2,), dtype wp.float32, " + f"and be on {device}" + ) depth_range_arr = depth_range🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor_renderer/utils.py` around lines 434 - 435, Validate the wp.array depth_range in the depth-range handling branch before assigning depth_range_arr, matching the tuple path: require at least two elements and ensure it resides on the expected device. Reject invalid arrays with the same established error behavior used for invalid tuple inputs, while preserving valid-array handling.
52-57: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winMirror the clamp used in
unpack_normal_to_rgba_kernel.Out-of-range normals wrap through the
wp.uint8conversion here, whereas Lines 133-135 clamp for that exact reason.♻️ Suggested change
- normal = normal_image[world_id, y, x] * 0.5 + wp.vec3f(0.5) - - buffer[py, px, 0] = wp.uint8(normal[0] * 255.0) - buffer[py, px, 1] = wp.uint8(normal[1] * 255.0) - buffer[py, px, 2] = wp.uint8(normal[2] * 255.0) + normal = normal_image[world_id, y, x] * 0.5 + wp.vec3f(0.5) + + buffer[py, px, 0] = wp.uint8(wp.int32(wp.clamp(normal[0], 0.0, 1.0) * 255.0)) + buffer[py, px, 1] = wp.uint8(wp.int32(wp.clamp(normal[1], 0.0, 1.0) * 255.0)) + buffer[py, px, 2] = wp.uint8(wp.int32(wp.clamp(normal[2], 0.0, 1.0) * 255.0)) buffer[py, px, 3] = wp.uint8(255)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor_renderer/utils.py` around lines 52 - 57, Update the normal-to-RGBA conversion in the affected kernel to clamp each scaled normal component to the valid 0–255 range before converting with wp.uint8, matching the existing clamp behavior in unpack_normal_to_rgba_kernel. Preserve the alpha assignment and apply the same handling to all three RGB channels.newton/_src/sensors/camera_sensor_renderer/render_context.py (4)
121-128: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueTrim the heightfield comment and its duplicate at Line 443.
The same point is made twice (here and inline in the launch args). Keep one short "why".
As per path instructions: "comments that restate what the code already shows, repeat the same point in multiple places, or narrate obvious steps" should be flagged.♻️ Suggested trim
- # Heightfields are triangulated meshes (their wp.Mesh lives in - # shape_source_ptr), so the renderer treats them as meshes: it reuses - # the MESH ray-intersection path, which keeps heightfield handling out - # of the render kernels entirely (no extra shape-type branch, so no - # register/occupancy cost). The remapped type array is what the render - # kernel dispatches on; model.shape_type (HFIELD) is left untouched for - # collision and BVH bounds. + # Heightfields carry a wp.Mesh in shape_source_ptr, so the renderer + # dispatches them through the MESH path; model.shape_type stays HFIELD + # for collision and BVH bounds.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor_renderer/render_context.py` around lines 121 - 128, Trim the verbose heightfield explanation above self.shape_render_type in the render context, keeping only one concise statement explaining why heightfields use the mesh render path. Remove the duplicate explanation near the launch arguments around the corresponding inline comment, while leaving the rendering behavior unchanged.Sources: Coding guidelines, Path instructions
342-390: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winPrefer explicit exceptions over
assertfor user-supplied buffer validation.These checks guard public input reached via
CameraSensor.update(newton/_src/sensors/camera_sensor.pyLines 621-642). Underpython -Othey are stripped, and a mismatched buffer then becomes an out-of-bounds kernel write. The BVH preconditions above already raiseRuntimeError; raisingValueErrorhere would be consistent and always active.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor_renderer/render_context.py` around lines 342 - 390, Replace the user-supplied buffer validation asserts in the camera rendering validation block with always-active ValueError checks, preserving each existing shape, dtype, and device condition and message. Apply this to camera_transforms, camera_rays, world_render_flags, and all optional image buffers, while leaving the existing validation semantics unchanged.
410-414: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winKey the kernel cache on the config tuple, not its hash.
Storing only
hash(...)makes a hash collision silently dispatch a kernel specialized for a different config/clear-data combination. Since all three components are hashable, use the tuple directly so equality decides.♻️ Proposed change
- kernel_cache_key = hash((config, self.state, clear_data)) + kernel_cache_key = (config, self.state, clear_data)Also update the annotation on Line 52:
self.kernel_cache: dict[tuple[RenderConfig, RenderContext.State, ClearData], wp.Kernel] = {}Note
self.stateis a mutableunsafe_hashdataclass, so it must not be mutated after being used as a key; copying it into the key (e.g.dataclasses.replace(self.state)) would be safer still.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor_renderer/render_context.py` around lines 410 - 414, Update the kernel cache annotation and access in the renderer initialization and kernel creation flow to use the full tuple (config, state, clear_data) as the key instead of its hash, preserving equality-based collision safety. In the cache logic around create_kernel, use a copied state via dataclasses.replace(self.state) before keying if needed to prevent later mutation of the unsafe-hash state key.
580-581: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsolidate texture/mesh bookkeeping onto one attribute.
__load_texture_and_mesh_datapopulatesself.__mesh_data/self.__texture_data(created here, not in__init__), while__init__andinit_from_modelmaintain a separateself._texture_data_source. As a result_texture_data_sourcestays empty after model textures load and only reflectsassign_checkerboard_material, and the keep-alive references for the loadedwp.Texture2Dobjects live under a different name than the checkerboard path uses.♻️ Suggested consolidation
- self.__mesh_data = [] - self.__texture_data = [] + self._mesh_data_source: list[MeshData] = [] + self._texture_data_source = []…and update the corresponding
len(...)/append(...)/wp.array(...)uses in this method.Also applies to: 639-643
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor_renderer/render_context.py` around lines 580 - 581, Consolidate texture and mesh bookkeeping onto the existing _texture_data_source attribute instead of maintaining separate __texture_data and __mesh_data state. Update __load_texture_and_mesh_data, including its len, append, and wp.array operations, to use the consolidated attributes, and align __init__, init_from_model, and assign_checkerboard_material so loaded texture references and checkerboard data share the same storage.newton/examples/sensors/example_camera_sensor.py (1)
404-421: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winUse
self.world_count_totalinstead of the literal 24.The world count is already derived (
worlds_per_row * worlds_per_col) and used at Line 429; the hardcoded 24 silently breaks these assertions if the grid changes.♻️ Proposed change
- expected_shape = (24, self.camera_sensor.height, self.camera_sensor.width) + expected_shape = (self.world_count_total, self.camera_sensor.height, self.camera_sensor.width) @@ - assert normal_image.shape == (24, self.camera_sensor.height, self.camera_sensor.width, 3) + assert normal_image.shape == (*expected_shape, 3)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/examples/sensors/example_camera_sensor.py` around lines 404 - 421, Replace the hardcoded world-count value 24 in the expected_shape and normal_image shape assertions within the sensor validation block with self.world_count_total, preserving the existing height, width, and channel dimensions.newton/_src/utils/import_mjcf.py (1)
1326-1341: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the shared default/ignore helpers.
is_ignored_class(Line 483) andresolve_element_attrib(Line 492) already implement this class-default merge and ignore-class check; inlining them here duplicates logic that the geom/site paths share.♻️ Suggested consolidation
for camera_index, camera in enumerate(parent_element.findall("camera")): - camera_defaults = incoming_defaults - if "class" in camera.attrib: - camera_class = camera.attrib["class"] - if any(re.match(pattern, camera_class) for pattern in ignore_classes): - continue - if camera_class in class_defaults: - camera_defaults = merge_attrib(incoming_defaults, class_defaults[camera_class]) - if "camera" in camera_defaults: - camera_attrib = merge_attrib(camera_defaults["camera"], camera.attrib) - else: - camera_attrib = camera.attrib + camera_class = camera.attrib.get("class") + if camera_class is not None and is_ignored_class(camera_class): + continue + camera_attrib = resolve_element_attrib(camera, "camera", incoming_defaults)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/utils/import_mjcf.py` around lines 1326 - 1341, Update the camera processing loop to reuse the shared is_ignored_class and resolve_element_attrib helpers for class filtering and default attribute resolution, removing the duplicated camera_class/class_defaults merge logic while preserving the existing camera ignore-name check and name fallback.newton/_src/utils/import_usd.py (2)
1233-1239: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winPrefer the aperture overload so the authored horizontal FOV survives.
Only
verticalAperturefeeds the FOV, so the horizontal FOV is implied by the fixed 4:3 ray grid and any authoredhorizontalAperture(including aperture offsets) is discarded.compute_camera_rays_pinholeacceptsfocal_length/horizontal_aperture/vertical_aperturedirectly (camera_sensor.pyLine 219).♻️ Suggested change
width, height = DEFAULT_CAMERA_RESOLUTION camera_rays = CameraSensor.compute_camera_rays_pinhole( width, height, - _usd_camera_fov(usd_camera, time_code), + focal_length=focal_length, + horizontal_aperture=horizontal_aperture, + vertical_aperture=vertical_aperture, device="cpu", )(keep
_usd_camera_fovas the fallback when the aperture attributes are unauthored or non-positive)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/utils/import_usd.py` around lines 1233 - 1239, Update the camera ray construction using CameraSensor.compute_camera_rays_pinhole to pass the authored focal length, horizontal aperture, and vertical aperture when those values are positive, preserving horizontal FOV and aperture offsets. Retain _usd_camera_fov as the fallback whenever the aperture attributes are unauthored or non-positive, while keeping the existing resolution and CPU device behavior.
1219-1225: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse
_get_rigid_body_ancestor_path.Line 813 already walks ancestors against
path_body_map; this loop duplicates it.♻️ Suggested consolidation
- body = -1 - ancestor = prim.GetParent() - while ancestor and ancestor.IsValid() and ancestor.GetPath() != stage.GetPseudoRoot().GetPath(): - ancestor_path = str(ancestor.GetPath()) - if ancestor_path in path_body_map: - body = path_body_map[ancestor_path] - break - ancestor = ancestor.GetParent() + ancestor_path = _get_rigid_body_ancestor_path(prim.GetParent()) + body = path_body_map.get(ancestor_path, -1) if ancestor_path is not None else -1🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/utils/import_usd.py` around lines 1219 - 1225, Replace the duplicated ancestor traversal in the surrounding import flow with the existing `_get_rigid_body_ancestor_path` helper already used near line 813. Pass the current prim and `path_body_map` as required, then use its returned ancestor path to retrieve the corresponding body while preserving the existing pseudo-root boundary behavior.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/sensors/camera_sensor_renderer/render_context.py`:
- Around line 327-341: Update the early-return path in the render method
containing the has_shapes/has_particles condition so supplied output images are
cleared using the configured clear_data values before returning, preserving the
documented clear-before-render contract for empty scenes. Ensure the same
shape/device validation performed for normal rendering is not bypassed on this
path.
- Around line 24-25: Rename the nested RenderContext.State dataclass to
RenderState and update all references to self.state and its construction
accordingly, while ensuring the update and render method annotations continue to
use the imported simulation State type.
In `@newton/_src/sensors/camera_sensor_renderer/utils.py`:
- Around line 92-95: Update the depth buffer write in the relevant utility
function so the RGB channels retain the grayscale value while the alpha channel
is always set to opaque 255. Match the alpha behavior of
unpack_depth_to_rgba_kernel and flatten_normal_image, ensuring both hit and miss
pixels remain opaque.
- Around line 110-117: Update convert_ray_depth_to_forward_depth to normalize
ray_dir_world before computing its dot product with cam_forward_world, while
preserving the existing zero-length guard and output behavior for invalid rays.
- Line 599: Update the return annotation of the nearby function in utils.py so
it describes the tuple actually returned rather than calling wp.array(). Use the
appropriate tuple element types already established by the function’s return
values, while leaving the implementation unchanged.
In `@newton/_src/sensors/camera_sensor.py`:
- Around line 519-520: Validate each shape_index in the shape_indices loop
before indexing shape_world, rejecting negative values (and preserving failure
for indices beyond the array bounds) so invalid indices cannot wrap to the final
world. Apply the guard immediately before the shape_world lookup in the code
handling world_index.
In `@newton/_src/utils/import_mjcf.py`:
- Around line 1343-1364: In the camera import flow before constructing the
sensor, inspect the MJCF camera attributes and warn when mode is present and not
“fixed”, or when orthographic is set to true. Keep the existing fixed pinhole
construction unchanged, but make the warnings clearly state that the authored
camera mode/projection is ignored.
In `@newton/_src/utils/import_usd.py`:
- Around line 1217-1231: Update the camera transform computation in the import
flow around cam_world_xform so it applies the articulation_root_xform rebase
when override_root_xform is enabled, before ancestor body lookup and
body-relative conversion. Ensure both body-attached and static cameras use the
final rebased pose consistently with articulated bodies and visual shapes.
---
Nitpick comments:
In `@newton/_src/sensors/camera_sensor_renderer/render_context.py`:
- Around line 121-128: Trim the verbose heightfield explanation above
self.shape_render_type in the render context, keeping only one concise statement
explaining why heightfields use the mesh render path. Remove the duplicate
explanation near the launch arguments around the corresponding inline comment,
while leaving the rendering behavior unchanged.
- Around line 342-390: Replace the user-supplied buffer validation asserts in
the camera rendering validation block with always-active ValueError checks,
preserving each existing shape, dtype, and device condition and message. Apply
this to camera_transforms, camera_rays, world_render_flags, and all optional
image buffers, while leaving the existing validation semantics unchanged.
- Around line 410-414: Update the kernel cache annotation and access in the
renderer initialization and kernel creation flow to use the full tuple (config,
state, clear_data) as the key instead of its hash, preserving equality-based
collision safety. In the cache logic around create_kernel, use a copied state
via dataclasses.replace(self.state) before keying if needed to prevent later
mutation of the unsafe-hash state key.
- Around line 580-581: Consolidate texture and mesh bookkeeping onto the
existing _texture_data_source attribute instead of maintaining separate
__texture_data and __mesh_data state. Update __load_texture_and_mesh_data,
including its len, append, and wp.array operations, to use the consolidated
attributes, and align __init__, init_from_model, and
assign_checkerboard_material so loaded texture references and checkerboard data
share the same storage.
In `@newton/_src/sensors/camera_sensor_renderer/utils.py`:
- Around line 434-435: Validate the wp.array depth_range in the depth-range
handling branch before assigning depth_range_arr, matching the tuple path:
require at least two elements and ensure it resides on the expected device.
Reject invalid arrays with the same established error behavior used for invalid
tuple inputs, while preserving valid-array handling.
- Around line 52-57: Update the normal-to-RGBA conversion in the affected kernel
to clamp each scaled normal component to the valid 0–255 range before converting
with wp.uint8, matching the existing clamp behavior in
unpack_normal_to_rgba_kernel. Preserve the alpha assignment and apply the same
handling to all three RGB channels.
In `@newton/_src/sensors/camera_sensor.py`:
- Around line 621-624: Reorder the logic in the sensor rendering method so
self._update_transforms(model, state) runs before model.init_render_context().
Preserve the existing world_render_flags defaulting behavior, and ensure
transform validation fails before render-context initialization and texture
loading.
- Line 232: Expand the Google-style docstrings for all five public static
ray-generation factories on CameraSensor with Args: entries, keeping types in
annotations. At newton/_src/sensors/camera_sensor.py lines 232-232, document
camera_fov [rad], focal_length/aperture/offset parameters [mm], out_rays,
device, and the all-or-nothing aperture requirement; at lines 287-287, document
camera as UsdGeom.Camera, time as a USD time code, out_rays, and device; at
lines 320-320, document fx/fy/cx/cy and image dimensions [px], dimensionless
k1-k4, and max_fov [rad]; at lines 371-371, document optical centers and
image/nominal dimensions [px] including their must-match rule, k0-k4, and
max_fov [rad]; and at lines 420-420, document optical centers and image/nominal
dimensions [px], k0-k3, and max_fov [rad].
- Line 118: Update the shape_indices initialization in the camera sensor setup
to use Warp’s explicit zero-length array allocation with int32 dtype and the
existing rays device, rather than constructing it from an empty Python list.
In `@newton/_src/utils/import_mjcf.py`:
- Around line 1326-1341: Update the camera processing loop to reuse the shared
is_ignored_class and resolve_element_attrib helpers for class filtering and
default attribute resolution, removing the duplicated
camera_class/class_defaults merge logic while preserving the existing camera
ignore-name check and name fallback.
In `@newton/_src/utils/import_usd.py`:
- Around line 1233-1239: Update the camera ray construction using
CameraSensor.compute_camera_rays_pinhole to pass the authored focal length,
horizontal aperture, and vertical aperture when those values are positive,
preserving horizontal FOV and aperture offsets. Retain _usd_camera_fov as the
fallback whenever the aperture attributes are unauthored or non-positive, while
keeping the existing resolution and CPU device behavior.
- Around line 1219-1225: Replace the duplicated ancestor traversal in the
surrounding import flow with the existing `_get_rigid_body_ancestor_path` helper
already used near line 813. Pass the current prim and `path_body_map` as
required, then use its returned ancestor path to retrieve the corresponding body
while preserving the existing pseudo-root boundary behavior.
In `@newton/examples/sensors/example_camera_sensor.py`:
- Around line 404-421: Replace the hardcoded world-count value 24 in the
expected_shape and normal_image shape assertions within the sensor validation
block with self.world_count_total, preserving the existing height, width, and
channel dimensions.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 1b82276c-0e83-4963-9988-786d146518bd
📒 Files selected for processing (35)
CHANGELOG.mdasv/benchmarks/simulation/bench_camera_sensor.pydocs/api/newton.rstdocs/api/newton_geometry.rstdocs/api/newton_sensors.rstnewton/__init__.pynewton/_src/geometry/__init__.pynewton/_src/geometry/inertia.pynewton/_src/geometry/types.pynewton/_src/geometry/utils.pynewton/_src/sensors/__init__.pynewton/_src/sensors/camera_sensor.pynewton/_src/sensors/camera_sensor_renderer/__init__.pynewton/_src/sensors/camera_sensor_renderer/camera_utils.pynewton/_src/sensors/camera_sensor_renderer/gaussians.pynewton/_src/sensors/camera_sensor_renderer/lighting.pynewton/_src/sensors/camera_sensor_renderer/raytrace.pynewton/_src/sensors/camera_sensor_renderer/render.pynewton/_src/sensors/camera_sensor_renderer/render_context.pynewton/_src/sensors/camera_sensor_renderer/textures.pynewton/_src/sensors/camera_sensor_renderer/tiling.pynewton/_src/sensors/camera_sensor_renderer/types.pynewton/_src/sensors/camera_sensor_renderer/utils.pynewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/utils/import_mjcf.pynewton/_src/utils/import_usd.pynewton/_src/viewer/viewer.pynewton/examples/sensors/example_camera_sensor.pynewton/geometry.pynewton/sensors.pynewton/tests/test_camera_sensor.pynewton/tests/test_examples.pynewton/tests/test_import_mjcf_cameras.pynewton/tests/test_import_usd_cameras.py
🚧 Files skipped from review as they are similar to previous changes (24)
- docs/api/newton_geometry.rst
- docs/api/newton_sensors.rst
- newton/_src/geometry/types.py
- newton/_src/sensors/init.py
- newton/init.py
- newton/_src/sensors/camera_sensor_renderer/init.py
- CHANGELOG.md
- newton/_src/geometry/init.py
- newton/tests/test_examples.py
- newton/_src/sensors/camera_sensor_renderer/lighting.py
- newton/_src/sensors/camera_sensor_renderer/textures.py
- newton/sensors.py
- newton/_src/geometry/utils.py
- newton/_src/geometry/inertia.py
- newton/_src/sim/model.py
- newton/_src/sensors/camera_sensor_renderer/types.py
- newton/_src/sensors/camera_sensor_renderer/tiling.py
- newton/tests/test_camera_sensor.py
- newton/_src/sensors/camera_sensor_renderer/raytrace.py
- newton/_src/viewer/viewer.py
- newton/_src/sensors/camera_sensor_renderer/render.py
- newton/_src/sensors/camera_sensor_renderer/gaussians.py
- newton/_src/sim/builder.py
- newton/_src/sensors/camera_sensor_renderer/camera_utils.py
daniela-hase
force-pushed
the
dev/camera-sensor
branch
from
July 24, 2026 22:32
b719f82 to
fc4065e
Compare
Contributor
There was a problem hiding this comment.
Actionable comments posted: 5
♻️ Duplicate comments (1)
newton/_src/sensors/camera_sensor.py (1)
508-544: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winNegative
shape_indexstill silently wraps to the wrong world.
shape_world[int(shape_index)]at line 520 uses NumPy indexing, so a negativeshape_indexwraps to the end of the array and produces a plausible-but-wrong world mapping instead of failing. This mirrors an unresolved comment from a previous review commit.🛡️ Proposed guard
for shape_index in shape_indices: - world_index = int(shape_world[int(shape_index)]) + shape_index = int(shape_index) + if not 0 <= shape_index < len(shape_world): + raise RuntimeError(f"CameraSensor references invalid shape index {shape_index}.") + world_index = int(shape_world[shape_index])🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor.py` around lines 508 - 544, Validate each shape_index in _compute_shape_index_by_world before indexing shape_world, rejecting negative indices with a clear RuntimeError instead of allowing NumPy’s negative-index wrapping. Keep the existing world-range validation and mapping behavior unchanged for non-negative indices.
🧹 Nitpick comments (5)
newton/_src/utils/import_mjcf.py (3)
1327-1342: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winReuse the existing default/class-resolution helpers.
resolve_element_attrib(camera, "camera", incoming_defaults)(line 492) already performs the class-default merge implemented here, andis_ignored_class(line 483) already wraps theignore_classesregex loop._parse_sites_implfollows exactly that pattern, so mirroring it removes the duplicated resolution logic.♻️ Proposed simplification
- camera_defaults = incoming_defaults - if "class" in camera.attrib: - camera_class = camera.attrib["class"] - if any(re.match(pattern, camera_class) for pattern in ignore_classes): - continue - if camera_class in class_defaults: - camera_defaults = merge_attrib(incoming_defaults, class_defaults[camera_class]) - if "camera" in camera_defaults: - camera_attrib = merge_attrib(camera_defaults["camera"], camera.attrib) - else: - camera_attrib = camera.attrib + camera_class = camera.attrib.get("class") + if camera_class is not None and is_ignored_class(camera_class): + continue + camera_attrib = resolve_element_attrib(camera, "camera", incoming_defaults)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/utils/import_mjcf.py` around lines 1327 - 1342, Update the camera loop to reuse the existing is_ignored_class and resolve_element_attrib helpers instead of duplicating ignore-class matching and class-default merging. Follow the established _parse_sites_impl pattern, while preserving camera-name filtering and the existing camera attribute/name behavior.
2031-2032: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winNo way to opt out of MJCF camera import.
parse_usdgainedload_cameras: bool = True, butparse_mjcfunconditionally creates camera shapes here (and at the frame/worldbody call sites, lines 1577 and 2504). Existing MJCF assets with<camera>elements now gain extra shapes with no importer switch, unlikeparse_sites/parse_visuals. Consider a matchingload_cameras/parse_camerasparameter for symmetry with the USD path.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/utils/import_mjcf.py` around lines 2031 - 2032, Introduce a load_cameras/parse_cameras opt-out parameter for the MJCF parsing flow, matching the existing USD and parse_sites/parse_visuals behavior. Thread the parameter through parse_mjcf and its frame/worldbody camera call sites, including _parse_cameras_impl, and skip camera shape creation when disabled while preserving the current default-enabled behavior.
162-162: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicated
DEFAULT_CAMERA_RESOLUTIONconstant in both importers. The same(640, 480)default is now defined independently in two modules, so the two import paths can drift apart silently.
newton/_src/utils/import_mjcf.py#L162: import the shared default instead of redefining it locally.newton/_src/utils/import_usd.py#L73: move the constant to a single owner (e.g. alongsideCameraSensor) and import it here.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/utils/import_mjcf.py` at line 162, The camera resolution default is duplicated across both importer modules. In newton/_src/utils/import_usd.py at lines 73-73, make the shared constant owned by the camera-related implementation (such as alongside CameraSensor); in newton/_src/utils/import_mjcf.py at lines 162-162, remove the local definition and import that shared DEFAULT_CAMERA_RESOLUTION instead.asv/benchmarks/simulation/bench_camera_sensor.py (1)
311-313: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAnnotate the ASV class attributes as
ClassVarto satisfy RUF012.Ruff flags the mutable class-level default.
ClassVarkeeps the ASV-required attribute names while silencing the rule (also applies to theparamstuples in the subclasses).♻️ Proposed annotation
- param_names = ["resolution", "world_count", "iterations"] + param_names: ClassVar[list[str]] = ["resolution", "world_count", "iterations"] scene: str render_order = RENDER_ORDERAdd
from typing import ClassVarto the imports.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@asv/benchmarks/simulation/bench_camera_sensor.py` around lines 311 - 313, Import ClassVar and annotate the mutable ASV class attributes, including param_names, render_order, and the params tuples in subclasses, as ClassVar while preserving their existing values and required attribute names.Source: Linters/SAST tools
newton/_src/sensors/camera_sensor.py (1)
219-447: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winPublic ray-generation methods lack
Args:docstring sections.
compute_camera_rays_pinhole,compute_camera_rays_usd_pinhole,compute_camera_rays_fisheye_opencv,compute_camera_rays_fisheye_ftheta, andcompute_camera_rays_fisheye_kannala_brandtare public API surfaces with 5-14 parameters each (many with non-obvious units likecamera_fov,k1-k4,max_fov), but each only has a one-line summary docstring with noArgs:entries.As per coding guidelines, "Use Google-style docstrings...format
Args:entries asname: description" and "Include SI units for physical quantities in public API docstrings."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/sensors/camera_sensor.py` around lines 219 - 447, Expand the docstrings for compute_camera_rays_pinhole, compute_camera_rays_usd_pinhole, compute_camera_rays_fisheye_opencv, compute_camera_rays_fisheye_ftheta, and compute_camera_rays_fisheye_kannala_brandt with Google-style Args sections covering every parameter. Document non-obvious meanings, distortion coefficients, field-of-view values, image dimensions, offsets, and physical quantities with SI units where applicable.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@newton/_src/sensors/camera_sensor.py`:
- Around line 237-261: Update the camera ray-selection logic around use_aperture
to reject calls that provide both camera_fov and any aperture parameters,
raising a clear ValueError before launching the aperture kernel. Preserve the
existing aperture completeness validation and the camera_fov-required behavior
when aperture parameters are absent.
- Around line 449-506: Resolve the effective device at the start of finalize,
using self.rays.device when device is None, and use that resolved device for all
shape_indices and render-buffer allocations. Update the wp.array, wp.empty, and
wp.full calls in finalize, while preserving the existing device-migration
behavior and world-count branching.
In `@newton/_src/utils/import_mjcf.py`:
- Around line 1305-1316: Update _parse_camera_fov to handle MuJoCo cameras using
focalpixel, sensorsize, and resolution instead of falling back to the 45°
default. Validate the required two-value attributes and compute the vertical FOV
from focalpixel and resolution using the sensor dimensions; if the intrinsic
combination cannot be supported, explicitly raise or warn rather than silently
using fovy.
In `@newton/_src/utils/import_usd.py`:
- Around line 1242-1248: The USD camera ray construction in the import flow
should preserve authored intrinsics instead of deriving only vertical FOV.
Update the call to CameraSensor.compute_camera_rays_pinhole to use resolved
focal length, horizontal/vertical aperture, and horizontal/vertical aperture
offsets when available, while retaining the existing fallback for unresolved
attributes.
In `@newton/examples/sensors/example_camera_sensor.py`:
- Around line 386-392: The _get_camera_transform method currently uses the
world-to-camera view rotation directly; transpose or otherwise invert the
extracted 3×3 block from get_view_matrix() before passing it to
wp.quat_from_matrix. Preserve the viewer camera position and fallback transform,
and ensure the resulting camera orientation matches the ViewerGL camera basis.
---
Duplicate comments:
In `@newton/_src/sensors/camera_sensor.py`:
- Around line 508-544: Validate each shape_index in
_compute_shape_index_by_world before indexing shape_world, rejecting negative
indices with a clear RuntimeError instead of allowing NumPy’s negative-index
wrapping. Keep the existing world-range validation and mapping behavior
unchanged for non-negative indices.
---
Nitpick comments:
In `@asv/benchmarks/simulation/bench_camera_sensor.py`:
- Around line 311-313: Import ClassVar and annotate the mutable ASV class
attributes, including param_names, render_order, and the params tuples in
subclasses, as ClassVar while preserving their existing values and required
attribute names.
In `@newton/_src/sensors/camera_sensor.py`:
- Around line 219-447: Expand the docstrings for compute_camera_rays_pinhole,
compute_camera_rays_usd_pinhole, compute_camera_rays_fisheye_opencv,
compute_camera_rays_fisheye_ftheta, and
compute_camera_rays_fisheye_kannala_brandt with Google-style Args sections
covering every parameter. Document non-obvious meanings, distortion
coefficients, field-of-view values, image dimensions, offsets, and physical
quantities with SI units where applicable.
In `@newton/_src/utils/import_mjcf.py`:
- Around line 1327-1342: Update the camera loop to reuse the existing
is_ignored_class and resolve_element_attrib helpers instead of duplicating
ignore-class matching and class-default merging. Follow the established
_parse_sites_impl pattern, while preserving camera-name filtering and the
existing camera attribute/name behavior.
- Around line 2031-2032: Introduce a load_cameras/parse_cameras opt-out
parameter for the MJCF parsing flow, matching the existing USD and
parse_sites/parse_visuals behavior. Thread the parameter through parse_mjcf and
its frame/worldbody camera call sites, including _parse_cameras_impl, and skip
camera shape creation when disabled while preserving the current default-enabled
behavior.
- Line 162: The camera resolution default is duplicated across both importer
modules. In newton/_src/utils/import_usd.py at lines 73-73, make the shared
constant owned by the camera-related implementation (such as alongside
CameraSensor); in newton/_src/utils/import_mjcf.py at lines 162-162, remove the
local definition and import that shared DEFAULT_CAMERA_RESOLUTION instead.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: c6995a43-6afe-4f41-81cd-b9e61733c2eb
📒 Files selected for processing (35)
CHANGELOG.mdasv/benchmarks/simulation/bench_camera_sensor.pydocs/api/newton.rstdocs/api/newton_geometry.rstdocs/api/newton_sensors.rstnewton/__init__.pynewton/_src/geometry/__init__.pynewton/_src/geometry/inertia.pynewton/_src/geometry/types.pynewton/_src/geometry/utils.pynewton/_src/sensors/__init__.pynewton/_src/sensors/camera_sensor.pynewton/_src/sensors/camera_sensor_renderer/__init__.pynewton/_src/sensors/camera_sensor_renderer/camera_utils.pynewton/_src/sensors/camera_sensor_renderer/gaussians.pynewton/_src/sensors/camera_sensor_renderer/lighting.pynewton/_src/sensors/camera_sensor_renderer/raytrace.pynewton/_src/sensors/camera_sensor_renderer/render.pynewton/_src/sensors/camera_sensor_renderer/render_context.pynewton/_src/sensors/camera_sensor_renderer/textures.pynewton/_src/sensors/camera_sensor_renderer/tiling.pynewton/_src/sensors/camera_sensor_renderer/types.pynewton/_src/sensors/camera_sensor_renderer/utils.pynewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/utils/import_mjcf.pynewton/_src/utils/import_usd.pynewton/_src/viewer/viewer.pynewton/examples/sensors/example_camera_sensor.pynewton/geometry.pynewton/sensors.pynewton/tests/test_camera_sensor.pynewton/tests/test_examples.pynewton/tests/test_import_mjcf_cameras.pynewton/tests/test_import_usd_cameras.py
🚧 Files skipped from review as they are similar to previous changes (29)
- newton/_src/sensors/init.py
- newton/_src/geometry/init.py
- docs/api/newton_sensors.rst
- newton/tests/test_examples.py
- docs/api/newton_geometry.rst
- newton/_src/sensors/camera_sensor_renderer/init.py
- newton/_src/geometry/utils.py
- newton/_src/geometry/inertia.py
- newton/_src/sensors/camera_sensor_renderer/lighting.py
- newton/_src/sensors/camera_sensor_renderer/tiling.py
- docs/api/newton.rst
- newton/_src/viewer/viewer.py
- CHANGELOG.md
- newton/sensors.py
- newton/_src/sensors/camera_sensor_renderer/textures.py
- newton/_src/geometry/types.py
- newton/_src/sensors/camera_sensor_renderer/types.py
- newton/tests/test_import_mjcf_cameras.py
- newton/geometry.py
- newton/_src/sim/model.py
- newton/tests/test_import_usd_cameras.py
- newton/_src/sensors/camera_sensor_renderer/render.py
- newton/tests/test_camera_sensor.py
- newton/_src/sensors/camera_sensor_renderer/gaussians.py
- newton/_src/sim/builder.py
- newton/_src/sensors/camera_sensor_renderer/raytrace.py
- newton/_src/sensors/camera_sensor_renderer/camera_utils.py
- newton/_src/sensors/camera_sensor_renderer/utils.py
- newton/_src/sensors/camera_sensor_renderer/render_context.py
Codecov Report❌ Patch coverage is 📢 Thoughts on this report? Let us know! |
daniela-hase
force-pushed
the
dev/camera-sensor
branch
from
July 24, 2026 23:02
fc4065e to
333cec9
Compare
daniela-hase
force-pushed
the
dev/camera-sensor
branch
from
July 24, 2026 23:08
333cec9 to
699fe54
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (3)
newton/_src/utils/import_mjcf.py (2)
1344-1359: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueReuse the existing class-default and ignore-class helpers.
resolve_element_attrib(camera, "camera", incoming_defaults)andis_ignored_class(camera_class)already implement exactly this merge/filter logic (see Lines 483-497), and every other element parser in this file goes through them. Hand-rolling it here duplicates the resolution rules and will drift if they change.♻️ Proposed refactor
- camera_defaults = incoming_defaults - if "class" in camera.attrib: - camera_class = camera.attrib["class"] - if any(re.match(pattern, camera_class) for pattern in ignore_classes): - continue - if camera_class in class_defaults: - camera_defaults = merge_attrib(incoming_defaults, class_defaults[camera_class]) - if "camera" in camera_defaults: - camera_attrib = merge_attrib(camera_defaults["camera"], camera.attrib) - else: - camera_attrib = camera.attrib + camera_class = camera.attrib.get("class") + if camera_class is not None and is_ignored_class(camera_class): + continue + camera_attrib = resolve_element_attrib(camera, "camera", incoming_defaults)🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/utils/import_mjcf.py` around lines 1344 - 1359, Update the camera parsing loop around camera_attrib to use the existing resolve_element_attrib(camera, "camera", incoming_defaults) helper for class-default merging and is_ignored_class(camera_class) for class filtering. Remove the duplicated class-default and ignore-class logic while preserving the existing camera-name filtering and fallback naming behavior.
1380-1395: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winNo way to opt out of MJCF camera import, and each camera allocates a full ray bundle.
parse_mjcfhas flags for every other optional entity (parse_sites,parse_visuals,parse_meshes) andparse_usdgainedload_cameras=Truein this same PR, but MJCF cameras are always parsed. Combined with theDEFAULT_CAMERA_RESOLUTIONfallback, every unnamed-resolution<camera>eagerly builds a 640×480 ray array (≈7 MB ofvec3fper camera) plus a distinctCameraSensor, so an MJCF with several cameras replicated across worlds pays that cost even when the caller never renders. Consider adding aparse_cameras: bool = Trueparameter (threaded throughModelBuilder.add_mjcf) for parity withload_cameras.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/_src/utils/import_mjcf.py` around lines 1380 - 1395, Add a parse_cameras: bool = True option to parse_mjcf and thread it through ModelBuilder.add_mjcf, then guard the MJCF camera parsing and ray-bundle/CameraSensor allocation under this option. Preserve the current camera import behavior when enabled and skip camera shapes entirely when disabled.newton/examples/sensors/example_camera_sensor.py (1)
345-352: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winPreallocate the color/albedo RGBA buffers like the other outputs.
to_rgba_from_colorallocates a fresh(24, 256, 256, 4)buffer on every frame for both color and albedo, while depth/normal/shape reuseout_buffer. Since examples get copied as templates, keeping the allocation out of the per-frame path is worth it.♻️ Proposed refactor
- color_rgba = utils.to_rgba_from_color(self.camera_sensor_color_image) - albedo_rgba = utils.to_rgba_from_color(self.camera_sensor_albedo_image) + utils.to_rgba_from_color(self.camera_sensor_color_image, out_buffer=self.color_rgba) + utils.to_rgba_from_color(self.camera_sensor_albedo_image, out_buffer=self.albedo_rgba)Add the buffers next to the existing ones in
__init__:self.color_rgba = wp.empty((n, H, W, 4), dtype=wp.uint8, device=device) self.albedo_rgba = wp.empty((n, H, W, 4), dtype=wp.uint8, device=device)and log
self.color_rgba/self.albedo_rgbabelow.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/examples/sensors/example_camera_sensor.py` around lines 345 - 352, Preallocate color and albedo RGBA buffers in the camera sensor class initializer, alongside the existing depth_rgba, normal_rgba, shape_rgba, and semantic_rgba buffers, using the sensor batch and image dimensions with uint8 storage on the configured device. Update the per-frame conversions in the camera sensor update method to pass color_rgba and albedo_rgba as out_buffer values, and include both buffers in the nearby logging/output setup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@newton/_src/utils/import_mjcf.py`:
- Around line 1344-1359: Update the camera parsing loop around camera_attrib to
use the existing resolve_element_attrib(camera, "camera", incoming_defaults)
helper for class-default merging and is_ignored_class(camera_class) for class
filtering. Remove the duplicated class-default and ignore-class logic while
preserving the existing camera-name filtering and fallback naming behavior.
- Around line 1380-1395: Add a parse_cameras: bool = True option to parse_mjcf
and thread it through ModelBuilder.add_mjcf, then guard the MJCF camera parsing
and ray-bundle/CameraSensor allocation under this option. Preserve the current
camera import behavior when enabled and skip camera shapes entirely when
disabled.
In `@newton/examples/sensors/example_camera_sensor.py`:
- Around line 345-352: Preallocate color and albedo RGBA buffers in the camera
sensor class initializer, alongside the existing depth_rgba, normal_rgba,
shape_rgba, and semantic_rgba buffers, using the sensor batch and image
dimensions with uint8 storage on the configured device. Update the per-frame
conversions in the camera sensor update method to pass color_rgba and
albedo_rgba as out_buffer values, and include both buffers in the nearby
logging/output setup.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: 3b8cb974-cff5-4a55-882f-acc0310e669a
📒 Files selected for processing (35)
CHANGELOG.mdasv/benchmarks/simulation/bench_camera_sensor.pydocs/api/newton.rstdocs/api/newton_geometry.rstdocs/api/newton_sensors.rstnewton/__init__.pynewton/_src/geometry/__init__.pynewton/_src/geometry/inertia.pynewton/_src/geometry/types.pynewton/_src/geometry/utils.pynewton/_src/sensors/__init__.pynewton/_src/sensors/camera_sensor.pynewton/_src/sensors/camera_sensor_renderer/__init__.pynewton/_src/sensors/camera_sensor_renderer/camera_utils.pynewton/_src/sensors/camera_sensor_renderer/gaussians.pynewton/_src/sensors/camera_sensor_renderer/lighting.pynewton/_src/sensors/camera_sensor_renderer/raytrace.pynewton/_src/sensors/camera_sensor_renderer/render.pynewton/_src/sensors/camera_sensor_renderer/render_context.pynewton/_src/sensors/camera_sensor_renderer/textures.pynewton/_src/sensors/camera_sensor_renderer/tiling.pynewton/_src/sensors/camera_sensor_renderer/types.pynewton/_src/sensors/camera_sensor_renderer/utils.pynewton/_src/sim/builder.pynewton/_src/sim/model.pynewton/_src/utils/import_mjcf.pynewton/_src/utils/import_usd.pynewton/_src/viewer/viewer.pynewton/examples/sensors/example_camera_sensor.pynewton/geometry.pynewton/sensors.pynewton/tests/test_camera_sensor.pynewton/tests/test_examples.pynewton/tests/test_import_mjcf_cameras.pynewton/tests/test_import_usd_cameras.py
🚧 Files skipped from review as they are similar to previous changes (28)
- docs/api/newton_sensors.rst
- docs/api/newton.rst
- newton/_src/geometry/init.py
- newton/sensors.py
- docs/api/newton_geometry.rst
- newton/_src/sensors/init.py
- newton/_src/viewer/viewer.py
- newton/_src/geometry/utils.py
- newton/_src/sensors/camera_sensor_renderer/init.py
- newton/init.py
- newton/_src/sensors/camera_sensor_renderer/tiling.py
- newton/_src/sensors/camera_sensor_renderer/textures.py
- newton/_src/sensors/camera_sensor_renderer/lighting.py
- newton/_src/geometry/types.py
- newton/tests/test_examples.py
- newton/_src/sensors/camera_sensor_renderer/types.py
- newton/geometry.py
- newton/_src/sensors/camera_sensor_renderer/raytrace.py
- newton/_src/sensors/camera_sensor_renderer/camera_utils.py
- newton/_src/sim/model.py
- newton/tests/test_camera_sensor.py
- newton/_src/sensors/camera_sensor_renderer/render.py
- newton/_src/sim/builder.py
- newton/_src/sensors/camera_sensor_renderer/gaussians.py
- newton/_src/sensors/camera_sensor_renderer/render_context.py
- newton/_src/sensors/camera_sensor_renderer/utils.py
- newton/_src/geometry/inertia.py
- newton/_src/sensors/camera_sensor.py
daniela-hase
force-pushed
the
dev/camera-sensor
branch
from
July 27, 2026 21:13
06a11bb to
22e43e0
Compare
daniela-hase
marked this pull request as ready for review
July 27, 2026 21:30
daniela-hase
force-pushed
the
dev/camera-sensor
branch
from
July 27, 2026 22:12
90cc9fc to
b635da2
Compare
daniela-hase
force-pushed
the
dev/camera-sensor
branch
from
July 29, 2026 20:56
d0de11b to
65ae967
Compare
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
1 participant
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.
Add a prototype
CameraSensorshape source that renders from camera shapes in the model instead of storing camera state as separate model attributes.This introduces
CameraSensoras a public sensor/geometry type, with camera rays, render settings, and shape indices owned by the sensor. Camera transforms now come from the existingmodel.shape_*arrays, so camera sensors behave like other shape-backed sources such as meshes and gaussians.Main changes:
CameraSensorand public exports vianewton,newton.geometry, andnewton.sensorsModelBuilderand model shape handlingcamera_sensor_renderercloned from the raytrace renderer and adapted for per-world camera outputCameraSensor.update()rendering with sharedmodel.render_contextChecklist
CHANGELOG.mdhas been updatedTest plan
Summary by CodeRabbit
CameraSensorwith pinhole and fisheye ray generation and GPU-accelerated rendering outputs (color/HDR, depth, forward depth, normals, albedo, shape-index).CameraSensor.