Skip to content

Conversation

@ooctipus
Copy link
Collaborator

@ooctipus ooctipus commented Nov 27, 2025

Description

PhysxSchema is not a native package in pxr, and will not be importable without launched isaacsim
Newton reads the physxscheme directly from string, this pr makes sure we can alter change schema in similar string way without isaacsim dependency

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

greptile-apps bot commented Nov 27, 2025

Greptile Overview

Greptile Summary

This PR removes the dependency on PhysxSchema from pxr by replacing typed API calls with direct USD string-based schema manipulation. The change enables IsaacLab to modify PhysX schemas without launching Isaac Sim, similar to how Newton reads schemas directly.

Key Changes

  • Removed all PhysxSchema imports across the codebase
  • Replaced PhysxSchema.PhysxXxxAPI.Apply(prim) with prim.AddAppliedSchema("PhysxXxxAPI")
  • Replaced PhysxSchema.PhysxXxxAPI(prim) checks with "PhysxXxxAPI" in prim.GetAppliedSchemas()
  • Replaced api.RemoveAPI() with prim.RemoveAppliedSchema()
  • Added safe_set_attribute_on_usd_prim function to set attributes directly on prims using string-based attribute names
  • Enhanced safe_set_attribute_on_usd_prim to support Token type for string values

Critical Issues Found

  • Incorrect attribute naming in schemas.py:543 and schemas.py:552: Uses PhysxRigidBodyAPI:sleepThreshold and PhysxContactReportAPI:threshold instead of the correct lowercase prefix format (physxRigidBody:sleepThreshold and physxContactReport:threshold). This will cause runtime errors as the attributes won't be found.

Impact

The approach is sound and aligns with the stated goal, but the attribute naming bugs must be fixed before merge to prevent runtime failures in contact sensor activation.

Confidence Score: 2/5

  • This PR has critical logical errors that will cause runtime failures
  • The PR implements a valid approach to remove PhysxSchema dependency, but contains critical bugs in schemas.py where incorrect attribute names are used (PhysxRigidBodyAPI:sleepThreshold instead of physxRigidBody:sleepThreshold). These bugs will cause contact sensor activation to fail at runtime. The rest of the implementation is solid and follows the correct pattern.
  • schemas.py lines 543 and 552 require immediate fixes for correct attribute naming

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/sim/schemas/schemas.py 1/5 replaced PhysxSchema API usage with string-based schema manipulation; contains critical bugs with incorrect attribute naming
source/isaaclab/isaaclab/sim/spawners/materials/physics_materials.py 5/5 replaced PhysxSchema API with string-based schema addition and direct attribute setting
source/isaaclab/isaaclab/sim/utils/utils.py 5/5 added string type support to safe_set_attribute_on_usd_prim and simplified attribute setting logic

Sequence Diagram

sequenceDiagram
    participant User
    participant IsaacLab
    participant USD_Prim
    participant PhysX_Schema
    
    Note over User,PhysX_Schema: Before PR: Using PhysxSchema API
    User->>IsaacLab: Apply physics properties
    IsaacLab->>PhysX_Schema: Import PhysxSchema
    PhysX_Schema-->>IsaacLab: Return schema API objects
    IsaacLab->>PhysX_Schema: PhysxArticulationAPI.Apply(prim)
    PhysX_Schema->>USD_Prim: Apply schema to prim
    IsaacLab->>PhysX_Schema: api.CreateAttribute().Set(value)
    PhysX_Schema->>USD_Prim: Set attribute value
    
    Note over User,PhysX_Schema: After PR: Direct USD string-based approach
    User->>IsaacLab: Apply physics properties
    IsaacLab->>USD_Prim: Check prim.GetAppliedSchemas()
    USD_Prim-->>IsaacLab: Return list of schemas
    IsaacLab->>USD_Prim: prim.AddAppliedSchema("PhysxArticulationAPI")
    IsaacLab->>USD_Prim: prim.CreateAttribute(name, type)
    IsaacLab->>USD_Prim: attr.Set(value)
    
    Note over IsaacLab,USD_Prim: No dependency on PhysxSchema package
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.

8 files reviewed, 2 comments

Edit Code Review Agent Settings | Greptile

ooctipus and others added 3 commits November 26, 2025 17:17
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Signed-off-by: ooctipus <[email protected]>
Co-authored-by: greptile-apps[bot] <165735046+greptile-apps[bot]@users.noreply.github.com>
Signed-off-by: ooctipus <[email protected]>
@ooctipus ooctipus changed the title Replaces PhysxSchema with direct USD api with string token. Replaces PhysX schema interactions via pxr.PhysxSchema API helpers with direct prim schema apply/get calls Nov 27, 2025
Copy link
Collaborator

@AntoineRichard AntoineRichard left a comment

Choose a reason for hiding this comment

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

LGTM! Thanks Octi!

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