[Newton] Adds Pose, Velocity, Acceleration (PVA) sensor by camevor · Pull Request #5267 · isaac-sim/IsaacLab

camevor · 2026-04-14T12:49:21Z

Description

Implements a Pose, Velocity, Acceleration sensor.

blocked by #5264

isaaclab-review-bot

🤖 Isaac Lab Review Bot

Summary

This PR adds two new Newton-backend sensors: an IMU sensor wrapping Newton's SensorIMU, and a PVA (Pose, Velocity, Acceleration) sensor that reads body state directly from Newton's analytic simulation arrays (body_q, body_qd, body_qdd). The implementation is well-structured, follows existing patterns (FrameTransformer, PhysX PVA), and includes good test coverage. There is one consistency gap and a few items worth addressing.

Design Assessment

Design is sound. The architecture correctly mirrors the existing PhysX PVA pattern while leveraging Newton's analytic accelerations (body_qdd) instead of numerical differentiation — a meaningful accuracy improvement. The IMU wraps Newton's native SensorIMU with push-based updates in the physics loop, while PVA reads state lazily via the sensor framework's _update_outdated_buffers mechanism. Both approaches are appropriate for their respective sensors.

The site registration via NewtonManager.cl_register_site and the request_extended_state_attribute("body_qdd") pattern are clean and consistent with the existing FrameTransformer precedent. The invalidation/re-registration in _invalidate_initialize_callback correctly handles close/reinit cycles.

Findings

🟡 Warning: PVA factory type annotations not updated — source/isaaclab/isaaclab/sensors/pva/pva.py

The IMU factory (source/isaaclab/isaaclab/sensors/imu/imu.py) was correctly updated to include Newton types in the union annotations:

data: BaseImuData | PhysXImuData | NewtonImuData
def __new__(cls, ...) -> BaseImu | PhysXImu | NewtonImu:

However, the PVA factory (source/isaaclab/isaaclab/sensors/pva/pva.py) and PVA data factory (source/isaaclab/isaaclab/sensors/pva/pva_data.py) were not updated with Newton PVA types. For consistency and correct type checking:

# pva.py
if TYPE_CHECKING:
    from isaaclab_newton.sensors.pva import Pva as NewtonPva
    from isaaclab_newton.sensors.pva import PvaData as NewtonPvaData
    from isaaclab_physx.sensors.pva import Pva as PhysXPva
    from isaaclab_physx.sensors.pva import PvaData as PhysXPvaData

class Pva(FactoryBase, BasePva):
    data: BasePvaData | PhysXPvaData | NewtonPvaData
    def __new__(cls, *args, **kwargs) -> BasePva | PhysXPva | NewtonPva:
        ...

And similarly for pva_data.py. The CHANGELOG entry for isaaclab would also need updating to mention the PVA factory change.

🟡 Warning: wp.normalize on zero gravity vector produces NaN — source/isaaclab_newton/isaaclab_newton/sensors/pva/kernels.py:60

The projected gravity computation normalizes the raw gravity vector in-kernel:

g = gravity[wp.max(world_idx, 0)]
projected_gravity_b = wp.quat_rotate_inv(sensor_quat, wp.normalize(g))

If gravity is (0, 0, 0) (zero-g simulation), wp.normalize returns NaN. The PhysX PVA avoids this by receiving a pre-normalized direction vector from Python. Consider either:

Guarding with a length check: g_len = wp.length(g); g_dir = g / wp.max(g_len, 1e-8)
Or matching the PhysX pattern by normalizing on the Python side and passing a unit vector

This is an edge case (zero-g sims are rare), but NaN propagation in RL training is notoriously hard to debug.

🔵 Suggestion: Acceleration semantics difference should be documented — source/isaaclab_newton/isaaclab_newton/sensors/pva/pva.py:1

The Newton PVA sensor reports coordinate acceleration (from body_qdd), which includes gravitational acceleration during freefall. The PhysX PVA reports numerically differentiated acceleration, which converges to the same thing but with timestep-dependent accuracy. The tests correctly verify this (freefall test expects (0, 0, -9.81)), but the class docstring should explicitly note this semantic: "Accelerations are coordinate accelerations (not proper accelerations), meaning gravity is included." This helps users who switch between backends understand why freefall acceleration is non-zero.

🔵 Suggestion: request_extended_contact_attribute is added but unused — source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py:478

request_extended_contact_attribute() is added alongside request_extended_state_attribute(), presumably for future contact-related sensors. This is fine as forward-looking infrastructure, but since nothing in this PR calls it, consider whether it belongs in this PR or a follow-up to keep the change focused.

Test Coverage

Feature PR coverage: ✅ Adequate
Both sensors have dedicated test files (test_imu.py: 7 tests, test_pva.py: 10 tests)
Tests cover: initialization, data shapes, gravity-at-rest, velocity-at-rest, freefall, reset, sensor offset with rotation, and string representation
The test_offset_and_rotated_body test is particularly good — it validates the lever-arm correction and body-frame gravity projection with a 90° rotated body and offset sensor
The freefall tests correctly verify the coordinate acceleration semantics (≈ -9.81 m/s² in z)
Minor gap: No test for multi-step accumulation accuracy (e.g., integrating PVA velocity over time and comparing to position delta). Not blocking, but would strengthen confidence in the velocity transport formula.

CI Status

Pre-commit, linting, and link checks pass. Docker/docs builds still pending (infrastructure jobs, not related to this PR).

Verdict

Minor fixes needed

