Skip to content

Conversation

@Soappyooo
Copy link

@Soappyooo Soappyooo commented Nov 25, 2025

Description

This PR fixes a bug in the mesh converter where collision approximation was missing when applying mesh collision properties. The fix ensures that the collision approximation token is properly set on the USD mesh collision API.

Fixes #4077

Key changes

  1. Added mesh_approximation_token attribute to all MeshCollisionPropertiesCfg classes
  2. Modified modify_mesh_collision_properties to explicitly set the mesh collision approximation using the token
  3. Updated extract_mesh_collision_api_and_attrs to exclude mesh_approximation_token from custom attributes
  4. Fixed a bug where len(custom_attrs > 0) was used instead of len(custom_attrs) > 0
  5. Added comprehensive unit tests for various collision approximation types (convex decomposition, triangle mesh, SDF)

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

greptile-apps bot commented Nov 25, 2025

Greptile Overview

Greptile Summary

This PR fixes a critical bug where mesh collision approximation attributes were not being set on USD prims during mesh conversion. The fix ensures that collision approximation tokens (convex decomposition, triangle mesh, SDF, etc.) are properly applied to mesh collision APIs.

Key changes:

  • Added mesh_approximation_token attribute to all MeshCollisionPropertiesCfg classes to store the appropriate collision approximation token
  • Modified modify_mesh_collision_properties in schemas.py to explicitly set the approximation attribute on the UsdPhysics.MeshCollisionAPI using the token from the config
  • Removed the old approach that tried to derive approximation from class names and added it to custom_attrs dictionary
  • Fixed a logic bug where len(custom_attrs > 0) was incorrectly used instead of len(custom_attrs) > 0
  • Updated extract_mesh_collision_api_and_attrs to exclude mesh_approximation_token from custom attributes and added proper type hints and docstring
  • Added comprehensive unit tests for convex decomposition, triangle mesh, and SDF collision approximations
  • Updated existing test helper to use the new mesh_approximation_token attribute

The implementation correctly uses UsdPhysics.Tokens for most approximation types and PhysxSchema.Tokens.sdf for SDF mesh collision, which aligns with the USD/PhysX API distinction in the codebase.

Confidence Score: 5/5

  • This PR is safe to merge - it fixes a critical bug with proper test coverage
  • The changes are well-structured and address the reported bug effectively. The fix includes comprehensive unit tests, corrects a logic bug, and follows the existing codebase patterns. All changes are confined to the collision approximation handling with no breaking changes to the API.
  • No files require special attention

Important Files Changed

File Analysis

Filename Score Overview
source/isaaclab/isaaclab/sim/schemas/schemas_cfg.py 5/5 Added mesh_approximation_token attribute to all mesh collision property config classes to properly store collision approximation tokens
source/isaaclab/isaaclab/sim/schemas/schemas.py 5/5 Fixed collision approximation setting by explicitly applying it via MeshCollisionAPI and corrected logic bug in conditional expression
source/isaaclab/test/sim/test_mesh_converter.py 5/5 Updated test to use new mesh_approximation_token attribute and added comprehensive tests for convex decomposition, triangle mesh, and SDF

Sequence Diagram

sequenceDiagram
    participant User
    participant MeshConverter
    participant schemas.py
    participant MeshCollisionAPI
    participant Prim
    
    User->>MeshConverter: Convert mesh with collision config
    MeshConverter->>schemas.py: modify_mesh_collision_properties(prim_path, cfg)
    schemas.py->>Prim: Get prim at path
    schemas.py->>MeshCollisionAPI: Apply(prim) if not already applied
    schemas.py->>MeshCollisionAPI: Set Approximation attribute using cfg.mesh_approximation_token
    Note over schemas.py,MeshCollisionAPI: NEW: Explicit approximation setting
    schemas.py->>schemas.py: extract_mesh_collision_api_and_attrs(cfg)
    Note over schemas.py: Excludes mesh_approximation_token from custom_attrs
    schemas.py->>MeshCollisionAPI: Apply api_func (USD or PhysX)
    schemas.py->>MeshCollisionAPI: Set custom attributes (hull_vertex_limit, etc.)
    MeshCollisionAPI-->>schemas.py: Collision properties applied
    schemas.py-->>MeshConverter: Properties set
    MeshConverter-->>User: Mesh converted with correct approximation
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.

4 files reviewed, no comments

Edit Code Review Agent Settings | Greptile


physx_func: callable = MISSING

mesh_approximation_token: UsdPhysics.Tokens | PhysxSchema.Tokens = MISSING
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we add docstrings to all the attributes in this class? It seems previous MR missed out on this during the review process.

Copy link
Contributor

Choose a reason for hiding this comment

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

Instead of tokens, can we make them "str" which gets remapped to tokens internally?


mesh_collision_appx_type = type(cfg).__name__.partition("PropertiesCfg")[0]

if use_usd_api:
Copy link
Contributor

Choose a reason for hiding this comment

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

It is still useful to keep docstrings for the logic on mesh API decision making

Copy link
Contributor

@Mayankm96 Mayankm96 left a comment

Choose a reason for hiding this comment

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

Thank you for the fix and adding the test! Made a few comments. Please also update the extension.toml and CHANGELOG files.

Soappyooo and others added 3 commits November 26, 2025 10:36
Co-authored-by: Mayank Mittal <12863862+Mayankm96@users.noreply.github.com>
Signed-off-by: Fan Dongxuan <soappyooo@outlook.com>
@Soappyooo
Copy link
Author

Updated the code according to the review.

Key Changes

1. Replaced token-based API with string-based API

  • Changed mesh_approximation_token to mesh_approximation_name (string) across all mesh collision configs
  • Added MESH_APPROXIMATION_TOKENS dictionary to map strings to USD/PhysX tokens

2. Enhanced validation and error handling

  • Added validation for mesh approximation strings with helpful error messages
  • Improved type hints and docstrings in extract_mesh_collision_api_and_attrs() and modify_mesh_collision_properties()

3. Updated all mesh collision configuration classes

  • Converted all configs (BoundingCube, BoundingSphere, ConvexHull, ConvexDecomposition, TriangleMesh, TriangleMeshSimplification, SDFMesh) to use string names
  • Updated docstrings to reference MESH_APPROXIMATION_TOKENS constant

4. Updated tests and version

  • Modified test_mesh_converter.py to work with new string-based API
  • Bumped version to 0.48.7 in CHANGELOG and extension.toml

@Soappyooo Soappyooo requested a review from Mayankm96 November 26, 2025 04:14
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.

[Bug Report] Mesh converter missing approximation attr for mesh collision

2 participants