Skip to content

Conversation

@GiulioRomualdi
Copy link
Contributor

Description

Please include a summary of the change and which issue is fixed. Please also include relevant motivation and context.
List any dependencies that are required for this change.

Fixes #4050

Type of change

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

Screenshots

Please attach before and after screenshots of the change if applicable.

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 Nov 20, 2025
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 20, 2025

Greptile Overview

Greptile Summary

Fixed a critical shape mismatch bug in randomize_joint_parameters that caused runtime errors during environment initialization. The issue occurred when indexing joint parameter tensors - the code was incorrectly applying env_ids[:, None] broadcasting in all cases, creating tensors with shape (num_envs, 1, num_joints) instead of the expected (num_envs, num_joints).

Key Changes:

  • Introduced env_ids_for_slice variable that conditionally applies [:, None] broadcasting only when both env_ids and joint_ids are tensors
  • Applied the fix consistently across all joint parameter writes: friction coefficients (static, dynamic, viscous), armature, and position limits
  • The fix aligns with how downstream methods (e.g., write_joint_friction_coefficient_to_sim) expect to receive parameter tensors

Impact:

  • Resolves RuntimeError: shape mismatch that prevented startup randomization of actuator parameters
  • Maintains correct behavior for all edge cases: when either or both of env_ids/joint_ids are slices vs tensors

Confidence Score: 5/5

  • This PR is safe to merge with high confidence
  • The fix is minimal, well-targeted, and correctly addresses the root cause. The logic properly handles all edge cases (slice vs tensor combinations), follows the same pattern used in downstream methods, and resolves a critical bug that completely blocked the feature
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/envs/mdp/events.py 5/5 Fixed tensor shape mismatch by conditionally applying broadcasting only when both env_ids and joint_ids are tensors

Sequence Diagram

sequenceDiagram participant User as User Code participant RJP as randomize_joint_parameters participant Helper as _randomize_prop_by_op participant Asset as Articulation Asset User->>RJP: Call with env_ids & joint_ids (tensors) Note over RJP: Determine env_ids_for_slice<br/>if both are tensors:<br/>env_ids_for_slice = env_ids[:, None]<br/>else: env_ids_for_slice = env_ids RJP->>Helper: Call _randomize_prop_by_op(data, env_ids, joint_ids) Note over Helper: Returns tensor shape (num_envs, num_joints) Helper-->>RJP: friction_coeff tensor RJP->>RJP: Index: friction_coeff[env_ids_for_slice, joint_ids] Note over RJP: Result shape: (num_envs, num_joints)<br/>NOT (num_envs, 1, num_joints) RJP->>Asset: write_joint_friction_coefficient_to_sim<br/>(static_friction_coeff, env_ids, joint_ids) Note over Asset: Asset expects shape:<br/>(len(env_ids), len(joint_ids)) Asset->>Asset: Internally applies env_ids[:, None] for indexing Asset-->>RJP: Success 
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.

1 file reviewed, no comments

Edit Code Review Agent Settings | Greptile

@GiulioRomualdi
Copy link
Contributor Author

Hi @ooctipus is there something I can do to have this PR merged in main?

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

1 participant