The implementation is correct and well-tested. The missing PVA factory type annotations are a consistency gap that should be addressed before merge (it's a 4-line change mirroring the IMU factory update). The zero-gravity NaN edge case and acceleration semantics documentation are worth considering but not blocking.

source/isaaclab_newton/isaaclab_newton/sensors/pva/kernels.py

source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py

camevor · 2026-04-14T13:11:14Z

@isaaclab-review-bot

Warning: PVA factory type annotations not updated

Fixed.

Warning: wp.normalize on zero gravity vector produces NaN

wp.normalize safely handles zero-length vectors.

Suggestion: Acceleration semantics difference should be documented

PhysX PVA also reports coordinate acceleration.

Suggestion: request_extended_contact_attribute is added but unused

Keeping it here for consistency. The contact sensor can be adapted to use it later.

AntoineRichard

Couple comments around some comments, docs, and visualization.

AntoineRichard · 2026-04-14T13:21:15Z

source/isaaclab/isaaclab/sensors/imu/base_imu.py

-    linear acceleration (accelerometer) in the sensor's body frame. Unlike the PVA
-    sensor, it does not provide pose, linear velocity, angular acceleration, or
-    projected gravity.
+    proper acceleration (accelerometer) in the sensor's body frame. Unlike the


why proper acceleration and not linear?

Good point, I'll add back "linear". Proper is to distinguish from coordinate acceleration that reflects the change in velocity due to gravity.

AntoineRichard · 2026-04-14T13:21:59Z

source/isaaclab/isaaclab/sensors/imu/base_imu_data.py

    @abstractmethod
    def lin_acc_b(self) -> wp.array:
-        """IMU frame linear acceleration relative to the world expressed in IMU frame [m/s^2].
+        """Proper linear acceleration in the IMU frame [m/s^2].


why proper? I feel like linear is more explicit than proper which could leave some room for doubt?

AntoineRichard · 2026-04-14T13:23:05Z

source/isaaclab/isaaclab/sensors/pva/base_pva.py

-    accelerations/velocities.
+    The sensor can be attached to any prim path with a rigid ancestor in its tree and produces world-frame
+    pose, body-frame velocities, body-frame coordinate accelerations, and projected gravity. This differs
+    from the :class:`~isaaclab.sensors.imu.BaseImu` sensor, which reports proper acceleration.


proper accelerations again. I'm not sure that's the way to go. Linear is more descriptive IMO.

Left as is here, because the focus in here is purely on the different convention.

AntoineRichard · 2026-04-14T13:25:11Z

source/isaaclab_newton/isaaclab_newton/sensors/imu/kernels.py

+@wp.kernel
+def imu_copy_kernel(
+    env_mask: wp.array(dtype=wp.bool),
+    accelerometer: wp.array(dtype=wp.vec3f),
+    gyroscope: wp.array(dtype=wp.vec3f),
+    out_lin_acc_b: wp.array(dtype=wp.vec3f),
+    out_ang_vel_b: wp.array(dtype=wp.vec3f),
+):
+    """Copy Newton SensorIMU outputs into owned IsaacLab buffers.
+
+    Launch with dim=num_envs. Sensor indices map 1:1 to environment indices.
+    """
+    idx = wp.tid()
+    if not env_mask[idx]:
+        return
+    out_lin_acc_b[idx] = accelerometer[idx]
+    out_ang_vel_b[idx] = gyroscope[idx]
+
+
+@wp.kernel
+def imu_reset_kernel(
+    env_mask: wp.array(dtype=wp.bool),
+    out_lin_acc_b: wp.array(dtype=wp.vec3f),
+    out_ang_vel_b: wp.array(dtype=wp.vec3f),
+):
+    """Zero out IMU data for reset environments.
+
+    Launch with dim=num_envs.
+    """
+    idx = wp.tid()
+    if not env_mask[idx]:
+        return
+    out_lin_acc_b[idx] = wp.vec3f(0.0, 0.0, 0.0)
+    out_ang_vel_b[idx] = wp.vec3f(0.0, 0.0, 0.0)


I'm wondering if we should have full docstrings on those?

AntoineRichard · 2026-04-14T13:25:51Z

source/isaaclab_newton/isaaclab_newton/sensors/pva/kernels.py

Same here, should we have the full dosctrings?

AntoineRichard · 2026-04-14T13:26:42Z

source/isaaclab_newton/isaaclab_newton/sensors/pva/pva.py

There is a visualization option for the PhysX PVA I think. Do we want to have that here too?

camevor added 8 commits April 14, 2026 11:44

Add Newton IMU sensor

f525280

Add Frame Transformer Sensor invalidation

6a00eb6

Fix changelog

29ff170

Fix changelog

63617ac

Add IMU package

dd157d9

Fix tests

a6c6841

Remove unused local

c9bf8ef

Add pva sensor

2172421

github-actions bot added the isaac-lab Related to Isaac Lab team label Apr 14, 2026

camevor self-assigned this Apr 14, 2026

camevor requested a review from AntoineRichard April 14, 2026 12:49

isaaclab-review-bot bot reviewed Apr 14, 2026

View reviewed changes

source/isaaclab_newton/isaaclab_newton/sensors/pva/kernels.py Show resolved Hide resolved

source/isaaclab_newton/isaaclab_newton/physics/newton_manager.py Show resolved Hide resolved

Add factory type annotations

a73ec0d

Improve IMU & PVA docstrings

ff31026

AntoineRichard reviewed Apr 14, 2026

View reviewed changes

camevor added 2 commits April 14, 2026 15:51

Improve wording in docstrings

7621c14

Address PR review

183ad34

Conversation

camevor commented Apr 14, 2026

Description

Uh oh!

isaaclab-review-bot bot left a comment

Choose a reason for hiding this comment

🤖 Isaac Lab Review Bot

Summary

Design Assessment

Findings

Test Coverage

CI Status

Verdict

Uh oh!

Uh oh!

Uh oh!

camevor commented Apr 14, 2026

Uh oh!

AntoineRichard left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants