Skip to content

Conversation

@pascal-roth
Copy link
Collaborator

Description

Cleans up new util functions

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

greptile-apps bot commented Nov 19, 2025

Greptile Summary

  • Modernized imports by switching from import typing to specific imports (Literal, Callable, Iterable) and added Literal["x", "y", "z"] type hint for axis parameter
  • Replaced get_carb_setting() wrapper function with direct carb_setting.get() call and removed unused import
  • Removed parameter names from multiple docstring Args sections (e.g., attaching_early:, stage:, usd_path:), which is inconsistent with codebase documentation standards used in other files like nucleus.py and actuator_base.py

Confidence Score: 4/5

  • This PR is safe to merge with minor style inconsistencies
  • The code changes are functionally correct (improved imports, removed wrapper function). The main issue is documentation style inconsistency where parameter names were removed from docstrings, deviating from the codebase standard. No logic errors or security issues.
  • The docstring changes in stage.py should be reviewed to ensure consistency with project documentation standards

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/sim/utils/stage.py Cleaned up imports and replaced wrapper function call, but removed parameter names from docstrings inconsistently with codebase standards

Sequence Diagram

sequenceDiagram
    participant User
    participant Stage
    participant CarbSettings
    participant PhysX
    participant USDContext
    
    User->>Stage: attach_stage_to_usd_context(attaching_early)
    Stage->>Stage: Check Isaac Sim version
    Stage->>Stage: Check if stage is in memory
    Stage->>PhysX: attach_stage(stage_id)
    Stage->>CarbSettings: get("/physics/fabricUpdateTransformations")
    CarbSettings-->>Stage: is_rendering_enabled
    alt rendering enabled
        Stage->>USDContext: attach_stage_with_callback(stage_id)
        Stage->>PhysX: attach_stage(stage_id)
    end
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, 6 comments

Edit Code Review Agent Settings | Greptile
React with 👍 or 👎 to share your feedback on this new summary format

Args:
attaching_early: Whether to attach the stage to the usd context before stage is created. Defaults to False.
Whether to attach the stage to the usd context before stage is created. Defaults to False.
Copy link
Contributor

Choose a reason for hiding this comment

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

style: parameter name missing in docstring - should be attaching_early: to match codebase standard (see nucleus.py or actuator_base.py for examples)

Suggested change
Whether to attach the stage to the usd context before stage is created. Defaults to False.
attaching_early: Whether to attach the stage to the usd context before stage is created. Defaults to False.

Args:
stage: The stage to set temporarily.
The stage to set temporarily.
Copy link
Contributor

Choose a reason for hiding this comment

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

style: parameter name missing - should be stage: to match codebase standard

Suggested change
The stage to set temporarily.
stage: The stage to set temporarily.

Args:
predicate: user defined function that takes a prim_path (str) as input and returns True/False if the prim
predicate: user defined function that takes a prim_path as input and returns True/False if the prim
Copy link
Contributor

Choose a reason for hiding this comment

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

style: parameter name missing - should be predicate: to match codebase standard

Suggested change
predicate: user defined function that takes a prim_path as input and returns True/False if the prim
predicate: user defined function that takes a prim_path as input and returns True/False if the prim

Args:
usd_path (str): Path to the USD file to open.
usd_path: Path to the USD file to open.
Copy link
Contributor

Choose a reason for hiding this comment

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

style: parameter name missing - should be usd_path: to match codebase standard

Suggested change
usd_path: Path to the USD file to open.
usd_path: Path to the USD file to open.

Args:
usd_path (str): File path to save the current stage to
usd_path: File path to save the current stage to
Copy link
Contributor

Choose a reason for hiding this comment

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

style: parameter name missing - should be usd_path: to match codebase standard

Suggested change
usd_path: File path to save the current stage to
usd_path: File path to save the current stage to

Comment on lines +725 to +726
path: path we want to check
parent: Parent prim for the given path. Defaults to None.
Copy link
Contributor

Choose a reason for hiding this comment

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

style: parameter names missing - should be path: and parent: to match codebase standard

Suggested change
path: path we want to check
parent: Parent prim for the given path. Defaults to None.
path: path we want to check
parent: Parent prim for the given path. Defaults to None.

@pascal-roth pascal-roth changed the title Cleans up new util functions Removes instances of IsaacSim SimulationContext Nov 23, 2025
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.

2 participants