Skip material randomization on physics backends that don't support it

[Kripner]

Summary

• randomize_rigid_body_material uses PhysX-specific APIs (link_paths, max_shapes, get/set_material_properties) that don't exist on Newton's ArticulationView, causing AttributeError when running tasks like Isaac-Open-Drawer-Franka-v0 with env.sim=newton.
• Detect backend support via hasattr check on get_material_properties and skip the term with a warning when unsupported.
• Also use the asset's num_shapes_per_body property when available (Newton provides this directly) instead of the PhysX-specific link_paths workaround.

[greptile-apps]

Greptile Summary

This PR adds graceful degradation to randomize_rigid_body_material by detecting whether the active physics backend supports the required PhysX-specific APIs (get_material_properties, set_material_properties) via a hasattr check and silently skipping — with a one-time warning — when they are absent. It also adds a fast-path to use asset.num_shapes_per_body when available, preserving the existing PhysX workaround as a fallback.

Confidence Score: 5/5

• Safe to merge; all remaining findings are P2 style/documentation issues that do not affect runtime behaviour.
• The logic is correct: the early-return guard in both __init__ and __call__ ensures unsupported backends never hit PhysX-specific APIs; material_buckets is only accessed after the guard; the PhysX workaround is fully preserved. Only two minor P2 issues remain (a redundant import and a misleading code comment).
• No files require special attention.

Important Files Changed

Filename	Overview
source/isaaclab/isaaclab/envs/mdp/events.py	Adds _supported flag to skip material randomization on physics backends lacking get_material_properties; also adds a num_shapes_per_body fast-path. Logic is correct; two minor style issues (redundant import, misleading comment).

Flowchart

%%{init: {'theme': 'neutral'}}%%
flowchart TD
    A["__init__ called"] --> B{"hasattr(root_view,\n'get_material_properties')?"}
    B -- No --> C["_supported = False\nlog warning\nreturn early"]
    B -- Yes --> D["_supported = True"]
    D --> E{"BaseArticulation AND\nbody_ids != slice(None)?"}
    E -- No --> F["num_shapes_per_body = None"]
    E -- Yes --> G{"hasattr(asset,\n'num_shapes_per_body')?"}
    G -- Yes --> H["Use asset.num_shapes_per_body directly"]
    G -- No --> I["PhysX workaround:\nbuild list from link_paths\nvalidate sum == max_shapes"]
    H --> J["Sample material_buckets"]
    F --> J
    I --> J

    K["__call__ invoked"] --> L{"_supported?"}
    L -- No --> M["return (no-op)"]
    L -- Yes --> N["Randomize & apply\nmaterial properties"]

Loading

_{Reviews (1): Last reviewed commit: "Skip material randomization on physics b..." | Re-trigger Greptile}

[isaaclab-review-bot]

🤖 Isaac Lab Review Bot

Summary

This PR adds Newton backend compatibility to randomize_rigid_body_material by detecting whether the physics backend supports get_material_properties via hasattr and skipping the term with a warning when unsupported. It also adds a fast path using asset.num_shapes_per_body when available (Newton provides this natively) instead of the PhysX-specific link_paths + create_rigid_body_view workaround.

Design Assessment

Silently skipping is the right approach for this term. The alternative — raising an error — would break every task that uses material randomization when run on Newton (≥15 task configs across locomotion, manipulation, dexterous tasks). The warning at init time is appropriate.

However, two design points deserve attention:

1. The hasattr check is a duck-typing probe, not a formal capability query. If Newton eventually adds get_material_properties but with a different signature, this will silently break at runtime rather than at init. Consider documenting this assumption.
2. When hasattr(self.asset, "num_shapes_per_body") is True, the value is used without validation against max_shapes. The PhysX path validates sum(num_shapes_per_body) == max_shapes. The Newton path trusts the backend blindly. If Newton ever returns incorrect data, there's no guardrail.

Architecture Impact

• Blast radius: ~15 task configurations reference randomize_rigid_body_material. All will silently degrade to no-op on Newton. This is acceptable for an initial compatibility fix.
• Other event terms: randomize_rigid_body_collider_offsets also uses root_view.get_rest_offsets() / root_view.get_contact_offsets() — these likely need the same treatment for Newton. This PR doesn't address that, but it's out of scope.
• No cross-module impact beyond the events module.

Implementation Verdict

Minor fixes requested — Two concrete issues below, one cleanup and one correctness concern.

Test Coverage

No tests added. Given this is a backend-specific guard, unit-testing would require mocking the Newton backend. At minimum, a note in the PR description about manual testing on Newton would be valuable. The author mentions Isaac-Open-Drawer-Franka-v0 with env.sim=newton — was this actually tested end-to-end?

CI Status

⚠️ Only the labeler check ran (passed). No linting, unit tests, or integration tests executed on this PR. The CI pipeline appears incomplete — this is a repo-level concern, not a PR-level one.

Findings

[Low] Redundant import logging (cleanup)
Lines 208-209: logging is imported inside the if not self._supported block, but it's already imported at module level (line 18) and used via the module-level logger. This redundant import should be removed in favor of using the existing logger object.

[Medium] Missing validation on Newton's num_shapes_per_body
Line 221: When hasattr(self.asset, "num_shapes_per_body") is True, the value is assigned directly without the sum(num_shapes_per_body) == max_shapes sanity check that the PhysX path performs. If Newton's property is malformed, material assignment will silently corrupt the wrong shape indices. The validation should apply to both paths.

[Info] Warning fires once but skip is perpetual
The logging.warning() fires in __init__, but the if not self._supported: return in __call__ executes silently on every invocation. For mode="reset" terms (e.g., ShadowHand configs), this means material randomization is silently skipped every episode reset without any ongoing indication. This is acceptable behavior, but worth documenting in the class docstring.

[isaaclab-review-bot]

[Low] Redundant import: logging is already imported at module level (line 18). Use the existing module-level logger instead:

if not self._supported:
    logger.warning(
        "randomize_rigid_body_material: skipping because the current physics backend"
        " does not support material property randomization."
    )

This also avoids creating a new logger instance and is consistent with the rest of the file.

[isaaclab-review-bot]

[Medium] Missing validation: The PhysX path (lines 224-233) validates sum(num_shapes_per_body) == max_shapes. This Newton path trusts the backend property blindly. If it's ever wrong, materials will be assigned to incorrect shape indices.

Suggestion — add the same sanity check:

if hasattr(self.asset, "num_shapes_per_body"):
    self.num_shapes_per_body = self.asset.num_shapes_per_body
    # validate against max_shapes (same check as PhysX path)
    num_shapes = sum(self.num_shapes_per_body)
    expected_shapes = self.asset.root_view.max_shapes
    if num_shapes != expected_shapes:
        raise ValueError(
            "Randomization term 'randomize_rigid_body_material' failed to parse the number of shapes"
            f" per body. Expected total shapes: {expected_shapes}, but got: {num_shapes}."
        )

[ooctipus]

#5098

this PR should support it : D, I am closing this PR for now.