Skip to content

Conversation

@renezurbruegg
Copy link
Collaborator

@renezurbruegg renezurbruegg commented Oct 15, 2025

Description

Adds preserve_order flag to JointPositionToLimitsActionCfg

Type of change

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

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 enhancement New feature or request isaac-lab Related to Isaac Lab team labels Oct 15, 2025
Copy link
Collaborator

@ooctipus ooctipus left a comment

Choose a reason for hiding this comment

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

The changes looks good to me, though can you share what bug you went into without it?

Copy link
Contributor

@Mayankm96 Mayankm96 left a comment

Choose a reason for hiding this comment

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

Thanks!

@Mayankm96 Mayankm96 moved this to In review in Isaac Lab Oct 16, 2025
@renezurbruegg
Copy link
Collaborator Author

The changes looks good to me, though can you share what bug you went into without it?

Without the preserving order flag, the order of the joints in the action term are defined by the internal parsing of physx.
So e,g, providing ["index_q1", "middle_q1", "ring_q1"] as joint expressions (with preserve_order=False, which is the default), can lead to the action term internally using ["ring_q1", "middle_q1", "index_q1"] as joint order.

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.

Greptile Overview

Greptile Summary

Adds preserve_order flag to JointPositionToLimitsActionCfg to control whether joint ordering follows the config specification or the articulation's internal ordering.

Key Changes:

  • Added preserve_order: bool = False field to JointPositionToLimitsActionCfg
  • Updated JointPositionToLimitsAction.__init__ to pass preserve_order to find_joints() and resolve_matching_names_values() calls
  • Version bumped to 0.47.12

Issues Found:

  • EMAJointPositionToLimitsAction (subclass) doesn't pass preserve_order to its alpha dict resolution, creating inconsistency
  • CHANGELOG date appears to be in the future (2025-10-15)

Confidence Score: 3/5

  • Safe to merge with one logical inconsistency that should be fixed in the EMA subclass
  • The implementation correctly adds preserve_order to the base class and applies it consistently in JointPositionToLimitsAction. However, EMAJointPositionToLimitsAction's alpha dict resolution doesn't use preserve_order, creating inconsistent behavior in the subclass. This won't cause runtime errors but could lead to unexpected joint ordering when using EMA actions with dict-based alpha values.
  • source/isaaclab/isaaclab/envs/mdp/actions/joint_actions_to_limits.py needs the EMA subclass updated for consistency

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/config/extension.toml 5/5 Version bump from 0.47.11 to 0.47.12 - standard version increment
source/isaaclab/docs/CHANGELOG.rst 3/5 Added changelog entry for preserve_order flag - date shows 2025-10-15 which appears to be in the future
source/isaaclab/isaaclab/envs/mdp/actions/actions_cfg.py 5/5 Added preserve_order boolean flag with proper documentation and default value of False
source/isaaclab/isaaclab/envs/mdp/actions/joint_actions_to_limits.py 4/5 Applied preserve_order flag to find_joints and resolve_matching_names_values calls - missing from EMAJointPositionToLimitsAction.alpha initialization

Sequence Diagram

sequenceDiagram
    participant User
    participant JointPositionToLimitsActionCfg
    participant JointPositionToLimitsAction
    participant Articulation
    participant StringUtils

    User->>JointPositionToLimitsActionCfg: Set preserve_order=True
    User->>JointPositionToLimitsAction: Initialize with cfg
    
    JointPositionToLimitsAction->>Articulation: find_joints(joint_names, preserve_order)
    Articulation->>StringUtils: resolve_matching_names(keys, list, preserve_order)
    StringUtils-->>Articulation: Return ordered joint_ids and names
    Articulation-->>JointPositionToLimitsAction: joint_ids, joint_names
    
    alt scale is dict
        JointPositionToLimitsAction->>StringUtils: resolve_matching_names_values(scale, joint_names, preserve_order)
        StringUtils-->>JointPositionToLimitsAction: Return ordered indices and values
    end
    
    alt clip is dict
        JointPositionToLimitsAction->>StringUtils: resolve_matching_names_values(clip, joint_names, preserve_order)
        StringUtils-->>JointPositionToLimitsAction: Return ordered indices and values
    end
    
    Note over JointPositionToLimitsAction: Actions now respect<br/>config order
Loading

Additional Comments (1)

  1. source/isaaclab/isaaclab/envs/mdp/actions/joint_actions_to_limits.py, line 219-220 (link)

    logic: Missing preserve_order parameter in EMAJointPositionToLimitsAction.__init__. The alpha dict resolution should also respect the preserve_order flag for consistency with parent class.

1 file reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

Changelog
---------

0.47.12 (2025-10-15)
Copy link
Contributor

Choose a reason for hiding this comment

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

style: Date shows 2025-10-15 which appears to be in the future (today is 2025-11-07). Verify this date is correct.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request isaac-lab Related to Isaac Lab team

Projects

Status: In review

Development

Successfully merging this pull request may close these issues.

4 participants