Skip to content

Fixes Newton body_inertia#4921

Merged
kellyguo11 merged 5 commits intoisaac-sim:developfrom
AntoineRichard:antoiner/fix/newton_body_inertia
Mar 11, 2026
Merged

Fixes Newton body_inertia#4921
kellyguo11 merged 5 commits intoisaac-sim:developfrom
AntoineRichard:antoiner/fix/newton_body_inertia

Conversation

@AntoineRichard
Copy link
Copy Markdown
Collaborator

Description

  • Fixed ArticulationData.body_inertia and RigidObjectData.body_inertia returning raw mat33f arrays instead of (N, B, 9) float32 as documented in the interface contract.
  • Removed unnecessary wp.clone() from RigidObjectCollectionData.body_inertia, making all three asset data classes consistently zero-copy.

Type of change

  • Bug fix (non-breaking change which fixes an issue)

Checklist

  • I have read and understood the contribution guidelines
  • I have run the pre-commit checks with ./isaaclab.sh --format
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • I have updated the changelog and the corresponding version in the extension's config/extension.toml file
  • I have added my name to the CONTRIBUTORS.md or my name already exists there

…reinterpretation

The previous fix assumed get_attribute("body_inertia") returned float32
with ndim==4, but Newton returns mat33f with ndim==2. The ndim guard
always failed, returning raw mat33f arrays instead of (N, B, 9) float32.

Use view(wp.float32).reshape() which correctly reinterprets mat33f as
(3, 3) float32 then flattens to 9 — zero-copy, no clone needed.
The clone created a detached copy, inconsistent with Articulation and
RigidObject which use zero-copy view().reshape(). The view+reshape
chain on the sliced mat33f array is already zero-copy.
@github-actions github-actions Bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Mar 10, 2026
@greptile-apps
Copy link
Copy Markdown
Contributor

greptile-apps Bot commented Mar 10, 2026

Greptile Summary

This PR fixes body_inertia returning incorrect data in ArticulationData and RigidObjectData by replacing a ptr-based reshape (which assumed float32 dtype and ndim==4) with the correct view(wp.float32).reshape(N, B, 9) chain that properly handles Newton's actual mat33f dtype (ndim==2). It also removes the now-unnecessary wp.clone() from RigidObjectCollectionData, making all three asset data classes consistently zero-copy.

  • Root cause fixed: The old guard if _body_inertia_raw.ndim == 4 never triggered because Newton returns mat33f arrays with ndim==2, causing the fallback path to return a raw mat33f array with the wrong shape/dtype.
  • New approach: view(wp.float32) correctly reinterprets each mat33f element as 9 float32 scalars; reshape((N, B, 9)) then names the resulting shape. Both operations are zero-copy.
  • wp.clone() removal (rigid_object_collection_data.py): After [:,:,0] squeezes a trailing dimension of size 1, the result is already C-contiguous, so the clone was redundant. This makes write-back semantics consistent with the other two classes.
  • Tests: Shape assertions for body_inertia ((N, B, 9)) have been added to test_articulation.py, test_rigid_object.py, and test_rigid_object_collection.py, covering all three asset types.

Confidence Score: 5/5

  • This PR is safe to merge — it fixes a clearly broken code path with a clean, well-understood zero-copy reinterpretation, backed by new shape-assertion tests.
  • The fix is targeted and correct: the old ndim==4 guard never matched Newton's actual mat33f (ndim==2) output, so the else-branch always returned wrong-typed data. The new view(wp.float32).reshape() chain is the idiomatic Warp approach and is consistent across all three asset classes. Memory contiguity after the index squeezes is preserved (removing a trailing or middle dimension of size 1 maintains C-order strides). Tests cover the corrected shape for all three asset types.
  • No files require special attention.

Important Files Changed

