Skip to content

Conversation

@ooctipus
Copy link
Collaborator

Description

PhysxSchema requires omni.physx packages which is not available if ov is not available. Since it is not a pxr pacakge, we use string rather than api to avoid dependency on ov.

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

@greptile-apps
Copy link
Contributor

greptile-apps bot commented Nov 27, 2025

Greptile Overview

Greptile Summary

This PR removes the dependency on omni.physx packages by replacing PhysxSchema API calls with string token-based schema manipulation. The changes allow the code to work in environments where OpenVerse (OV) is not available.

Key Changes:

  • Replaced PhysxSchema imports with Tf.Token from pxr package
  • Changed API checks from prim.HasAPI(PhysxSchema.PhysxArticulationAPI) to "PhysxArticulationAPI" in prim.GetAppliedSchemas()
  • Replaced PhysxSchema.PhysxArticulationAPI.Apply(prim) with prim.AddAppliedSchema(Tf.Token("PhysxArticulationAPI"))
  • Replaced prim.RemoveAPI(PhysxSchema.PhysxArticulationAPI) with prim.RemoveAppliedSchema(Tf.Token("PhysxArticulationAPI"))
  • Modified safe_set_attribute_on_usd_prim() to directly set attributes instead of using omni.kit.commands.execute()
  • Added string type support to safe_set_attribute_on_usd_prim()

Critical Issue Found:

  • In schemas.py:182-183, the attribute copying logic for PhysX articulation properties was changed from copying ALL existing attributes to only copying attributes present in the cfg dict. This breaks existing functionality and will lose PhysX attributes not explicitly in the config. The test file check_floating_base_made_fixed.py correctly implements this by iterating over all physxArticulation:* attributes.

Confidence Score: 2/5

  • Critical bug in attribute copying logic will cause data loss in production
  • The PR has a critical logic error in schemas.py where PhysX articulation attributes are only copied from the config dict instead of from the source prim, causing existing attributes to be lost. While the overall approach of removing PhysxSchema dependency is sound, this specific bug must be fixed before merge.
  • Pay close attention to source/isaaclab/isaaclab/sim/schemas/schemas.py lines 182-183 - the attribute copying logic is broken and will lose existing PhysX attributes

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/sim/schemas/schemas.py 2/5 Major refactor replacing PhysxSchema API with string tokens, critical bug in attribute copying at line 182
source/isaaclab/isaaclab/sim/utils.py 4/5 Simplified safe_set_attribute_on_usd_prim, removed omni.kit.commands dependency, added string type support
source/isaaclab/test/deps/isaacsim/check_floating_base_made_fixed.py 5/5 Test file correctly implements attribute copying by iterating over all physxArticulation attributes

Sequence Diagram

sequenceDiagram
    participant Client as Client Code
    participant Utils as sim/utils.py
    participant Schemas as sim/schemas/schemas.py
    participant Prim as USD Prim
    
    Note over Client,Prim: PhysxSchema API Removal Pattern
    
    Client->>Schemas: modify_articulation_root_properties()
    Schemas->>Prim: GetAppliedSchemas()
    Prim-->>Schemas: List of schema names
    Schemas->>Schemas: Check "PhysxArticulationAPI" in list
    alt Schema not applied
        Schemas->>Prim: AddAppliedSchema(Tf.Token("PhysxArticulationAPI"))
    end
    
    Schemas->>Utils: safe_set_attribute_on_usd_prim(prim, "PhysxArticulationAPI:attr", value)
    Utils->>Utils: Convert to camel case if needed
    Utils->>Utils: Determine SDF type from value
    Utils->>Prim: GetAttribute(attr_name)
    alt Attribute doesn't exist
        Utils->>Prim: CreateAttribute(attr_name, sdf_type)
    end
    Utils->>Prim: attr.Set(value)
    
    Note over Client,Prim: Schema Removal Pattern
    
    Client->>Prim: RemoveAppliedSchema(Tf.Token("PhysxArticulationAPI"))
    
    Note over Client,Prim: Before: Used PhysxSchema.PhysxArticulationAPI
    Note over Client,Prim: After: Uses string tokens to avoid omni.physx dependency
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.

7 files reviewed, 1 comment

Edit Code Review Agent Settings | Greptile

Comment on lines 182 to 183
for attr_name, value in cfg.items():
safe_set_attribute_on_usd_prim(parent_prim, f"PhysxArticulationAPI:{attr_name}", value, camel_case=True)
Copy link
Contributor

Choose a reason for hiding this comment

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

logic: copies only cfg attributes, not existing PhysX attributes from articulation_prim

original code iterated over all schema attributes from the prim, this only sets attributes in cfg. any existing PhysX articulation attributes not in cfg will be lost

Suggested change
for attr_name, value in cfg.items():
safe_set_attribute_on_usd_prim(parent_prim, f"PhysxArticulationAPI:{attr_name}", value, camel_case=True)
# -- physx attributes
for attr in articulation_prim.GetAttributes():
if not attr.GetName().startswith("physxArticulation:"):
continue
parent_attr = parent_prim.GetAttribute(attr.GetName())
if not parent_attr:
parent_attr = parent_prim.CreateAttribute(attr.GetName(), attr.GetTypeName())
parent_attr.Set(attr.Get())

@ooctipus ooctipus changed the title Replaces PhysxSchema API call with string Token get and set [Newton]Replaces PhysxSchema API call with string Token get and set 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! Let's get the merges in quickly and see what breaks later :D

@AntoineRichard
Copy link
Collaborator

@ooctipus Could you resolve the merge conflicts? It's not straight forward, and I don't have time to do it. Thanks! :D

@ooctipus ooctipus force-pushed the feature/newton/string_schema branch from 217b510 to 8cb922d Compare December 1, 2025 20:08
@ooctipus
Copy link
Collaborator Author

ooctipus commented Dec 1, 2025

Done : ))

@kellyguo11 kellyguo11 merged commit cee0550 into isaac-sim:dev/newton Dec 1, 2025
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants