Skip to content

Conversation

@Mayankm96
Copy link
Contributor

@Mayankm96 Mayankm96 commented Jan 7, 2026

Description

This MR ensures we don't try setting transforms for non-xformable prims (Scopes, Materials, Shaders). Previously, they were not being set but an error was being thrown.

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 Jan 7, 2026
@Mayankm96 Mayankm96 changed the title Adds check to create prim to not make trasnforms for non-xformable prims Add check to avoid creating transforms for non-xformable prims Jan 7, 2026
@Mayankm96 Mayankm96 merged commit 20f7707 into isaac-sim:main Jan 7, 2026
8 checks passed
@greptile-apps
Copy link
Contributor

greptile-apps bot commented Jan 7, 2026

Greptile Summary

Added early validation check in create_prim to prevent transform operations on non-Xformable prim types (Materials and Shaders). Previously, the code would attempt to apply transforms and throw errors; now it returns early with a debug message.

Key Changes:

  • Added IsA(UsdGeom.Xformable) check in create_prim before attempting transform operations
  • Enhanced error message in standardize_xform_ops to include prim type information
  • Added test coverage for non-Xformable prim creation

Critical Issue:

  • The implementation incorrectly identifies Scope prims as non-Xformable. In USD, UsdGeom.Scope inherits from UsdGeom.Xformable, so Scope prims CAN have transforms. The error messages and test both incorrectly list "scope prims" alongside Materials and Shaders, which will cause confusion and potentially incorrect behavior when users try to transform Scope prims.

Confidence Score: 2/5

  • Not safe to merge - contains logic error regarding Scope prims that will cause incorrect behavior
  • The PR has a fundamental misunderstanding about USD prim types. Scope prims ARE Xformable in USD and can have transforms, but the code and tests treat them as non-Xformable. This will prevent users from setting transforms on Scope prims when they should be able to, and the test will either fail or give false confidence.
  • All three files need attention: prims.py and transforms.py have incorrect error messages mentioning scope prims, and test_utils_prims.py has a test that incorrectly assumes Scope is not Xformable

Important Files Changed

Filename Overview
source/isaaclab/isaaclab/sim/utils/prims.py Added early return check for non-Xformable prims to prevent transform operations on Material, Shader, and Scope types
source/isaaclab/isaaclab/sim/utils/transforms.py Enhanced error message to include prim type information and clarify expected behavior for non-Xformable prims
source/isaaclab/test/sim/test_utils_prims.py Added comprehensive test for non-Xformable prim types, but test incorrectly assumes Scope is not Xformable when it actually inherits from Xformable in USD

Sequence Diagram

sequenceDiagram
    participant User
    participant create_prim
    participant USD Stage
    participant standardize_xform_ops

    User->>create_prim: create_prim(path, type, translation, orientation, scale)
    create_prim->>USD Stage: DefinePrim(path, type)
    USD Stage-->>create_prim: prim
    
    alt prim is valid
        create_prim->>create_prim: Apply attributes, USD references, semantic labels
        
        create_prim->>create_prim: Check if prim.IsA(UsdGeom.Xformable)
        
        alt prim is NOT Xformable (Material, Shader)
            create_prim->>create_prim: logger.debug("Not Xformable, skipping transforms")
            create_prim-->>User: return prim (without transforms)
        else prim is Xformable (Xform, Mesh, Scope, etc.)
            create_prim->>create_prim: Convert position/orientation to local frame
            create_prim->>standardize_xform_ops: standardize_xform_ops(prim, translation, orientation, scale)
            
            standardize_xform_ops->>standardize_xform_ops: Check if prim.IsA(UsdGeom.Xformable)
            
            alt prim is NOT Xformable
                standardize_xform_ops->>standardize_xform_ops: logger.error("Not Xformable")
                standardize_xform_ops-->>create_prim: return False
            else prim is Xformable
                standardize_xform_ops->>USD Stage: Set xformOp:translate/orient/scale
                standardize_xform_ops-->>create_prim: return True
            end
            
            create_prim-->>User: return prim (with transforms)
        end
    else prim is invalid
        create_prim-->>User: raise ValueError
    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.

Additional Comments (4)

  1. source/isaaclab/isaaclab/sim/utils/prims.py, line 169 (link)

    logic: Scope prims ARE Xformable in USD (UsdGeom.Scope inherits from UsdGeom.Xformable), so mentioning "scope prims" here is misleading

  2. source/isaaclab/isaaclab/sim/utils/transforms.py, line 136 (link)

    logic: Scope prims ARE Xformable in USD (UsdGeom.Scope inherits from UsdGeom.Xformable), remove mention of scope prims

  3. source/isaaclab/test/sim/test_utils_prims.py, line 252-257 (link)

    logic: The test incorrectly assumes Scope is not Xformable. In USD, UsdGeom.Scope inherits from UsdGeom.Xformable, so Scope prims ARE Xformable and can have transforms. Remove Scope from this test and only test with truly non-Xformable types like Material and Shader

  4. source/isaaclab/test/sim/test_utils_prims.py, line 285-304 (link)

    logic: Remove the Scope test section entirely - Scope prims ARE Xformable in USD and can have transforms. Test will fail or give incorrect results. Consider adding a Shader prim test instead

3 files reviewed, 4 comments

Edit Code Review Agent Settings | Greptile

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.

1 participant