Cable attachments to rigid bodies via Newton fixed joints#5731
Cable attachments to rigid bodies via Newton fixed joints#5731mmichelis wants to merge 78 commits into
Conversation
There was a problem hiding this comment.
Review: Cable Attachments to Rigid Bodies via Newton Fixed Joints
This is a well-structured, substantial feature addition that extends the cable system with a declarative attachment API for welding cable endpoints to rigid bodies. The architecture follows established patterns from the deformable contrib package.
✅ Strengths
-
Clean Declarative API: The
CableAttachmentCfgdataclass provides an intuitive interface for specifying cable-to-rigid-body welds at scene configuration time. -
Proper Multi-Env Handling: The
apply_cable_attachments_to_builderhook correctly filters bybody_worldto prevent cross-env binding bugs, and handles both unexpanded regex templates (USD-imported targets) and pre-expanded labels (builder-hook targets). -
Comprehensive Reset Logic: The
NewtonVBDManager.reset(soft=True)properly restoresbody_q,body_q_prev,body_qd, andbody_inertia_q— critical for AVBD's implicit velocity computation. -
Thorough Test Coverage: The test suite includes edge cases like multi-world cloning, head/tail anchor resolution, catenary formation, and the FK mask regression.
-
Well-Written Documentation: The
using-cables.rstguide is comprehensive with clear examples, parameter tables, and limitation notes.
🔍 Issues & Recommendations
1. FK Mask Build Race Condition (Medium Priority)
File: vbd_manager.py, _build_fk_mask()
The FK mask is built lazily in forward() the first time it's called. However, if forward() is invoked before start_simulation() completes (e.g., from a Kit visualizer callback), model.joint_type or model.joint_articulation may be None.
def _build_fk_mask(cls) -> None:
model = cls._model
if model is None or model.joint_type is None or model.joint_articulation is None:
return # Silent early return — mask stays NoneRecommendation: Consider building the mask at the end of start_simulation() instead of lazily, or add explicit state tracking to distinguish "not yet built" from "no cables present".
2. Hardcoded Test Asset Path (Low Priority)
File: test_cable_attachment.py, _build_cable_plug_scene()
usd_path="/home/mmichelis/Documents/IsaacLab-Origin/scripts/demos/plug_mesh_flange_only.usda"This absolute path will fail in CI or on other developers' machines.
Recommendation: Use a relative path from the test directory or a bundled test asset.
3. Missing Collision Filtering Between Attached Bodies (Medium Priority)
File: cable_object.py, apply_cable_attachments_to_builder()
The fixed joint is created with collision_filter_parent=True, which filters collisions between the cable anchor body and the target. However, if the target has multiple collision shapes, only the direct parent-child pair is filtered. Adjacent cable segments may still collide with the target's collision geometry.
Recommendation: Document this behavior or consider extending the collision filter to include the target's entire collision group.
4. Registry Entry Mutation Across Worlds (Low Priority, Defensive)
File: cable_object.py, add_cable_entry_to_builder()
if env_idx == 0:
entry.body_offsets.clear()
entry.head_segment_body_indices.clear()
entry.tail_segment_body_indices.clear()This relies on env_idx == 0 being processed first. While the current cloner guarantees this, the pattern is fragile if the world iteration order ever changes.
Recommendation: Add an assertion or use a different initialization pattern (e.g., check if lists are non-empty and raise if out-of-order).
5. sync_curves_to_usd Always Copies Full body_q (Performance)
File: vbd_manager.py, sync_curves_to_usd()
wp.copy(cls._cable_body_q_cpu, cls._state_0.body_q)This copies the entire body_q buffer to CPU every render frame, even though only cable bodies are needed.
Recommendation: For scenes with many rigid bodies and few cables, consider slicing out only the cable body indices before the copy.
6. _solver_specific_clear Doesn't Clear _pending_cable_attachments (Bug)
File: coupled_featherstone_vbd_manager.py and coupled_mjwarp_vbd_manager.py
@classmethod
def _solver_specific_clear(cls) -> None:
NewtonManager._cable_registry = []
NewtonManager._deformable_registry = []
NewtonManager._per_world_builder_hooks = []
# Missing: NewtonManager._pending_cable_attachments = []The vbd_manager.py version clears it, but the coupled managers don't. This could cause stale attachments to persist across scene resets when using coupled solvers.
Recommendation: Add NewtonManager._pending_cable_attachments = [] to both coupled manager _solver_specific_clear methods.
7. Docstring Claims (x, y, z, w) Quaternion Order (Documentation)
File: cable_object_cfg.py
cable_local_quat: tuple[float, float, float, float] = (0.0, 0.0, 0.0, 1.0)
"""Joint anchor orientation as quaternion ``(x, y, z, w)`` in the cable..."""The default (0.0, 0.0, 0.0, 1.0) and docstring correctly state (x, y, z, w). However, Isaac Lab's InitialStateCfg.rot uses (w, x, y, z) convention. This inconsistency could confuse users.
Recommendation: Add a note clarifying that this differs from the InitialStateCfg.rot convention, or consider aligning conventions.
📝 Minor Suggestions
-
Line 200 in
cable_object.py: The error message for invalidcable_anchoris good but could include the valid options. -
CableObjectCfg.attachmentstyping: Considerlist[CableAttachmentCfg] = field(default_factory=list)to avoid potential mutable default issues (thoughconfigclasslikely handles this). -
Changelog entries: The
mym-cable-attachment.minor.rstandmym-cable.minor.rstfiles are well-written and comprehensive.
Summary
This is high-quality work that follows the project's established patterns. The core attachment mechanism is sound, the multi-env handling is correct, and the test coverage is thorough. The issues identified are mostly edge cases and polish items rather than fundamental problems.
Verdict: Ready for merge after addressing the hardcoded test path (blocker for CI) and the _pending_cable_attachments clearing in coupled managers (potential bug).
Update (00f6494): New commits improve the cable_anchor API by replacing Literal["head", "tail"] with Python-style integer indexing (0 = head, -1 = tail), allowing attachment to any segment. The registry entry now stores all segment body indices in segment_body_indices: list[list[int]]. Docstrings were also trimmed for conciseness.
✅ These are clean refactors that improve API flexibility.
- Hardcoded test asset path — still references
/home/mmichelis/...(lines 435, 637, 647) _pending_cable_attachmentsnot cleared in coupled managers (coupled_featherstone_vbd_manager.py,coupled_mjwarp_vbd_manager.py)
No new issues introduced by these commits.
Update (0c389e6): Documentation-only changes — expanded the using-cables.rst guide with a new "Attaching Cables to Rigid Bodies" section, comprehensive parameter tables, a "Provisional / Experimental" note, and expanded limitation documentation. API docs updated to include CableAttachmentCfg and apply_cable_attachments_to_builder. The documentation is thorough and well-organized.
✅ No new issues. Documentation quality is excellent.
- Hardcoded test asset path
_pending_cable_attachmentsnot cleared in coupled managers
Update (70acfd0): Changed requires_graph_coloring from instance attribute to ClassVar[bool] in NewtonSolverCfg (base) and all VBD solver configs. Added improved docstring clarifying this is a solver-level property read by the cloner, not a user-tunable field.
✅ Good typing fix — ClassVar correctly marks this as class metadata rather than per-instance config.
- Hardcoded test asset path in
test_cable_attachment.py _pending_cable_attachmentsnot cleared in coupled managers
Update (4e01db5): This is a large, well-rounded commit that brings the cable attachment feature to completion:
Key Changes:
spawn_cablenow raisesRuntimeErrorunder non-Newton backends (fail-fast instead of inert geometry)- New comprehensive test suite for attachments (
test_cable_attachment.py) VBDManager.reset(soft=True)properly restores cable body state- FK mask extended to exclude
FREEjoints (not justCABLE) - New
cable_pendulum.pydemo demonstrating the attachment API - Documentation expanded with parameter tables, attachment guide, and limitation notes
✅ The changes are high quality and well-tested.
- Hardcoded test asset path —
test_cable_attachment.pystill references/home/mmichelis/Documents/IsaacLab-Origin/scripts/demos/plug_mesh_flange_only.usda(lines 435, 637, 647). This will fail in CI. _pending_cable_attachmentsnot cleared incoupled_featherstone_vbd_manager.pyandcoupled_mjwarp_vbd_manager.py— their_solver_specific_clear()methods need to includeNewtonManager._pending_cable_attachments = []to avoid stale attachments persisting across scene resets.
Update (2b2d944): Minor cosmetic changes only — added missing newline at EOF in cables.py and reformatted a long docstring in test_cable_attachment.py for line length compliance.
✅ No new issues.
- Hardcoded test asset path in
test_cable_attachment.py _pending_cable_attachmentsnot cleared in coupled managers
Update (4c210ca): Demo script cleanup — replaced local y_axis_quat() helper with isaaclab.utils.math.quat_from_angle_axis library function in cable_pendulum.py.
✅ Good refactor — uses library utilities instead of local helpers.
- Hardcoded test asset path in
test_cable_attachment.py _pending_cable_attachmentsnot cleared in coupled managers
70acfd0 to
4e01db5
Compare
…BasisCurve in USD. Currently only supported in Newton backend as add_rod_graph
4c210ca to
a1ddd58
Compare
Description
Adds a declarative way to weld a
CableObjectendpoint ("head"or"tail") to a rigid body on another spawned asset, realized as a Newton-native fixed joint at model-build time. No per-step Python synchronization is required — VBD (and other cross-articulation-aware solvers like XPBD) enforce the constraint natively each step.Continues on PR #5641.
New public API (
isaaclab_contrib.cable)CableAttachmentCfg— dataclass describing one weld:target_prim_path,cable_anchor("head"/"tail"), andcable_local_{pos,quat}/target_local_{pos,quat}for the parent/child joint frames.CableObjectCfg.attachments: list[CableAttachmentCfg]— zero or more attachments declared per cable.Cable builder hook (
cable_object.py)apply_cable_attachments_to_builderruns afteradd_registered_cables_to_builder, resolves head/tail body indices recorded on eachCableRegistryEntry, looks up the target body inbuilder.body_label(filtering bybody_worldso cloning binds to the correct env), and callsadd_joint_fixed.env_.*→env_{world_idx}) at hook time so they match the form of USD-imported sibling bodies underInteractiveScenecloning.CableRegistryEntrygainedhead_segment_body_indicesandtail_segment_body_indiceslists (one entry per world).CableObject.resetoverride (replaced by a unified VBD-wide soft reset, see below).VBD manager (
deformable/vbd_manager.py)eval_fkskip mask from "non-cable articulations" to_fk_maskcovering bothJointType.CABLEandJointType.FREEjoints — needed because rigid bodies welded to cables share the cable's articulation graph and VBD owns theirbody_qdirectly.NewtonVBDManager.reset(soft=...)override snaps every body (cable segments + rigid bodies) back to its rest-poseModel.body_qand clearsbody_q_prev,body_qd,body_inertia_qfor soft resets, replacing the previous cable-only reset.Demo
scripts/demos/cable_pendulum.py— spawns N cables, each with its tail welded to a rigid plug, optionally equally spaced; supports a wildcard-cloned variant.New tests
source/isaaclab_contrib/test/cable/test_cable_attachment.pyType of change
Checklist
pre-commitchecks with./isaaclab.sh --formatconfig/extension.tomlfileCONTRIBUTORS.mdor my name already exists there