Skip to content

Conversation

@ooctipus
Copy link
Collaborator

Description

This PR fixes inplace operation issue reported in #4211 in observation.py, that observation mdp term body_pose when body_ids is either slice or int

Fixes #4211

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

@github-actions github-actions bot added bug Something isn't working isaac-lab Related to Isaac Lab team labels Dec 17, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Dec 17, 2025

Greptile Summary

This PR fixes an in-place modification bug in body_pose_w() where subtracting environment origins was modifying the original tensor when using slice or int indexing.

Key Changes:

  • Adds .clone() call when body_ids is slice or int to prevent modifying the original asset.data.body_pose_w tensor
  • Bumps version from 0.50.5 to 0.50.6
  • Updates CHANGELOG with bug fix documentation

Issue Found:
The isinstance(asset_cfg.body_ids, int) check is unreachable because SceneEntityCfg._resolve_body_names() converts int to list[int] before observations run. Additionally, PyTorch's advanced indexing with list[int] already creates a copy, so only slice indexing actually needs .clone().

Other Issues:

  • Missing backtick in CHANGELOG reStructuredText markup on line 10

Confidence Score: 3/5

  • This PR fixes a real bug but the implementation has a logic issue that makes part of the fix ineffective
  • The PR correctly identifies and addresses the in-place modification bug for slice indexing, which was the main issue. However, the int check is unreachable at runtime since int values are converted to list[int] during configuration resolution. The fix will work for the default slice(None) case but the int branch is dead code. Additionally, there's a minor syntax error in the CHANGELOG.
  • Pay close attention to source/isaaclab/isaaclab/envs/mdp/observations.py - the logic needs refinement to remove the unreachable int check

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/envs/mdp/observations.py Adds .clone() call for slice/int indexing to prevent in-place modification, but the int check may be unreachable after resolution
source/isaaclab/docs/CHANGELOG.rst Documents the bug fix with a minor formatting issue in the reStructuredText markup
source/isaaclab/config/extension.toml Correctly bumps patch version from 0.50.5 to 0.50.6 for this bug fix

Sequence Diagram

sequenceDiagram
    participant User as User Config
    participant Manager as ManagerBase
    participant Cfg as SceneEntityCfg
    participant Obs as body_pose_w()
    participant Tensor as PyTorch Tensor

    User->>Manager: Configure body_ids (int, list[int], or slice)
    Manager->>Cfg: resolve(scene)
    
    alt body_ids is int
        Cfg->>Cfg: Convert int to list[int]
        Note over Cfg: Lines 250-251 in scene_entity_cfg.py
    end
    
    Note over Cfg: After resolve:<br/>body_ids is either list[int] or slice
    
    Manager->>Obs: Call observation function
    Obs->>Tensor: Index with body_ids
    
    alt body_ids is slice
        Tensor-->>Obs: Returns VIEW (shares memory)
        Note over Obs: ⚠️ Needs .clone() to avoid in-place modification
    else body_ids is list[int]
        Tensor-->>Obs: Returns COPY (separate memory)
        Note over Obs: ✓ No .clone() needed
    end
    
    Obs->>Obs: Subtract env_origins
    Obs->>Manager: Return pose
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 (2)

  1. source/isaaclab/isaaclab/envs/mdp/observations.py, line 157-158 (link)

    logic: the isinstance(asset_cfg.body_ids, int) check will never be True at runtime. SceneEntityCfg._resolve_body_names() converts int to list[int] before observations run (see scene_entity_cfg.py:250-251). Also, PyTorch's advanced indexing with list[int] already creates a copy, so only slice needs .clone().

  2. source/isaaclab/docs/CHANGELOG.rst, line 10 (link)

    syntax: missing backtick after :meth:

3 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

@kellyguo11 kellyguo11 moved this to In review in Isaac Lab Dec 19, 2025
@Mayankm96 Mayankm96 changed the title Fixes in place modification to observation mdp term body_pose when body_ids is either slice or int Fixes in place modification to body_pose mdp term when body_ids is either slice or int Dec 28, 2025
@Mayankm96 Mayankm96 merged commit dd30442 into isaac-sim:main Dec 28, 2025
8 of 9 checks passed
@github-project-automation github-project-automation bot moved this from In review to Done in Isaac Lab Dec 28, 2025
@Mayankm96
Copy link
Contributor

Thanks for the fix and addressing the comments! :)

@ooctipus ooctipus deleted the fix/body_pose_inplace_op branch December 29, 2025 03:26
thiagolages pushed a commit to thiagolages/isaac-lab that referenced this pull request Jan 6, 2026
…either slice or int (isaac-sim#4233)

# Description

This PR fixes inplace operation issue reported in isaac-sim#4211 in
observation.py, that observation mdp term body_pose when body_ids is
either slice or int

Fixes isaac-sim#4211 

## 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`
- [ ] I have made corresponding changes to the documentation
- [x] My changes generate no new warnings
- [ ] 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

---------

Co-authored-by: Kelly Guo <kellyg@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

Status: Done

Development

Successfully merging this pull request may close these issues.

[Bug Report] Unintended in-place operation in body_pose_w

3 participants