Support masked deformable reset in SolverVBD.reset() - #3760
Conversation
Extend SolverVBD.reset() to honor StateFlags.PARTICLE_Q / PARTICLE_QD, copying model.particle_q / particle_qd into the state for particles in the world_mask-selected worlds. Masking mirrors the rigid path: a None mask also restores global (world == -1) particles, while an explicit mask restores globals only through its final entry. One path covers both cloth and volumetric (tet) soft bodies, and it runs even when an external solver integrates the bodies or the model has no bodies. Particle and body-particle solver history is left untouched by design: particle_q_prev rebaselines from the incoming state each step, contacts rebuild per step, and tet/cloth elasticity is stateless, so no history cold-start is needed. Remove the unsupported-particle warning and document the new behavior. Add a device-side, graph-capture-safe reset_particle_state kernel that reuses the shared _reset_world_selected predicate. Cover masked cloth + tet reset (selected-only, per-flag isolation, None-includes-globals, global-only mask, wrong-device validation), the particle-only / external-rigid paths, and a captured-graph replay in test_solver_vbd. Implements newton-physics#3400.
A masked particle reset teleports particles discontinuously. The next step only refits the self-contact BVH, which keeps a valid but low-quality tree across the jump. When particle self-contact is enabled and reset restores positions, rebuild the BVH from the post-reset positions instead so query quality is preserved. The rebuild is issued immediately in reset() (not deferred into step), so the graph-captured step path stays on the capture-safe refit; a rebuild is not graph-capturable, so reset must be called outside capture when self-contact is enabled -- the usual episode-reset pattern. Regression test spies on the detector to confirm reset rebuilds once on a position-restoring reset and not on a velocity-only or flags=0 reset. Implements newton-physics#3400.
Replace the particle-only masked reset test with a combined test that puts fixed bodies, a cloth grid, and a tet soft grid in the same worlds (two local plus the global range) and drives one masked reset() across both sides: BODY_Q/PARTICLE_Q restore positions and BODY_QD/PARTICLE_QD velocities in lockstep, world_mask=None includes globals, and an explicit mask's final entry selects only globals. This exercises the shared world-mask selection for rigid and particle state through a single call, complementing _rigid_reset_state_and_history (rigid history and pose-deferral depth) which is left untouched. Wrong-device particle validation is preserved. Implements newton-physics#3400.
|
Important Review skippedNo new commits to review since the last review. ⚙️ Run configurationConfiguration used: Path: .coderabbit.yml Review profile: CHILL Plan: Pro Plus Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughChangesVBD particle reset
Estimated code review effort: 3 (Moderate) | ~20 minutes Sequence Diagram(s)sequenceDiagram
participant Caller
participant SolverVBD
participant reset_particle_state
participant SelfContactBVH
Caller->>SolverVBD: reset flags and world mask
SolverVBD->>reset_particle_state: restore selected particle state
reset_particle_state-->>SolverVBD: updated positions and velocities
SolverVBD->>SelfContactBVH: rebuild after position reset
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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
newton/_src/solvers/vbd/particle_vbd_kernels.py (1)
1352-1377: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAvoid a Sphinx
:func:cross-reference to a private helper.The docstring references
:func:_reset_world_selected``. This function has a leading underscore and lives innewton/_src/solvers/vbd/rigid_vbd_kernels.py, an internal module. It has no public Sphinx target, so the cross-reference likely does not resolve. Use an inline code literal instead.Based on learnings, "avoid Sphinx cross-references (:func:) to
parse_usdin docstrings... use inline code literals (e.g.,parse_usd()) rather than:func:references, since there is no valid public Sphinx target for this private function." As per coding guidelines, "In docstrings, use Sphinx cross-references with the shortest possible targets, prefer public API paths, and never referencenewton._src."📝 Proposed fix
- A non-null ``particle_q`` / ``particle_qd`` output is the caller's request to - reset that field. The shared :func:`_reset_world_selected` predicate applies - the same per-world masking as the rigid reset, mapping global particles - (``particle_world == -1``) to the mask's final slot. + A non-null ``particle_q`` / ``particle_qd`` output is the caller's request to + reset that field. The shared ``_reset_world_selected`` predicate applies + the same per-world masking as the rigid reset, mapping global particles + (``particle_world == -1``) to the mask's final slot.🤖 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/_src/solvers/vbd/particle_vbd_kernels.py` around lines 1352 - 1377, Update the reset_particle_state docstring to replace the Sphinx :func: reference to the private _reset_world_selected helper with an inline code literal, such as ``_reset_world_selected()``; leave the reset behavior and surrounding documentation unchanged.Sources: Coding guidelines, Learnings
🤖 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/_src/solvers/vbd/particle_vbd_kernels.py`:
- Around line 1352-1377: Update the reset_particle_state docstring to replace
the Sphinx :func: reference to the private _reset_world_selected helper with an
inline code literal, such as ``_reset_world_selected()``; leave the reset
behavior and surrounding documentation unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yml
Review profile: CHILL
Plan: Pro Plus
Run ID: b326cda8-0709-4dcf-8a1f-da24334958a3
📒 Files selected for processing (4)
CHANGELOG.mdnewton/_src/solvers/vbd/particle_vbd_kernels.pynewton/_src/solvers/vbd/solver_vbd.pynewton/tests/test_solver_vbd.py
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
|
🔵 Nice to see One thing to clean up before merge: the new reset docstring describes a graph-capture limitation that does not match how the self-contact BVH rebuild actually behaves. Details inline. |
| stateless, so no particle history cold-start is required. When particle | ||
| self-contact is enabled, reset rebuilds the self-contact BVH from the | ||
| post-reset positions so the discontinuous jump does not degrade tree | ||
| quality; a rebuild is not graph-capturable, so issue reset outside capture | ||
| in that case. |
There was a problem hiding this comment.
⚪ This says a self-contact BVH rebuild is not graph-capturable, but that path is already captured today. newton/examples/cloth/example_cloth_twist.py and newton/examples/cloth/example_cloth_rollers.py both call self.solver.rebuild_bvh(self.state_0) from inside wp.ScopedCapture(), and rebuild_bvh() is the same self.trimesh_collision_detector.rebuild(state.particle_q) call this paragraph is describing. Both examples run as CUDA tests in newton/tests/test_examples.py.
Warp backs this up: Bvh.rebuild() is documented as reusing the existing BVH buffers, with the native CUDA LBVH rebuild path being capture safe, and the detector builds its trees with the default LBVH constructor on CUDA.
The practical cost of leaving this in: someone running cloth with particle_enable_self_contact=True will move episode resets out of their captured graph for no reason, which is the opposite of what the new captured-graph reset test shows for the non-self-contact case.
| stateless, so no particle history cold-start is required. When particle | |
| self-contact is enabled, reset rebuilds the self-contact BVH from the | |
| post-reset positions so the discontinuous jump does not degrade tree | |
| quality; a rebuild is not graph-capturable, so issue reset outside capture | |
| in that case. | |
| stateless, so no particle history cold-start is required. When particle | |
| self-contact is enabled, reset rebuilds the self-contact BVH from the | |
| post-reset positions so the discontinuous jump does not degrade tree | |
| quality. |
The matching code comment around lines 1978-1980 needs the same edit. If there is a real constraint you hit here, it would be more useful to state that specific constraint instead.
Separately, worth a thought: this rebuild covers both trees across all worlds even when world_mask selects a single world, and the next step() refits from state_in.particle_q regardless, so callers who want an explicit rebuild already have the public rebuild_bvh(). Making it skippable could be a cheap win.
There was a problem hiding this comment.
@AnkaChan could you address this? Then we can merge.
Address CodeRabbit and Codecov feedback on PR newton-physics#3760: - reset_particle_state's docstring used a Sphinx :func: cross-reference to the private _reset_world_selected wp.func, which has no resolvable public target; replace it with an inline code literal. - Cover the PARTICLE_Q wrong-device guard in reset() -- the one patch line Codecov flagged -- with a symmetric assertion in _reset_masked_rigid_and_soft.
|
Addressed in f822fe3:
@coderabbitai please re-review. |
|
✅ Action performedReview finished.
|
2 participants
Description
SolverVBD.reset()(added in #3316) resets rigid bodies, joints, and cables perworld but ignored deformables — particle
StateFlagswere ignored and it warnedthat particle resets were unsupported. This adds first-class masked deformable
reset so cloth and volumetric soft-body particles are restored to model defaults
per world, matching the rigid-body semantics.
reset()now honorsStateFlags.PARTICLE_Q/StateFlags.PARTICLE_QD, copyingmodel.particle_q/model.particle_qdinto the state for particles in theworlds selected by
world_mask.world_mask=Nonealso restores global (world == -1) particles, while anexplicit mask restores globals only through its final entry.
when an external solver integrates the bodies or the model has no bodies.
Particle and body-particle solver history is intentionally left untouched:
particle_q_prevrebaselines from the incoming state at the start of everystep(), self-contact and body-particle contacts rebuild per step, and tet/clothelasticity is stateless — so no particle history cold-start is required. When
particle self-contact is enabled, reset rebuilds the self-contact BVH from the
restored positions so the discontinuous jump does not degrade tree quality (a
rebuild is not graph-capturable, so issue reset outside capture in that case).
Closes #3400.
Checklist
CHANGELOG.mdhas been updated (if user-facing change)Test plan
Full
test_solver_vbd.pymodule passes (89 tests). New tests:test_reset_masked_rigid_and_soft— fixed bodies + cloth + tet in shared worldsand the global range; one masked
reset()restores both rigid and particlestate together (per-field
*_Q/*_QDisolation,world_mask=Noneincludesglobals, an explicit final-entry mask selects only globals, wrong-device
validation).
test_soft_reset_particle_only_and_external— particle-only model (no bodies)and
integrate_with_external_rigid_solver=True.test_soft_reset_captured_graph_restores_particles— a capturedreset()restores selected-world defaults on replay (graph-capture safe).
test_soft_reset_rebuilds_self_contact_bvh— reset rebuilds the self-contactBVH once on a position-restoring reset, and not on a velocity-only or
flags=0reset.
New feature / API change
Summary by CodeRabbit
New Features
Bug Fixes