Filename Overview
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation_data.py Replaces the old ptr-based workaround (which checked ndim==4 for a mat33f array that is actually ndim==2) with a correct zero-copy view(wp.float32).reshape(N, B, 9) chain. Minor comment inaccuracy about intermediate shape.
source/isaaclab_newton/isaaclab_newton/assets/rigid_object/rigid_object_data.py Same fix as articulation_data.py; correctly uses view(wp.float32).reshape((num_instances, 1, 9)). Hardcoded 1 for body count matches the single-body contract of RigidObject. Same minor comment inaccuracy about the intermediate (N,1,3,3) step.
source/isaaclab_newton/isaaclab_newton/assets/rigid_object_collection/rigid_object_collection_data.py Removes the previously redundant wp.clone() before view(wp.float32).reshape(...). After [:,:,0] on a (N, B, 1) mat33f array the result is already C-contiguous, so the clone was unnecessary. Now consistent with the zero-copy semantics of articulation and rigid_object.
source/isaaclab_newton/config/extension.toml Patch version bumped from 0.5.4 to 0.5.5, consistent with the bug-fix change type.
source/isaaclab_newton/docs/CHANGELOG.rst Changelog entry accurately describes the root cause (mat33f ndim==2 vs previously assumed float32 ndim==4) and the fix applied.

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["Newton model\nget_attribute('body_inertia')"] --> B["Raw wp.array\nshape: (N, P, B) mat33f\n(P=1 padding dim)"]
    B --> C["[:, 0] or [:, :, 0]\nSqueeze padding dim"]
    C --> D["(N, B) mat33f\nC-contiguous"]
    D --> E["view(wp.float32)\nReinterpret mat33f → 9×float32"]
    E --> F["(N, B, 9) float32\nzero-copy"]
    F --> G["reshape((N, B, 9))\nno-op / name shape"]
    G --> H["_sim_bind_body_inertia\nor _body_inertia\n(N, B, 9) float32"]
    H --> I["body_inertia property\nreturned to callers"]
    H --> J["write_body_inertia_to_buffer\nkernel output target\n(writes back to Newton model)"]

    style A fill:#f0f0f0
    style F fill:#d4edda
    style H fill:#d4edda
Loading

Last reviewed commit: 879c5e6

Comment on lines +1234 to +1235
# Newton stores body_inertia as (N, 1, B) mat33f — the [:, 0] removes the padding dim
# giving (N, B) mat33f. Reinterpret as (N, B, 3, 3) float32 then flatten to (N, B, 9).
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Slightly inaccurate intermediate-step comment

The comment says "Reinterpret as (N, B, 3, 3) float32 then flatten to (N, B, 9)", implying a 4-dimensional intermediate array. In practice, calling view(wp.float32) on a (N, B) mat33f array will produce a (N, B, 9) float32 array (since Warp adds one trailing dimension of size 9 for the 9 scalar components of mat33f), and the subsequent reshape((N, B, 9)) is then effectively a no-op rather than a flattening step. The same inaccuracy appears in rigid_object_data.py (line 723).

This is cosmetic and does not affect correctness, but the comment could mislead future readers into expecting a 4D intermediate array.

Suggested change
# Newton stores body_inertia as (N, 1, B) mat33f — the [:, 0] removes the padding dim
# giving (N, B) mat33f. Reinterpret as (N, B, 3, 3) float32 then flatten to (N, B, 9).
# Newton stores body_inertia as (N, 1, B) mat33f — the [:, 0] removes the padding dim
# giving (N, B) mat33f. view(wp.float32) reinterprets each mat33f as 9 float32 scalars,
# yielding (N, B, 9) float32 directly. Both view() and reshape() are zero-copy.

@kellyguo11 kellyguo11 changed the title Fix Newton body_inertia Fixes Newton body_inertia Mar 10, 2026
Signed-off-by: ooctipus <zhengyuz@nvidia.com>
@kellyguo11 kellyguo11 merged commit 9a6f7a8 into isaac-sim:develop Mar 11, 2026
11 of 14 checks passed
daniela-hase pushed a commit to daniela-hase/IsaacLab that referenced this pull request Mar 30, 2026
# Description

- Fixed ArticulationData.body_inertia and RigidObjectData.body_inertia
returning raw mat33f arrays instead of (N, B, 9) float32 as documented
in the interface contract.
- Removed unnecessary wp.clone() from
RigidObjectCollectionData.body_inertia, making all three asset data
classes consistently zero-copy.

## Type of change

- Bug fix (non-breaking change which fixes an issue)

## Checklist

- [x] I have read and understood the [contribution
guidelines](https://isaac-sim.github.io/IsaacLab/main/source/refs/contributing.html)
- [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with
`./isaaclab.sh --format`
- [x] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [x] I have added tests that prove my fix is effective or that my
feature works
- [x] I have updated the changelog and the corresponding version in the
extension's `config/extension.toml` file
- [x] I have added my name to the `CONTRIBUTORS.md` or my name already
exists there

---------

Signed-off-by: ooctipus <zhengyuz@nvidia.com>
Co-authored-by: ooctipus <zhengyuz@nvidia.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants