Skip to content

Conversation

@AntoineRichard
Copy link
Collaborator

Description

Test & Benchmark for newton articulation. Requires #4190 to be merged first.

Type of change

  • New feature (non-breaking change which adds functionality)
  • Documentation update

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

@github-actions github-actions bot added the isaac-lab Related to Isaac Lab team label Dec 16, 2025
@AntoineRichard AntoineRichard self-assigned this Dec 16, 2025
@AntoineRichard AntoineRichard marked this pull request as ready for review December 17, 2025 09:43
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 17, 2025

Greptile Summary

This PR adds comprehensive testing and benchmarking infrastructure for the Newton articulation system, along with critical bug fixes and refactoring improvements.

Key Changes

Testing Infrastructure (New)

  • Added 119 unit tests for Articulation class via mock interface
  • Added 52 unit tests for ArticulationData class
  • Added benchmark suites for performance tracking
  • Created mock Newton interfaces to enable testing without USD stage

Code Quality & Refactoring

  • Extracted torch-to-warp conversion logic into reusable utility functions (make_complete_data_from_torch_single_index, make_complete_data_from_torch_dual_index, make_masks_from_torch_ids)
  • Moved body/joint finding logic to shared utilities
  • Improved timestamped buffer usage with .assign() method instead of wp.copy()
  • Added is_primed property to prevent post-instantiation modifications

Critical Bug Fixes

  • Fixed sign error in CoM-to-link frame transformation (negated com_position in transform_CoM_pose_to_link_frame_masked_root:604)
  • Fixed velocity projection formulas in velocity_projector and velocity_projector_inv (corrected which component gets cross product applied)
  • Fixed wrench update functions to preserve existing components instead of zeroing them

Improvements

  • Added missing device parameters to wp.launch() calls throughout
  • Optimized state computation by caching output buffers instead of recreating
  • Combined separate split_state_to_pose and split_state_to_velocity kernels into single split_state_to_pose_and_velocity kernel

Issues Found

  • Multiple docstring copy-paste errors (says "set to True" for non-boolean properties)
  • Incorrect docstring parameters in make_masks_from_torch_ids
  • Missing newline at end of shared.py
  • Performance TODO indicates "atrocious" perf that needs addressing

Confidence Score: 4/5

  • Safe to merge with minor documentation fixes needed
  • The PR includes extensive testing (171 tests total), fixes critical physics bugs, and improves code quality through refactoring. The changes are well-structured and the test coverage is excellent. Minor documentation issues don't impact functionality.
  • source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation_data.py - has multiple docstring errors that should be fixed before merge

Important Files Changed

Filename Overview
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py Major refactoring: extracted torch-to-warp conversions to utility functions, improved mask handling, fixed CoM/link frame transformations, and added better device parameter handling
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation_data.py Added is_primed property to prevent post-instantiation changes, improved timestamped buffer usage with .assign(), optimized state computation by caching buffers, and added missing device parameters
source/isaaclab/isaaclab/utils/warp/utils.py New utility functions for converting torch tensors to warp arrays with proper index/mask handling, reducing code duplication across the codebase
source/isaaclab_newton/isaaclab_newton/assets/utils/shared.py New helper functions for finding bodies/joints by name, but missing newline at end of file
source/isaaclab_newton/isaaclab_newton/kernels/state_kernels.py Added combined split_state_to_pose_and_velocity kernel, fixed sign error in CoM-to-link frame transformation (negated com_position)
source/isaaclab_newton/isaaclab_newton/kernels/velocity_kernels.py Fixed velocity projection formulas in velocity_projector and velocity_projector_inv, correcting which velocity component gets modified

Sequence Diagram

sequenceDiagram
    participant User
    participant Articulation
    participant ArticulationData
    participant WarpUtils
    participant Kernels
    participant Newton

    Note over User,Newton: Initialization Phase
    User->>Articulation: __init__(cfg)
    Articulation->>ArticulationData: create data buffers
    ArticulationData->>Newton: bind to simulation
    Articulation->>Articulation: _create_buffers()
    Articulation->>ArticulationData: set is_primed = True
    
    Note over User,Newton: State Write Operations
    User->>Articulation: write_root_state_to_sim(state, env_ids)
    Articulation->>WarpUtils: make_complete_data_from_torch_single_index()
    WarpUtils-->>Articulation: complete warp array
    Articulation->>WarpUtils: make_masks_from_torch_ids()
    WarpUtils-->>Articulation: warp mask
    Articulation->>Kernels: split_state_to_pose_and_velocity()
    Kernels-->>Articulation: pose, velocity
    Articulation->>Newton: set transforms and velocities
    
    Note over User,Newton: State Read Operations
    User->>Articulation: data.root_state_w
    Articulation->>ArticulationData: check timestamp
    alt timestamp stale
        ArticulationData->>Newton: get_root_transforms()
        ArticulationData->>Newton: get_root_velocities()
        ArticulationData->>Kernels: combine_pose_and_velocity_to_state()
        ArticulationData->>ArticulationData: update timestamp
    end
    ArticulationData-->>User: cached state
    
    Note over User,Newton: Joint Operations
    User->>Articulation: write_joint_state_to_sim(pos, vel, env_ids, joint_ids)
    Articulation->>WarpUtils: make_complete_data_from_torch_dual_index()
    WarpUtils-->>Articulation: complete arrays
    Articulation->>WarpUtils: make_masks_from_torch_ids() [2x]
    WarpUtils-->>Articulation: env_mask, joint_mask
    Articulation->>Kernels: update_array2D_with_array2D_masked()
    Kernels->>Newton: write to simulation
    
    Note over User,Newton: Update Cycle
    User->>Articulation: update(dt)
    Articulation->>ArticulationData: update(dt)
    ArticulationData->>ArticulationData: increment timestamp
    ArticulationData->>ArticulationData: invalidate cached properties
Loading

Copy link
Contributor

@greptile-apps greptile-apps bot left a comment

Choose a reason for hiding this comment

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

Additional Comments (7)

  1. source/isaaclab_newton/isaaclab_newton/assets/utils/shared.py, line 61 (link)

    style: missing newline at end of file

    Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!

  2. source/isaaclab/isaaclab/utils/warp/utils.py, line 95-100 (link)

    syntax: docstring args don't match function parameters - describes value, first_ids, second_ids, first_mask, second_mask, dtype, device but function only has N, first_ids, first_mask, device

  3. source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation_data.py, line 109-112 (link)

    syntax: docstring says "..note::" but should be ".. note::" (missing space after ..)

  4. source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation_data.py, line 159-162 (link)

    syntax: copy-paste error in docstring - says "Once this quantity is set to True" but should describe setting the default root pose, not a boolean

  5. source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation_data.py, line 185-188 (link)

    syntax: same docstring issue - says "Once this quantity is set to True" but should describe setting the default root velocity

  6. source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation_data.py, line 209-212 (link)

    syntax: same docstring issue - says "Once this quantity is set to True" for joint positions

  7. source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation_data.py, line 233-236 (link)

    syntax: same docstring issue - says "Once this quantity is set to True" for joint velocities

18 files reviewed, 7 comments

Edit Code Review Agent Settings | Greptile

@AntoineRichard AntoineRichard force-pushed the antoiner/newton_test_articulation branch from 3c3840e to 9767629 Compare December 17, 2025 12:10
@AntoineRichard AntoineRichard force-pushed the antoiner/newton_test_articulation branch from 9767629 to e594127 Compare December 17, 2025 16:54
@kellyguo11 kellyguo11 changed the title [Newton] Test Articulation Newton [Newton] Adds tests and benchmarks for Articulation Newton Dec 17, 2025
@kellyguo11 kellyguo11 merged commit e001d0f into isaac-sim:dev/newton Dec 17, 2025
4 of 9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

isaac-lab Related to Isaac Lab team

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants