Derive USD cable joint stiffness from local segment lengths - #3730
Derive USD cable joint stiffness from local segment lengths#3730jumyungc wants to merge 4 commits into
Conversation
Signed-off-by: JC Chang <jumyungc@nvidia.com>
📝 WalkthroughWalkthroughUSD deformable cable imports now derive stretch, shear, bend, and twist stiffness per joint from adjacent rest-segment lengths. The builder applies joint targets after rod construction, with tests and documentation covering graded cables and authored rest shapes. ChangesUSD cable stiffness
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant USDImporter
participant ModelBuilder
participant CableJointTargets
USDImporter->>ModelBuilder: construct rod or rod-graph joints
USDImporter->>USDImporter: compute adjacent rest-segment dual lengths
USDImporter->>ModelBuilder: apply material stiffness per joint
ModelBuilder->>CableJointTargets: update joint_target_ke and joint_target_mode
Possibly related issues
Possibly related PRs
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
Signed-off-by: JC Chang <jumyungc@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
newton/tests/test_import_usd_deformable_cable.py (1)
309-311: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winShorten the periodic-case comment.
This comment is useful, but it can be reduced to the non-obvious detail—the closing joint pairs the final and first segment lengths.
Suggested wording
- # A 3-4-5 triangle. add_rod appends the loop-closing joint last, and its dual length - # pairs the last and first segments (0.5 and 0.3) -- the only joint whose two operands - # are not neighbors in the body list. + # The closing joint is appended last and pairs the 0.5 and 0.3 segments.As per path instructions, inline Python comments should be brief and explain why non-obvious code exists.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@newton/tests/test_import_usd_deformable_cable.py` around lines 309 - 311, Shorten the inline comment in test_import_usd_deformable_cable.py to retain only the non-obvious behavior: the loop-closing joint pairs the final and first segment lengths. Remove the triangle explanation and other implementation details.Source: Path instructions
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@newton/tests/test_import_usd_deformable_cable.py`:
- Around line 309-311: Shorten the inline comment in
test_import_usd_deformable_cable.py to retain only the non-obvious behavior: the
loop-closing joint pairs the final and first segment lengths. Remove the
triangle explanation and other implementation details.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: bd4e4a23-bae6-4698-b49c-d7b662b0455d
📒 Files selected for processing (4)
CHANGELOG.mddocs/concepts/usd_parsing.rstnewton/_src/utils/import_usd_deformable_cable.pynewton/tests/test_import_usd_deformable_cable.py
🚧 Files skipped from review as they are similar to previous changes (2)
- docs/concepts/usd_parsing.rst
- newton/_src/utils/import_usd_deformable_cable.py
|
I haven't really reviewed this yet, but it would be good to account for the moduli->stiffness change being added to the proposal as well, @sschirm for details. |
Thanks for the heads-up! If it lands while this PR is open, I’ll update this PR accordingly. |
|
Thanks for addressing the mean-length normalization and for covering graded, periodic, welded, zero-stiffness, and standalone rest-shape cases. I ran the relevant tests locally; the PR selection passed, and the strengthened tests fail against the unfixed base as expected. I have one related request: now that the welded graph can receive per-joint stiffness, could it also retain authored I am reviewing the USD material attributes according to the proposal semantics implemented by Newton today. I would not block this PR while waiting for the possible proposal revision mentioned in the discussion. |
| # rather than changing the curve's behavior silently (a single, unwelded curve honors both). | ||
| for key in comp_paths: | ||
| kprim = curve_recs[key].prim | ||
| if deformable_read(kprim, "restShapePoints") is not None: |
There was a problem hiding this comment.
Would it be possible to preserve authored restShapePoints for welded cable graphs as part of this PR?
The previous warning made sense while add_rod_graph() could only receive one scalar stiffness, but this PR removes that limitation by applying stiffness per joint after construction. The graph also retains the owning curve and segment index for each edge in edge_owner, so it seems possible to compute a rest length for each edge from the transformed restShapePoints of that curve, while continuing to use the current points for node_positions and the constructed pose.
As written, an equivalent cable gets different normalization depending on whether it is imported standalone or as part of a welded graph: the standalone path uses authored rest lengths, while the welded path drops them and uses the possibly deformed current lengths. Could we pass those per-edge rest lengths to _apply_local_cable_stiffnesses() and replace test_welded_graph_drops_rest_shape_warns with a graded welded-rest-shape regression?
| return stretch, shear, bend, twist | ||
|
|
||
|
|
||
| def _apply_local_cable_stiffnesses( |
There was a problem hiding this comment.
Small simplification: this helper only reads ctx.builder from the import context. Would passing builder: ModelBuilder directly make the dependency clearer and keep the stiffness calculation independent of the larger import context?
Description
The experimental USD deformable importer converted cable material moduli to joint stiffness using terms like
E*A/LandE*I/L, but passed a single curve-wide mean segment length asLtoadd_rod()/add_rod_graph(), which broadcast it to every joint. A material constant therefore produced different physical stiffness depending only on how evenly the curve happenedto be sampled. The welded-graph path had the same behavior. Each joint now uses its own dual rest length,
0.5 * (L_parent + L_child)— the series compliance of the two segment halves it connects. Uniformly sampled cables are unchanged.Closes #3722.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Bug fix
Steps to reproduce:
Minimal reproduction:
New feature / API change
Summary by CodeRabbit
New Features
Changed
Documentation