Skip to content

Conversation

@Mayankm96
Copy link
Contributor

@Mayankm96 Mayankm96 commented Jan 5, 2026

Description

This MR introduces a simplified version of the ChangePrimProperty command.

The original command is designed to handle complex USD layer compositions, but most of our applications do not require this level of functionality. In practice, we either do not support multiple composition layers at all, or only support limited mechanisms such as references or variants.

Using the Kit-provided command also introduces unnecessary side effects, such as early stage attachment, due to its reliance on layer-resolving APIs. To avoid this extra coupling and complexity, this MR replaces the command with a lightweight implementation tailored to our actual use cases.

Type of change

  • Breaking change (existing functionality will not work without user modification)

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 Jan 5, 2026
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 5, 2026

Greptile Summary

Replaces Kit's ChangePropertyCommand with a lightweight custom implementation change_prim_property() that works directly with USD APIs. The new function eliminates unnecessary complexity from layer composition handling, removes the need for early stage attachment, and supports the project's actual use cases (single edit target operations). All call sites across visualization markers, spawners, and sensors have been updated to use the new function. The change is well-tested with 9 new test cases covering various scenarios including property creation, type handling, error cases, and timecode support.

Confidence Score: 5/5

  • This PR is safe to merge with minimal risk
  • The implementation is clean, well-documented, and thoroughly tested. The new function simplifies the codebase by removing unnecessary dependencies and side effects. All existing call sites have been properly migrated, and comprehensive tests validate the functionality
  • No files require special attention

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/sim/utils/prims.py Added new change_prim_property() function to replace Kit command, updated safe_set_attribute_on_usd_prim() to use it, and simplified add_usd_reference() to always use internal helper
source/isaaclab/test/sim/test_utils_prims.py Added 9 comprehensive test cases covering basic property changes, creating new properties, clearing values, different data types, error handling, and timecode support

Sequence Diagram

sequenceDiagram
    participant Client as Client Code
    participant NewFunc as change_prim_property()
    participant USD as USD Stage/Prim
    
    Note over Client,USD: Property Change Flow
    
    Client->>NewFunc: change_prim_property(prop_path, value, stage, ...)
    NewFunc->>NewFunc: Get stage handle (if not provided)
    NewFunc->>NewFunc: Convert prop_path to Sdf.Path
    NewFunc->>NewFunc: Extract prim_path from prop_path
    NewFunc->>USD: GetPrimAtPath(prim_path)
    USD-->>NewFunc: Return prim (or invalid)
    
    alt Prim invalid
        NewFunc-->>Client: Raise ValueError
    end
    
    NewFunc->>USD: GetPropertyAtPath(prop_path)
    USD-->>NewFunc: Return property (or None)
    
    alt Property doesn't exist
        alt type_to_create_if_not_exist provided
            NewFunc->>USD: CreateAttribute(name, type, is_custom)
            USD-->>NewFunc: Return new property
        else No type provided
            NewFunc-->>Client: Return False (log error)
        end
    end
    
    alt value is None
        NewFunc->>USD: property.Clear()
    else value provided
        NewFunc->>USD: property.Set(value, timecode)
    end
    
    NewFunc-->>Client: Return True (success)
Loading

Copy link
Contributor

@matthewtrepte matthewtrepte left a comment

Choose a reason for hiding this comment

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

looks good to me

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

None yet

Development

Successfully merging this pull request may close these issues.

2 participants