Repository navigation
Conversation
There was a problem hiding this comment.
Isaac Lab Review Bot
The change restores hand/finger-only contacts for rigid Franka Lift and Reorient by selecting primitive colliders, disabling the ten arm capsules through the task-local spawn override, and excluding disabled collision geometry from reset-clearance collection.
- Design and architecture: Collider selection remains localized to the rigid Franka task configuration, while the shared Lift/Reorient mesh collector consistently reflects enabled physics collision geometry. This separation matches the producer and consumer responsibilities traced in the patch.
- API: The existing collect_collision_meshes signature and return structure are unchanged. Its clearance semantics now deliberately exclude disabled colliders, and that user-visible behavior is documented in the isaaclab_tasks changelog fragment.
- Implementation: The collision-enabled check is applied only to prims carrying CollisionAPI, preserving default-enabled colliders while filtering explicitly disabled geometry. Tests cover primitive variant selection, the ten disabled arm capsules, the three enabled hand/finger colliders, and both default-enabled and explicitly enabled clearance geometry. A minor residual validation tradeoff is that the contact test applies collision properties directly rather than exercising the complete spawn path, though the configured pattern and resulting collider set are checked.
No blocking issues. No inline issue met the actionable-evidence threshold; the assessment above records the review feedback.
Automated review; human maintainers own approval decisions.
|
| link_path = "/Robot/Geometry" | ||
| for index in range(8): | ||
| link_path += f"/panda_link{index}" | ||
| names = [f"link{index}_capsule"] | ||
| if index in (5, 7): | ||
| names = [f"link{index}_capsule_0", f"link{index}_capsule_1"] | ||
| if selection == "convex_hulls": | ||
| names = [f"link{index}_c"] | ||
| for name in names: | ||
| prim = stage.DefinePrim(f"{link_path}/{name}", "Capsule" if selection == "primitives" else "Mesh") | ||
| UsdPhysics.CollisionAPI.Apply(prim) | ||
| hand_path = f"{link_path}/panda_hand" | ||
| for suffix, shape in ( | ||
| ("hand_capsule", "Capsule"), | ||
| ("panda_leftfinger/left_finger_pad", "Cube"), | ||
| ("panda_rightfinger/right_finger_pad", "Cube"), | ||
| ): | ||
| UsdPhysics.CollisionAPI.Apply(stage.DefinePrim(f"{hand_path}/{suffix}", shape)) |
There was a problem hiding this comment.
Synthetic Test Hides Asset Drift
This regression test manually recreates the external asset hierarchy expected by the new override, so it cannot detect the integration failure it is meant to guard against. If franka_panda.usda moves, renames, or adds an arm capsule, production only logs a warning or modifies a subset while this fixture still passes, leaving unintended arm contacts undetected. Please add an asset-backed assertion for the composed set of enabled colliders.
Note: If this suggestion doesn't match your team's coding style, reply to this and let me know. I'll remember it for next time!
|
run-ci |
|
superseded by #7829, which now carries the enabled-collider reset fix as part of the consolidated franka asset migration. |
The Franka asset update added active arm capsules to its primitive variant. The previous asset had only a hand capsule and two finger pads enabled. #7870 unblocked reset sampling by selecting convex hulls, but that also enabled arm and hand mesh contacts. This follow-up restores the original active contact geometry for rigid Franka Lift and Reorient: select
Colliders=primitivesand use the existing task-localcollision_propsoverride to disable all ten arm capsules.Disabling physics contacts alone would leave the reset hang: the clearance collector previously included every prim with
CollisionAPI, even whenphysics:collisionEnabled=false. The new link1 capsule extends 6 cm below the ground, so it would still reject every reset candidate. The collector now includes only enabled colliders.Reset behavior changes deliberately: the old collector also used disabled arm meshes as clearance proxies. These are no longer checked. Clearance now uses the enabled hand/finger colliders for Franka, with the existing object, table, and ground criteria and thresholds. The enabled-collider rule applies to the shared Lift/Reorient clearance collector; the arm-contact override belongs to the rigid Franka task configuration.
This corrects the collision-behavior limitation documented in #7870. The original reset hang was triggered by the external asset change, rather than by the unrelated PRs where the shared RL export test exposed it.
Validation:
test_lift_env_cfg.pypass with the fix.Lift-Franka__to__Lift-KukaAllegro) passed in 70.60 s.uv run isaaclab -fpassed.Local GPU validation used PyTorch 2.11+cu128, Newton 1.6rc1, and RSL-RL 5.4.1. CI uses the repository's locked dependencies; its results remain to be confirmed.