Adds friction/restitution index APIs and unify material randomization across PhysX and Newton#5098
Adds friction/restitution index APIs and unify material randomization across PhysX and Newton#5098ooctipus wants to merge 3 commits intoisaac-sim:developfrom
Conversation
github-actions
bot
added
enhancement
Greptile SummaryThis PR extends the material property API across all three asset types (Articulation, RigidObject, RigidObjectCollection) for both the PhysX and Newton backends, and unifies the Key changes:
Issues found:
Confidence Score: 2/5
Important Files Changed
Sequence DiagramsequenceDiagram
participant Env as ManagerBasedEnv
participant Term as randomize_rigid_body_material
participant PhysX as PhysX Asset
participant Newton as Newton Asset
Note over Term: __init__: detect backend via<br/>env.sim.physics_manager.__name__
alt PhysX backend
Term->>PhysX: root_view.get_material_properties()
PhysX-->>Term: default_materials (num_envs, max_shapes, 3) on CPU
Note over Term: _init_physx(): compute shape_indices<br/>(uses _physics_sim_view.create_rigid_body_view<br/>when body_ids filtered)
else Newton backend
Term->>Newton: data._sim_bind_shape_material_mu
Newton-->>Term: default_friction (num_envs, num_shapes)
Term->>Newton: data._sim_bind_shape_material_restitution
Newton-->>Term: default_restitution (num_envs, num_shapes)
end
Note over Env,Term: __call__: each environment reset step
Env->>Term: __call__(env_ids, ...)
Term->>Term: sample bucket_ids → material_samples
alt PhysX path (_call_physx)
Term->>Term: default_materials[env_ids, shape_idx] = material_samples
Term->>PhysX: set_material_properties_index(default_materials, env_ids)
PhysX->>PhysX: root_view.set_material_properties(warp_array, env_ids)
else Newton path (_call_newton)
Term->>Term: default_friction[env_ids, shape_idx] = material_samples[..., 0]
Term->>Term: default_restitution[env_ids, shape_idx] = material_samples[..., 2]
Term->>Newton: set_friction_mask(default_friction)
Term->>Newton: set_restitution_mask(default_restitution)
Newton->>Newton: wp.launch(write_2d_data_to_buffer_with_mask)<br/>+ SimulationManager.add_model_change(SHAPE_PROPERTIES)
end
Reviews (1): Last reviewed commit: "fix linting" | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| # note: workaround since Articulation doesn't expose shapes per body directly | ||
| num_shapes_per_body = [] | ||
| for link_path in asset.root_view.link_paths[0]: # type: ignore[union-attr] | ||
| link_physx_view = asset.root_view.create_rigid_body_view(link_path) # type: ignore |
There was a problem hiding this comment.
root_view does not expose create_rigid_body_view
ArticulationView (the type behind root_view) does not have a create_rigid_body_view method — that method only exists on the global physics-simulation view (_physics_sim_view). Calling it here will raise AttributeError at runtime whenever body_ids is not slice(None) on a PhysX articulation.
Every other call to create_rigid_body_view in the codebase is routed through _physics_sim_view (e.g. contact_sensor.py, frame_transformer.py, imu.py, the PhysX rigid-object/collection initialization code). The original pre-refactor code also used self.asset._physics_sim_view.create_rigid_body_view(link_path) for exactly this reason.
| link_physx_view = asset.root_view.create_rigid_body_view(link_path) # type: ignore | |
| link_physx_view = asset._physics_sim_view.create_rigid_body_view(link_path) # type: ignore |
|
|
||
| # apply to simulation via PhysX's unified material API | ||
| asset: PhysXRigidObject | PhysXArticulation | PhysXRigidObjectCollection = self.asset # type: ignore[assignment] | ||
| asset.set_material_properties_index(materials=self.default_materials, env_ids=env_ids) |
There was a problem hiding this comment.
self.default_materials has shape (num_envs, max_shapes, 3), but the docstring of set_material_properties_index across all three PhysX asset classes states the expected shape is (len(env_ids), max_shapes, 3).
If the underlying root_view.set_material_properties(materials, env_ids) call interprets materials as a partial tensor (rows indexed by env_ids), then only the first len(env_ids) environments would receive correctly updated values, regardless of which envs are in env_ids.
The original implementation retrieved the full buffer via get_material_properties(), updated the rows for env_ids, and passed the full buffer back — suggesting the IsaacSim TensorAPI reads row i from the full tensor when env_ids[i] is in the index set. The docstring in the new set_material_properties_index is therefore misleading and should match actual usage:
# In all three PhysX asset classes:
# Args:
# materials: Material properties for all shapes. Shape is (num_envs, max_shapes, 3)
# where the 3 values are [static_friction, dynamic_friction, restitution].This same discrepancy appears in all three PhysX asset files:
source/isaaclab_physx/isaaclab_physx/assets/articulation/articulation.pysource/isaaclab_physx/isaaclab_physx/assets/rigid_object/rigid_object.pysource/isaaclab_physx/isaaclab_physx/assets/rigid_object_collection/rigid_object_collection.py
| def _init_physx(self, use_body_ids: bool) -> None: | ||
| """Initialize PhysX-specific state: shape indices and cached materials.""" | ||
| asset: PhysXArticulation | PhysXRigidObject | PhysXRigidObjectCollection = self.asset # type: ignore[assignment] | ||
|
|
||
| # compute shape indices | ||
| if use_body_ids: | ||
| # note: workaround since Articulation doesn't expose shapes per body directly | ||
| num_shapes_per_body = [] | ||
| for link_path in asset.root_view.link_paths[0]: # type: ignore[union-attr] | ||
| link_physx_view = asset.root_view.create_rigid_body_view(link_path) # type: ignore | ||
| num_shapes_per_body.append(link_physx_view.max_shapes) | ||
| shape_indices_list = [] | ||
| for body_id in self.asset_cfg.body_ids: | ||
| start_idx = sum(num_shapes_per_body[:body_id]) | ||
| end_idx = start_idx + num_shapes_per_body[body_id] | ||
| shape_indices_list.extend(range(start_idx, end_idx)) | ||
| self.shape_indices = torch.tensor(shape_indices_list, dtype=torch.long) | ||
| else: | ||
| self.shape_indices = torch.arange(asset.root_view.max_shapes, dtype=torch.long) | ||
|
|
||
| # cache default materials | ||
| self.default_materials = wp.to_torch(asset.root_view.get_material_properties()).clone() | ||
|
|
There was a problem hiding this comment.
The original __init__ included a defensive check after computing num_shapes_per_body:
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 per body."
f" Expected total shapes: {expected_shapes}, but got: {num_shapes}."
)This guard caught any mis-parsed shape counts early and produced a clear error. The new _init_physx silently drops it. Consider adding an equivalent assertion after the loop so that configuration errors surface at initialization rather than causing silent, hard-to-diagnose incorrect material assignments at runtime.
| asset.set_friction_mask(friction=self.default_friction) | ||
| asset.set_restitution_mask(restitution=self.default_restitution) |
There was a problem hiding this comment.
env_ids
_call_newton updates only the env_ids rows in self.default_friction / self.default_restitution, but then applies the complete buffer to all environments via set_friction_mask / set_restitution_mask. This is functionally correct (unchanged envs are simply re-written with their existing values), but it launches kernels proportional to num_envs × num_shapes instead of len(env_ids) × len(shape_idx).
The index variants set_friction_index / set_restitution_index are already available and would target only the requested envs and shapes:
def _call_newton(self, env_ids, shape_idx, material_samples):
self.default_friction[env_ids[:, None], shape_idx] = material_samples[..., 0]
self.default_restitution[env_ids[:, None], shape_idx] = material_samples[..., 2]
asset: NewtonRigidObject | NewtonArticulation | NewtonRigidObjectCollection = self.asset
asset.set_friction_index(
friction=self.default_friction[env_ids][:, shape_idx],
shape_ids=shape_idx,
env_ids=env_ids,
)
asset.set_restitution_index(
restitution=self.default_restitution[env_ids][:, shape_idx],
shape_ids=shape_idx,
env_ids=env_ids,
)If full-buffer writes are intentional (e.g. for graphed-pipeline compatibility), please add a comment explaining the trade-off.
kellyguo11
left a comment
kellyguo11
left a comment
There was a problem hiding this comment.
PR Review: #5098
Summary
This PR adds friction/restitution material property APIs (set_friction_index/mask, set_restitution_index/mask) to Newton assets and set_material_properties_index/mask to PhysX assets, then unifies randomize_rigid_body_material to auto-detect the backend and dispatch accordingly. It also adds a new randomize_rigid_body_inertia event term. The architecture is sound, but there are CI-breaking issues and a few concerns that need attention.
Architecture Assessment
API Symmetry: Intentionally asymmetric by design — Newton exposes separate set_friction_*/set_restitution_* (single mu), while PhysX uses a combined 3-tuple set_material_properties_*. This is well-motivated by the different physics models and documented clearly.
Breaking Changes: No existing public API signatures are changed. The randomize_rigid_body_material event term maintains the same __call__ signature. The randomize_joint_parameters class now conditionally skips dynamic_friction_coeff/viscous_friction_coeff for Newton — this is correct since Newton doesn't have these properties.
Cross-Module Impact: 16 task configs use randomize_rigid_body_material. The unified event term should be transparent to all of them since the __call__ signature is unchanged.
CI Status
🔴 All test jobs failing. The develop branch CI is green (success), confirming these are regressions introduced by this PR.
Critical failure: KeyError: 'shape_material_mu' in Newton's _create_simulation_bindings — the new get_attribute("shape_material_mu", ...) calls break ALL Newton mock-based tests (isaaclab core [3/3] shows 100+ Newton test errors). The mock ArticulationView doesn't have shape_material_mu or shape_material_restitution registered as attributes. This must be fixed before merge.
Other failures: isaaclab_physx (test_rigid_object_collection), isaaclab_mimic, and other jobs also show failures that may cascade from the same root cause or be pre-existing flakiness.
Key Findings
-
🔴 Critical: Newton mock views missing
shape_material_mu/shape_material_restitutionattributes — The new_create_simulation_bindingscode inarticulation_data.py,rigid_object_data.py, andrigid_object_collection_data.pyunconditionally callsget_attribute("shape_material_mu", ...)during construction. The Newton test mock views (mock_articulation_view.py) don't have these attributes registered, causingKeyErrorin ALL Newton-based tests. -
🟡 Performance:
_call_newtonuses mask method instead of index method —_call_newtoncallsset_friction_mask(friction=self.default_friction)which writes ALL environments' material properties to the simulator, even when only a subset ofenv_idschanged. Should useset_friction_indexwith the specificenv_idsfor better performance. -
🟡 Newton CHANGELOG is inaccurate — States "Removed
shape_material_muandshape_material_restitutiondata properties" and "Removed hardcodeddefault_body_armature" but the diff shows NO removals — only additions. The changelog should accurately reflect what changed. -
🟡
isaaclab_tasksversion bump without changelog —extension.tomlbumps from 1.5.13 to 1.5.14 but there's no correspondingCHANGELOG.rstentry and no source files changed inisaaclab_tasks. This appears to be a spurious version bump. -
🟡 PhysX changelog date mismatch —
0.5.13 (2026-03-23)but commits are dated 2026-03-24. -
🔵 Missing PhysX tests for RigidObject and RigidObjectCollection — New
test_set_material_propertiestest only coversArticulationinisaaclab_physx. Should add equivalent tests forRigidObjectandRigidObjectCollection. -
🔵 Inconsistent
_resolve_shape_idsordering — In Newtonarticulation.py, list check comes before None check. Inrigid_object.pyandrigid_object_collection.py, None check comes first. While functionally equivalent, should be consistent across asset types. -
🔵 Code duplication — The
set_friction_index/maskandset_restitution_index/maskimplementations are nearly identical across all three Newton asset classes (~170 lines each × 3). Consider extracting a mixin or shared helper to reduce maintenance burden.
...aaclab_newton/isaaclab_newton/assets/rigid_object_collection/rigid_object_collection_data.py
Show resolved
Hide resolved
source/isaaclab_newton/isaaclab_newton/assets/articulation/articulation.py
Show resolved
Hide resolved
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot - PR #5098
Summary
This PR adds friction/restitution index APIs to Newton assets and unifies the randomize_rigid_body_material MDP event term to work transparently across PhysX and Newton backends. It also adds a new randomize_rigid_body_inertia event term.
Scope: 22 files changed across isaaclab, isaaclab_newton, and isaaclab_physx extensions.
Design Assessment
API Design: ✅ Good
- The Newton APIs (
set_friction_index/mask,set_restitution_index/mask) correctly separate friction and restitution because Newton uses a single friction coefficient (mu) vs PhysX's static/dynamic friction pair. - The PhysX APIs (
set_material_properties_index/mask) maintain the existing 3-tuple format[static_friction, dynamic_friction, restitution]. - Both follow the established
_index/_maskpattern used throughout Isaac Lab for vectorized property setters.
Unification Approach: ✅ Well-designed
- Backend detection via
env.sim.physics_manager.__name__.lower()is reasonable. - Separating
_init_physx/_init_newtonand_call_physx/_call_newtonkeeps the code organized. - Device handling correctly uses CPU for PhysX (tensor API requirement) and GPU for Newton.
Architecture Impact
| Component | Impact |
|---|---|
| Newton Articulation | +184 lines (friction/restitution setters, shape indexing) |
| Newton RigidObject | +189 lines (same pattern) |
| Newton RigidObjectCollection | +188 lines (same pattern) |
| PhysX assets | +59 lines each (material properties wrapper) |
| MDP events.py | Major refactor of randomize_rigid_body_material, new randomize_rigid_body_inertia |
Data bindings: Newton implementation accesses internal _sim_bind_shape_material_* attributes directly. The TODO comment acknowledges this is not ideal and suggests future API improvements.
Implementation Verdict: ⚠️ Bug Found
Critical Issue in _init_physx:
# In randomize_rigid_body_material._init_physx():
link_physx_view = asset.root_view.create_rigid_body_view(link_path) # ❌ WRONGThis should be:
link_physx_view = asset._physics_sim_view.create_rigid_body_view(link_path) # ✅ CORRECTThe root_view is an ArticulationView which does not have create_rigid_body_view(). That method exists on _physics_sim_view (the PhysicsSimulationView). This will cause an AttributeError at runtime when use_body_ids=True on PhysX backend (i.e., when asset_cfg.body_ids != slice(None)).
Location: source/isaaclab/isaaclab/envs/mdp/events.py, line ~255 (in _init_physx method)
Test Coverage
| Extension | New Tests |
|---|---|
| isaaclab_newton | test_set_material_properties for Articulation ✅ |
| isaaclab_newton | Updated test_rigid_body_set_material_properties (unskipped) ✅ |
| isaaclab_newton | Updated test_set_material_properties for RigidObjectCollection (unskipped) ✅ |
| isaaclab_physx | test_set_material_properties for Articulation ✅ |
| isaaclab_physx | test_set_material_properties_index for RigidObject ✅ |
Missing: No tests for randomize_rigid_body_inertia event term or the unified randomize_rigid_body_material with Newton backend.
CI Status: ❌ Multiple Failures
| Job | Status | Notes |
|---|---|---|
| isaaclab_newton | ❌ | test_contact_sensor.py::test_filter_enables_force_matrix - pre-existing flaky test |
| isaaclab_physx | ❌ | test_rigid_object_collection.py - 3 failures then timeout |
| isaaclab (core) | ❌ | test_first_frame_textured_rendering.py - unrelated |
| Others | ❌ | Likely cascading from above |
The test_rigid_object_collection.py failures in PhysX may be related to this PR given the new material properties code, but the test timed out before detailed error messages were captured.
Findings
-
🔴 Critical:
_init_physxcallsasset.root_view.create_rigid_body_view()but should callasset._physics_sim_view.create_rigid_body_view(). This will crash when using body_ids filtering on PhysX. -
🟡 Medium: The
randomize_rigid_body_inertiaclass was added but not mentioned in the PR description. Consider updating the description or splitting into a separate PR for clarity. -
🟡 Medium: No MDP event tests for the new
randomize_rigid_body_inertiaor the refactoredrandomize_rigid_body_materialwith Newton backend. -
🟢 Minor: The
randomize_joint_parametersclass also received backend detection changes for joint friction. This works but the Newton path only sets static friction (correct behavior documented in code).
Review generated by Isaac Lab Review Bot
There was a problem hiding this comment.
🤖 Isaac Lab Review Bot — PR #5098
Adds friction/restitution index APIs and unify material randomization across PhysX and Newton
Summary
Clean architectural PR that extends material property APIs (friction/restitution) to Newton assets via set_friction_index/mask and set_restitution_index/mask, adds equivalent set_material_properties_index/mask wrappers for PhysX, and refactors randomize_rigid_body_material to auto-detect the physics backend. Also includes a bonus randomize_rigid_body_inertia event term. Overall design is solid — the backend-specific dispatch pattern is the right approach. I have a few findings below.
🔴 Bug: _init_physx uses root_view.create_rigid_body_view instead of _physics_sim_view.create_rigid_body_view
File: source/isaaclab/isaaclab/envs/mdp/events.py, in _init_physx()
The original code on develop uses:
link_physx_view = self.asset._physics_sim_view.create_rigid_body_view(link_path)The PR changes this to:
link_physx_view = asset.root_view.create_rigid_body_view(link_path)root_view is an ArticulationView (or RigidBodyView / RigidObjectCollectionView), not a PhysicsSimulationView. The create_rigid_body_view() method lives on PhysicsSimulationView, not on ArticulationView. This will raise an AttributeError at runtime when use_body_ids=True on the PhysX backend.
Fix: Revert to asset._physics_sim_view.create_rigid_body_view(link_path).
🟡 Inconsistency: _call_newton uses set_friction_mask/set_restitution_mask but _call_physx uses set_material_properties_index
In _call_newton, the code calls asset.set_friction_mask(friction=self.default_friction) and asset.set_restitution_mask(restitution=self.default_restitution) — these are mask methods that update all environments since no env_mask is passed.
In _call_physx, the code calls asset.set_material_properties_index(materials=self.default_materials, env_ids=env_ids) — this is an index method that only updates the specified environments.
While both work correctly (the cached buffers are only mutated for env_ids), the Newton path writes full-buffer data to all simulation environments via the mask write kernel, even for environments that haven't changed. For large env counts with partial resets, this is unnecessary overhead.
Suggestion: Consider using set_friction_index/set_restitution_index with env_ids in _call_newton for consistency and to avoid writing unchanged environments to the sim.
🟡 randomize_rigid_body_inertia not mentioned in PR title/description
The PR title says "Adds friction/restitution index APIs and unify material randomization" but the PR also adds a completely new randomize_rigid_body_inertia event term (~140 lines). This is a significant feature addition that isn't mentioned in the PR description at all, only in the CHANGELOG. Consider:
- Updating the PR description to document this addition
- Or splitting it into a separate PR for cleaner review history
🟡 num_shapes property inconsistency across Newton assets
The num_shapes property on RigidObject and RigidObjectCollection returns self.data._num_shapes, where _num_shapes is computed as:
self._num_shapes = (
self._sim_bind_shape_material_mu.shape[1] if len(self._sim_bind_shape_material_mu.shape) > 1 else 1
)For a rigid body with a single collision shape, _sim_bind_shape_material_mu may be 1D (shape (num_instances,) rather than (num_instances, 1)), which would hit the fallback else 1. This works but is fragile — if the upstream attribute ever changes its squeeze behavior, this silently breaks. Consider adding an assertion or explicit reshape to (num_instances, num_shapes) during binding creation.
🟡 randomize_joint_properties backend detection duplicates pattern
The randomize_joint_properties.__init__ also adds backend detection via env.sim.physics_manager.__name__.lower(). This is the same pattern used in randomize_rigid_body_material.__init__. Consider extracting a shared utility like detect_physics_backend(env) -> str to avoid duplicating this heuristic.
🟢 CI Status
All core CI checks are failing, but this appears to be a pre-existing infrastructure issue (all 10 test shards fail including environments_training, isaaclab_rl, etc.) — not specific to this PR's changes. Pre-commit and docs checks pass.
🟢 Tests
Good test coverage:
- Newton: Tests for articulation, rigid object, and rigid object collection material properties
- PhysX: Tests for articulation and rigid object
set_material_properties_index - Previously-skipped Newton material tests are now unskipped ✅
Checklist concern
The author marked "I have made corresponding changes to the documentation" as unchecked. Since this adds new public API methods (set_friction_index/mask, set_restitution_index/mask, set_material_properties_index/mask), documentation for these should be added.
Review by 🤖 Isaac Lab Review Bot (SHA: 01d83a3)
2 participants
Description
This PR extends the material property API across all asset types (Articulation, RigidObject, RigidObjectCollection) and unifies the randomize_rigid_body_material event term to work transparently with both the PhysX and Newton physics backends.
New APIs
set_material_properties_indexandset_material_properties_maskto all three asset classes, providing a vectorized, index-based alternative to the existing set_material_properties.set_friction_index, set_friction_mask,set_restitution_index,set_restitution_mask, and a num_shapes property to all three asset classes. Newton's friction model uses a single coefficient (mu), so friction and restitution are exposed as separate APIs rather than a combined 3-tuple.Unified MDP event term
randomize_rigid_body_material now auto-detects the active physics backend at initialization and dispatches to a backend-specific implementation:
Type of change
Screenshots
Please attach before and after screenshots of the change if applicable.
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there