[Draft] Apply ALM hard constraints to VBD rigid bodies#2327
[Draft] Apply ALM hard constraints to VBD rigid bodies#2327jumyungc wants to merge 61 commits intonewton-physics:mainfrom
Conversation
Signed-off-by: JC <jumyungc@nvidia.com>
Signed-off-by: JC <jumyungc@nvidia.com>
Signed-off-by: JC <jumyungc@nvidia.com>
Signed-off-by: JC <jumyungc@nvidia.com>
Signed-off-by: JC <jumyungc@nvidia.com>
Signed-off-by: JC <jumyungc@nvidia.com>
Signed-off-by: JC <jumyungc@nvidia.com>
…newton into vbd-prismatic-revolute
Signed-off-by: JC <jumyungc@nvidia.com>
Signed-off-by: JC <jumyungc@nvidia.com>
| @@ -1322,17 +1171,78 @@ def _compute_rigid_force_element_adjacency(self, model): | |||
| # Main Solver Methods | |||
| # ===================================================== | |||
|
|
|||
| def set_rigid_history_update(self, update: bool): | |||
| """Set whether the next step() should update rigid solver history (warmstarts). | |||
| def set_rigid_contact_refresh(self, refresh: bool): | |||
There was a problem hiding this comment.
🔴 Public method renamed without deprecation shim
set_rigid_history_update() is renamed to set_rigid_contact_refresh() here. The examples were updated, but any external user code calling the old name will get an AttributeError.
Please keep the old method as a thin deprecated wrapper:
| def set_rigid_contact_refresh(self, refresh: bool): | |
| def set_rigid_contact_refresh(self, refresh: bool): | |
| """Control whether step() refreshes solver contact state. |
And add a wrapper elsewhere in the class:
def set_rigid_history_update(self, update: bool):
"""Deprecated: use :meth:`set_rigid_contact_refresh` instead."""
warnings.warn(
"set_rigid_history_update() is deprecated, use set_rigid_contact_refresh() instead.",
DeprecationWarning,
stacklevel=2,
)
self.set_rigid_contact_refresh(update)
| """ | ||
| # Linear DOF (stretch) | ||
| se_ke = 1.0e9 if stretch_stiffness is None else stretch_stiffness | ||
| se_ke = 1.0e4 if stretch_stiffness is None else stretch_stiffness |
There was a problem hiding this comment.
🔴 Default stretch_stiffness silently reduced by 5 orders of magnitude
This changes the default from 1.0e9 to 1.0e4. The same change also appears in add_rod() (line 6424) and add_rod_graph() (line 6602).
Most users rely on the default since the examples omit stretch_stiffness. After this PR, every simulation using cables or rods will behave differently without any explicit opt-in. The rationale (ALM hard constraints now handle stretch enforcement) makes sense, but the silent nature of the change is risky.
At minimum this needs a Changed entry in CHANGELOG.md explaining the new default and why. Consider whether a one-release deprecation warning is warranted given the magnitude of the behavior shift, for example:
if stretch_stiffness is None:
warnings.warn(
"Default stretch_stiffness changed from 1e9 to 1e4. "
"Pass stretch_stiffness=1e4 explicitly to silence this warning.",
FutureWarning, stacklevel=2,
)
| @@ -156,6 +161,20 @@ class SolverVBD(SolverBase): | |||
| state_in, state_out = state_out, state_in | |||
| """ | |||
|
|
|||
| class JointSlot: | |||
There was a problem hiding this comment.
⚪ Minor documentation gaps in JointSlot and set_joint_hard()
Two small things:
- The
STRETCH = 0/BEND = 1aliases are cable-specific, but that is easy to miss when reading the class. A short note like "STRETCH and BEND are convenience aliases for cables only" would help. set_joint_hard()does not mention the defaults per joint type. Users need to know what they are changing from. Consider adding something like: "Cable stretch defaults to hard; cable bend defaults to soft (for Dahl friction compatibility). All other structural slots default to hard."
| self.update_rigid_history = update | ||
| self._rigid_contact_refresh = refresh | ||
|
|
||
| def set_joint_hard(self, joint_index: int, hard: bool, slot: int | None = None): |
There was a problem hiding this comment.
Maybe something like set_joint_constraint_mode is more descriptive because "set_joint_hard" can also set a soft constraint mode?
There was a problem hiding this comment.
Also the type hint for slot should be JointSlot | None to make it easier to discover possible options.
There was a problem hiding this comment.
Alternatively, could this be controlled by a custom attribute on the model? If I am reading correctly he current approach will to one device-host-device roundtrip of the whole array per joint, which might be inefficient for large scenes
jumyungc
changed the title
|
I’m working on reusing as many existing parameters as possible. I’ll share an update soon for your review. Thank you! |
| ) | ||
| # Per-step joint lambda decay + C0 snapshot. | ||
| # Uses body_q_prev (previous-step pose for kinematic bodies) which | ||
| # provides implicit feedforward tracking force -- see avbd_rigid_review.md section 6. |
There was a problem hiding this comment.
Is this .md file commited somewhere?
Would be great if the using body_q_prev was not required, this complicates coupling in general. Start of step boyd positions should always be state_0.body_q, even for kinematic bodies
|
|
||
| # Early exit: no penetration or zero stiffness | ||
| if penetration_depth <= 0.0 or contact_ke <= 0.0: | ||
| if penetration_depth <= _SMALL_LENGTH_EPS and lam_n <= 0.0: |
There was a problem hiding this comment.
Should _SMALL_LENGTH_EPS be configurable depending on scene scale?
| if f_n <= 0.0 or contact_ke <= 0.0: | ||
| if lam_n > 0.0 and hard_contact == 1: | ||
| f_n = 0.0 | ||
| k_rescaled = lam_n / wp.max(wp.abs(penetration_depth), 1.0e-8) |
There was a problem hiding this comment.
The non-monotonicity around penetration-depth=0 is suprising
| I3 = wp.identity(n=3, dtype=float) | ||
| K_total = contact_ke * n_outer + contact_ke_t * (I3 - n_outer) | ||
| else: | ||
| # Kinematic-hard or soft: normal penalty + IPC velocity-based friction |
There was a problem hiding this comment.
what's the reason for using a different model for kinematic vs dynamic for hard contacts?
| contact_material_kd: wp.array[float], | ||
| contact_material_mu: wp.array[float], | ||
| ): | ||
| """Warmstart body-body contacts using hash table-based contact history. |
There was a problem hiding this comment.
Isn't the collision pipeline supposed to provide contact matching for force warmstarting? Or are there fundamental reasons for using a custom solution here?
Signed-off-by: JC <jumyungc@nvidia.com>
Signed-off-by: JC <jumyungc@nvidia.com>
Signed-off-by: JC <jumyungc@nvidia.com>
Signed-off-by: JC <jumyungc@nvidia.com>
Signed-off-by: JC <jumyungc@nvidia.com>
Signed-off-by: JC <jumyungc@nvidia.com>
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/examples/cable/example_cable_bundle_hysteresis.py (1)
226-233:⚠️ Potential issue | 🟠 MajorThese custom
ShapeConfigs no longer inherit the updated contact defaults.Creating a fresh
ShapeConfig(...)here falls back to the class defaults for omitted fields, so the obstacles and ground now getke/kdfromShapeConfiginstead of this example'sbuilder.default_shape_cfg(1.0e5/0.0). That quietly changes the obstacle/ground contact model back to a much softer, damped configuration.Suggested fix
- obstacle_cfg = newton.ModelBuilder.ShapeConfig( - density=builder.default_shape_cfg.density, - kf=builder.default_shape_cfg.kf, - ka=builder.default_shape_cfg.ka, - mu=0.0, # Frictionless obstacles - restitution=builder.default_shape_cfg.restitution, - ) + obstacle_cfg = builder.default_shape_cfg.copy() + obstacle_cfg.mu = 0.0 # Frictionless obstacles ... - ground_cfg = newton.ModelBuilder.ShapeConfig( - density=builder.default_shape_cfg.density, - kf=builder.default_shape_cfg.kf, - ka=builder.default_shape_cfg.ka, - mu=2.5, - restitution=builder.default_shape_cfg.restitution, - ) + ground_cfg = builder.default_shape_cfg.copy() + ground_cfg.mu = 2.5Also applies to: 267-274
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/examples/cable/example_cable_bundle_hysteresis.py` around lines 226 - 233, The custom obstacle/ground ShapeConfig is being constructed fresh (variable obstacle_cfg using newton.ModelBuilder.ShapeConfig) which omits ke/kd and thus falls back to class defaults; update the construction to inherit all contact defaults from builder.default_shape_cfg (either by cloning/copying builder.default_shape_cfg or by explicitly passing ke=builder.default_shape_cfg.ke and kd=builder.default_shape_cfg.kd along with the other overrides like mu and restitution) so the obstacles/ground keep the intended ke/kd values; apply the same fix to the other similar block (the one around the later ShapeConfig at lines you noted).
♻️ Duplicate comments (1)
newton/tests/test_cable.py (1)
299-317:⚠️ Potential issue | 🟠 MajorMake
_run_sim_loop()CUDA-capture state-neutral.On CUDA,
wp.ScopedCapturerecords a real execution ofsimulate_fn(), so this helper advances the closed-over state once before thefor _ in range(num_steps)loop even starts. That already makes the CUDA path run a different number of chunks than CPU, and it still breaks replay when the captured chunk ends with odd buffer parity (for example, the gripper path at Lines 3298-3326 swapsstate0/state1exactly once). Capture against scratch state or restore the mutated state after capture, and keep graph replay disabled for odd-parity chunks.In warp-lang 1.12.0, does `wp.ScopedCapture` execute captured kernel launches and mutate the referenced arrays/state before any later `wp.capture_launch(graph)` call?🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_cable.py` around lines 299 - 317, _cuda capture currently mutates closed-over state during wp.ScopedCapture in _run_sim_loop (when use_cuda_graph is true), so the captured run advances simulation state once and makes CUDA replay diverge; fix by making capture state-neutral: before entering wp.ScopedCapture, snapshot any mutable state that simulate_fn closes over (or run simulate_fn against dedicated scratch copies) and after capture restore the original state so no mutation persists, store capture graph separately, and when replaying with wp.capture_launch(graph) detect and skip replay for chunks whose parity would cause mismatch (i.e., if the capture advanced buffers an odd number of swaps) so you only replay safe/even-parity chunks; reference functions/vars: _run_sim_loop, simulate_fn, use_cuda_graph, wp.ScopedCapture, graph, wp.capture_launch, device.is_cuda, wp.is_mempool_enabled.
🧹 Nitpick comments (1)
newton/_src/solvers/vbd/particle_vbd_kernels.py (1)
2415-2415: Unusedshape_material_muparameter after switching to_eval_body_particle_contact.The kernel signature still accepts
shape_material_mu(line 2415), but it's no longer used since the call now goes to_eval_body_particle_contactwhich expects pre-averagedcontact_mu. The friction mixing now happens upstream when populatingbody_particle_contact_material_mu.This is fine if intentionally kept for call-site compatibility during this transition, but consider removing the unused parameter in a follow-up cleanup once all callers are updated.
Also applies to: 2442-2462
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/solvers/vbd/particle_vbd_kernels.py` at line 2415, The kernel signature still includes the now-unused parameter shape_material_mu in particle_vbd_kernels (e.g., where the function calls _eval_body_particle_contact), so remove shape_material_mu from the kernel signature and all internal references, and update any callers if present to stop passing it; alternatively keep it temporarily for compatibility but add a TODO comment and ensure the actual friction value is provided via body_particle_contact_material_mu which is passed into _eval_body_particle_contact—search for shape_material_mu, _eval_body_particle_contact, and body_particle_contact_material_mu to locate and update the kernel definition and any call sites.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@newton/tests/test_cable.py`:
- Around line 3689-3696: The test is passing the pre-step pose to
collect_rigid_contact_forces causing inconsistency with the ALM state populated
by solver.step; change the call to pass the post-step transforms (use
state1.body_q instead of state0.body_q) while keeping body_q_prev_snapshot (from
solver.body_q_prev) and the rest of the arguments the same so
collect_rigid_contact_forces receives the current body transforms consistent
with the ALM penalty/lambda state.
---
Outside diff comments:
In `@newton/examples/cable/example_cable_bundle_hysteresis.py`:
- Around line 226-233: The custom obstacle/ground ShapeConfig is being
constructed fresh (variable obstacle_cfg using newton.ModelBuilder.ShapeConfig)
which omits ke/kd and thus falls back to class defaults; update the construction
to inherit all contact defaults from builder.default_shape_cfg (either by
cloning/copying builder.default_shape_cfg or by explicitly passing
ke=builder.default_shape_cfg.ke and kd=builder.default_shape_cfg.kd along with
the other overrides like mu and restitution) so the obstacles/ground keep the
intended ke/kd values; apply the same fix to the other similar block (the one
around the later ShapeConfig at lines you noted).
---
Duplicate comments:
In `@newton/tests/test_cable.py`:
- Around line 299-317: _cuda capture currently mutates closed-over state during
wp.ScopedCapture in _run_sim_loop (when use_cuda_graph is true), so the captured
run advances simulation state once and makes CUDA replay diverge; fix by making
capture state-neutral: before entering wp.ScopedCapture, snapshot any mutable
state that simulate_fn closes over (or run simulate_fn against dedicated scratch
copies) and after capture restore the original state so no mutation persists,
store capture graph separately, and when replaying with wp.capture_launch(graph)
detect and skip replay for chunks whose parity would cause mismatch (i.e., if
the capture advanced buffers an odd number of swaps) so you only replay
safe/even-parity chunks; reference functions/vars: _run_sim_loop, simulate_fn,
use_cuda_graph, wp.ScopedCapture, graph, wp.capture_launch, device.is_cuda,
wp.is_mempool_enabled.
---
Nitpick comments:
In `@newton/_src/solvers/vbd/particle_vbd_kernels.py`:
- Line 2415: The kernel signature still includes the now-unused parameter
shape_material_mu in particle_vbd_kernels (e.g., where the function calls
_eval_body_particle_contact), so remove shape_material_mu from the kernel
signature and all internal references, and update any callers if present to stop
passing it; alternatively keep it temporarily for compatibility but add a TODO
comment and ensure the actual friction value is provided via
body_particle_contact_material_mu which is passed into
_eval_body_particle_contact—search for shape_material_mu,
_eval_body_particle_contact, and body_particle_contact_material_mu to locate and
update the kernel definition and any call sites.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 8e8b3392-4672-437d-b6dc-9a3935f21166
📒 Files selected for processing (14)
CHANGELOG.mdnewton/_src/sim/builder.pynewton/_src/solvers/vbd/particle_vbd_kernels.pynewton/_src/solvers/vbd/rigid_vbd_kernels.pynewton/_src/solvers/vbd/solver_vbd.pynewton/examples/basic/example_basic_conveyor.pynewton/examples/basic/example_basic_urdf.pynewton/examples/cable/example_cable_bundle_hysteresis.pynewton/examples/cable/example_cable_pile.pynewton/examples/cable/example_cable_twist.pynewton/examples/cloth/example_cloth_franka.pynewton/examples/contacts/example_contacts_rj45_plug.pynewton/tests/test_cable.pynewton/tests/test_kinematic_links.py
💤 Files with no reviewable changes (1)
- newton/examples/cloth/example_cloth_franka.py
✅ Files skipped from review due to trivial changes (1)
- CHANGELOG.md
🚧 Files skipped from review as they are similar to previous changes (4)
- newton/examples/basic/example_basic_conveyor.py
- newton/examples/contacts/example_contacts_rj45_plug.py
- newton/examples/cable/example_cable_pile.py
- newton/_src/sim/builder.py
Signed-off-by: JC <jumyungc@nvidia.com>
Signed-off-by: JC <jumyungc@nvidia.com>
There was a problem hiding this comment.
🧹 Nitpick comments (1)
newton/examples/basic/example_basic_urdf.py (1)
89-98: Optional cleanup: hoist duplicatedupdate_step_intervalassignment.
self.update_step_interval = 1is set in both branches; moving it above the branch reduces repetition.♻️ Suggested refactor
- if self.solver_type == "vbd": - self.update_step_interval = 1 + self.update_step_interval = 1 + if self.solver_type == "vbd": self.solver = newton.solvers.SolverVBD( self.model, iterations=1, # Example-specific stabilization for low-iteration AVBD; prefer more iterations / non-zero damping. rigid_avbd_gamma=0.9, ) else: - self.update_step_interval = 1 self.solver = newton.solvers.SolverXPBD(self.model, iterations=1)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/examples/basic/example_basic_urdf.py` around lines 89 - 98, Move the duplicated assignment self.update_step_interval = 1 out of the if/else and place it immediately before the conditional so it is set once for both branches; keep the instantiation of self.solver using newton.solvers.SolverVBD(self.model, ...) and newton.solvers.SolverXPBD(self.model, ...) inside the existing branches (preserving the SolverVBD arguments like iterations=1 and rigid_avbd_gamma=0.9).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@newton/examples/basic/example_basic_urdf.py`:
- Around line 89-98: Move the duplicated assignment self.update_step_interval =
1 out of the if/else and place it immediately before the conditional so it is
set once for both branches; keep the instantiation of self.solver using
newton.solvers.SolverVBD(self.model, ...) and
newton.solvers.SolverXPBD(self.model, ...) inside the existing branches
(preserving the SolverVBD arguments like iterations=1 and rigid_avbd_gamma=0.9).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 368b77d9-0e06-483d-8aee-a7027e9e5612
📒 Files selected for processing (2)
newton/examples/basic/example_basic_urdf.pynewton/tests/test_cable.py
✅ Files skipped from review due to trivial changes (1)
- newton/tests/test_cable.py
Signed-off-by: JC <jumyungc@nvidia.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
newton/_src/sim/builder.py (1)
4202-4222:⚠️ Potential issue | 🟡 MinorUpdate stale default-stiffness documentation to
1.0e4.The implementation now defaults cable/rod stretch stiffness to
1.0e4, but these docstrings still state1.0e9and describe the rod default as “high stiffness”. That will mislead users tuning VBD/ALM rods and cables.📝 Proposed doc fix
- stretch_stiffness: Cable stretch stiffness (stored as ``target_ke``) [N/m]. If None, defaults to 1.0e9. + stretch_stiffness: Cable stretch stiffness (stored as ``target_ke``) [N/m]. If None, defaults to 1.0e4. ... - segment length. If None, defaults to 1.0e9. + segment length. If None, defaults to 1.0e4. ... - - Bend defaults are 0.0 (no bending resistance unless specified). Stretch defaults to a high - stiffness (1.0e9), which keeps neighboring capsules closely coupled (approximately inextensible). + - Bend defaults are 0.0 (no bending resistance unless specified). Stretch defaults to + 1.0e4; pass an explicit value for stiffer soft stretch behavior. ... - # Stretch defaults: high stiffness to keep neighboring capsules tightly coupled + # Stretch default used when caller omits stretch_stiffness. ... - stretch_stiffness: Material-like axial stiffness (EA) [N], normalized by edge length - into an effective joint stiffness [N/m]. Defaults to 1.0e9. + stretch_stiffness: Material-like axial stiffness (EA) [N], normalized by edge length + into an effective joint stiffness [N/m]. Defaults to 1.0e4. ... - # Stretch defaults: high stiffness to keep neighboring capsules tightly coupled + # Stretch default used when caller omits stretch_stiffness.Also applies to: 6381-6430, 6414-6417, 6582-6608
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/_src/sim/builder.py` around lines 4202 - 4222, Docstrings for cable/rod stiffness are stale: update any occurrences describing default stretch stiffness as 1.0e9 or calling the rod default “high stiffness” to the actual implementation default 1.0e4; specifically edit the parameter docs for stretch_stiffness/target_ke (and any mentions of rod/cable defaults) near where se_ke is set and where bend_stiffness/target_ke and damping/target_kd are documented (references: stretch_stiffness, bend_stiffness, stretch_damping, bend_damping, se_ke) so the text and numeric default match the code (1.0e4 for stretch) and remove or reword “high stiffness” phrasing accordingly; apply the same changes to the other docstring blocks flagged in the comment.
♻️ Duplicate comments (1)
newton/tests/test_cable.py (1)
3299-3329:⚠️ Potential issue | 🟠 MajorKeep the captured gripper step chunk buffer-parity neutral.
simulate()swapsstate0/state1once, but_run_sim_loop()captures it once and replays fixed array addresses. On CUDA graph runs this can replay from the captured input buffer instead of the latest output. Wrap the two substeps inside the captured closure, or disable graph capture for this caller.Suggested fix: capture an even two-substep frame
# Run a fixed number of frames for a lightweight regression test. num_frames = 100 - num_steps = num_frames * sim_substeps sim_time_arr = wp.zeros(1, dtype=float, device=device) def simulate(): nonlocal state0, state1 - state0.clear_forces() - - wp.launch( - kernel=_drive_gripper_boxes_graph_kernel, - dim=2, - inputs=[ - float(ramp_time), - sim_time_arr, - gripper_body_ids, - gripper_signs, - anchor_p, - anchor_q, - 0.0, # seg_half_len - float(target_offset_mag), - float(initial_offset_mag), - float(pull_start_time), - float(pull_ramp_time), - float(pull_distance), - state0.body_q, - ], - device=device, - ) - - model.collide(state0, contacts) - solver.step(state0, state1, control, contacts, sim_dt) - state0, state1 = state1, state0 - wp.launch(_advance_time, dim=1, inputs=[sim_time_arr, sim_dt], device=device) + for _substep in range(sim_substeps): + state0.clear_forces() + + wp.launch( + kernel=_drive_gripper_boxes_graph_kernel, + dim=2, + inputs=[ + float(ramp_time), + sim_time_arr, + gripper_body_ids, + gripper_signs, + anchor_p, + anchor_q, + 0.0, # seg_half_len + float(target_offset_mag), + float(initial_offset_mag), + float(pull_start_time), + float(pull_ramp_time), + float(pull_distance), + state0.body_q, + ], + device=device, + ) + + model.collide(state0, contacts) + solver.step(state0, state1, control, contacts, sim_dt) + state0, state1 = state1, state0 + wp.launch(_advance_time, dim=1, inputs=[sim_time_arr, sim_dt], device=device) - _run_sim_loop(simulate, num_steps, device) + _run_sim_loop(simulate, num_frames, device)You can verify the remaining single-swap captured closures with:
#!/bin/bash python - <<'PY' import ast from pathlib import Path path = Path("newton/tests/test_cable.py") tree = ast.parse(path.read_text(), filename=str(path)) def is_state_swap(stmt): if not isinstance(stmt, ast.Assign): return False if len(stmt.targets) != 1 or not isinstance(stmt.targets[0], ast.Tuple): return False lhs = stmt.targets[0].elts rhs = stmt.value.elts if isinstance(stmt.value, ast.Tuple) else [] return ( len(lhs) == len(rhs) == 2 and all(isinstance(n, ast.Name) for n in lhs + rhs) and [n.id for n in lhs] == ["state0", "state1"] and [n.id for n in rhs] == ["state1", "state0"] ) for fn in [n for n in tree.body if isinstance(n, ast.FunctionDef) and n.name.endswith("_impl")]: nested = {n.name: n for n in fn.body if isinstance(n, ast.FunctionDef)} run_names = set() for node in ast.walk(fn): if isinstance(node, ast.Call) and isinstance(node.func, ast.Name) and node.func.id == "_run_sim_loop": if node.args and isinstance(node.args[0], ast.Name): run_names.add(node.args[0].id) for name in run_names: sim = nested.get(name) if sim is None: continue top_level_swaps = sum(is_state_swap(stmt) for stmt in sim.body) if top_level_swaps % 2 == 1: print(f"{fn.name}.{name}: top-level captured state swap count is odd ({top_level_swaps})") PYExpected result after the fix: no output.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@newton/tests/test_cable.py` around lines 3299 - 3329, The captured closure simulate performs a single state0/state1 swap which leads to odd swap parity when captured by CUDA graphs; make the closure parity-neutral by ensuring simulate performs an even number of swaps per invocation (e.g., perform the two substeps inside the closure so you do two solver steps + two swaps, or add a compensating swap before exiting simulate), updating the simulate function that launches _drive_gripper_boxes_graph_kernel, calls model.collide/solver.step and launches _advance_time so that the top-level swap count is even (refer to simulate, _run_sim_loop, _drive_gripper_boxes_graph_kernel and _advance_time to locate and modify the captured closure).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@newton/_src/sim/builder.py`:
- Around line 4202-4222: Docstrings for cable/rod stiffness are stale: update
any occurrences describing default stretch stiffness as 1.0e9 or calling the rod
default “high stiffness” to the actual implementation default 1.0e4;
specifically edit the parameter docs for stretch_stiffness/target_ke (and any
mentions of rod/cable defaults) near where se_ke is set and where
bend_stiffness/target_ke and damping/target_kd are documented (references:
stretch_stiffness, bend_stiffness, stretch_damping, bend_damping, se_ke) so the
text and numeric default match the code (1.0e4 for stretch) and remove or reword
“high stiffness” phrasing accordingly; apply the same changes to the other
docstring blocks flagged in the comment.
---
Duplicate comments:
In `@newton/tests/test_cable.py`:
- Around line 3299-3329: The captured closure simulate performs a single
state0/state1 swap which leads to odd swap parity when captured by CUDA graphs;
make the closure parity-neutral by ensuring simulate performs an even number of
swaps per invocation (e.g., perform the two substeps inside the closure so you
do two solver steps + two swaps, or add a compensating swap before exiting
simulate), updating the simulate function that launches
_drive_gripper_boxes_graph_kernel, calls model.collide/solver.step and launches
_advance_time so that the top-level swap count is even (refer to simulate,
_run_sim_loop, _drive_gripper_boxes_graph_kernel and _advance_time to locate and
modify the captured closure).
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro
Run ID: 5fcf86c5-4f0c-4068-af06-79cb141cd8dc
📒 Files selected for processing (5)
CHANGELOG.mdnewton/_src/sim/builder.pynewton/_src/solvers/vbd/rigid_vbd_kernels.pynewton/_src/solvers/vbd/solver_vbd.pynewton/tests/test_cable.py
🚧 Files skipped from review as they are similar to previous changes (1)
- CHANGELOG.md
5 participants
Description
Included:
Not included:
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
Bug Fixes
Tests
Chores
Documentation