[Newton] Speed up deformable cloning using Newton replication - #7572
[Newton] Speed up deformable cloning using Newton replication#7572camevor wants to merge 2 commits into
Conversation
Greptile SummarySpeeds up Newton deformable startup by adding registered cloth and soft-body geometry once and replicating it across translation-compatible environments, while retaining per-world construction for incompatible transforms.
Confidence Score: 5/5The PR appears safe to merge, with no concrete changed-code failure established in the supported cloning paths. The optimized path validates hook compatibility and replicated particle counts, while incompatible transforms and hooks continue through the existing per-world fallback. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[Clone mapping and environment transforms] --> B{Single homogeneous source?}
B -->|No| F[Per-world builder loop]
B -->|Yes| C{Every hook opts in and accepts transforms?}
C -->|No| F
C -->|Yes| D[Prepare deformables once on source builder]
D --> E[Replicate source builder across worlds]
E --> G[Validate particle count and bind per-world offsets]
F --> H[Add deformables independently for each world]
G --> I[Final Newton builder]
H --> I
Reviews (1): Last reviewed commit: "Newton-side cloth replication" | Re-trigger Greptile |
![isaaclab-review-bot[bot]](https://avatars.githubusercontent.com/u/157846462?s=60&v=4){kind=small}
There was a problem hiding this comment.
Isaac Lab Review Bot
Adds an opt-in Newton replication path for compatible per-world builder hooks, allowing registered deformable meshes to be built once and replicated while retaining the per-world fallback. The new hook protocol needs clearer transform-frame documentation.
- Design and architecture: The fast path is gated by hook capability checks and deformable rotation compatibility, with post-replication particle-layout validation and fallback to the existing per-world construction path. However, the cross-package hook contract does not document that its capability callback receives transforms relative to world 0 while its preparation callback receives the absolute world-0 pose.
- API: The
replicate_builder_mappingsignature and return contract remain unchanged, and the hook opt-in is additive. Document the coordinate frames, quaternion convention, and[m]units for the newly described hook arguments so future implementations cannot silently place replicated geometry using the wrong frame. - Implementation: The deformable offset calculation accounts for pre-existing destination particles and the complete source-builder particle stride, and offsets are reset before preparation. Regression coverage exercises both translation-only replication and the rotated-world fallback. The remaining issue is the ambiguity in the documented callback argument semantics.
Minor fixes needed. Posted 1 actionable finding inline.
Automated review; human maintainers own approval decisions.
| """Replicate source builders, naming homogeneous copies at their destinations.""" | ||
| """Replicate source builders, naming homogeneous copies at their destinations. | ||
|
|
||
| A hook joins the batched path by carrying ``_can_replicate_builder(xforms)`` and |
There was a problem hiding this comment.
🔵 Suggestion · Api — Hook protocol docstring omits transform frames
The documented extension point passes mixed frames: _can_replicate_builder receives xforms relative to world 0 (line 319), while _prepare_builder_replication receives the absolute world-0 pose (xforms_np[0], line 334). The docstring names the arguments but not their frames or units, so an implementer treating source_position/source_rotation as relative (or xforms as absolute) would silently mispose replicated geometry. State both frames and [m] units here.
| if not SimulationManager._deformable_registry: | ||
| return True | ||
| rotations = xforms[:, 3:] | ||
| return bool( | ||
| np.allclose(rotations[:, :3], 0.0, atol=1.0e-6, rtol=0.0) | ||
| and np.allclose(np.abs(rotations[:, 3]), 1.0, atol=1.0e-6, rtol=0.0) | ||
| ) |
There was a problem hiding this comment.
Can be explicitely state that this function tests for unit quaternion?
| if not SimulationManager._deformable_registry: | ||
| return True |
There was a problem hiding this comment.
I'm not expert but why this is there?
| for entry, source_offset, particle_count in replicated_entries: | ||
| entry.particles_per_body = particle_count | ||
| entry.particle_offsets = [ | ||
| destination_particle_base + world * source_particle_stride + source_offset | ||
| for world in range(num_worlds) | ||
| ] |
There was a problem hiding this comment.
Could probably be vectorized?
3 participants
Description
Speeds up Newton cloth and soft-body startup by replicating compatible deformables instead of rebuilding them per environment. Retains the existing fallback for rotated or incompatible environments and adds regression coverage for both paths.
Type of change
Release backport
developChecklist
Docker and GPU tests run on demand. Push the commits you want tested, then
comment
run-cion the pull request.pre-commitchecks with./isaaclab.sh --formatsource/<pkg>/changelog.d/for every touched package (do not editCHANGELOG.rstor bumpextension.toml— CI handles that)CONTRIBUTORS.mdor my name already exists there