Vbd Spring Constraint#292
Conversation
|
@AnkaChan There are now some merge conflicts to resolve |
14d829a to
0be87c7
Compare
📝 Walkthrough""" WalkthroughSpring-based force and Hessian computations have been integrated into the VBD solver. This includes extending data structures and kernels to support vertex-spring adjacency, implementing spring force and Hessian evaluation, and invoking these computations during simulation steps. New tests for cloth stitching using springs were added, along with a corresponding experiment setup. Changes
Sequence Diagram(s)sequenceDiagram
participant SimLoop as Simulation Loop
participant Adjacency as ForceElementAdjacencyInfo
participant Kernels as Spring Kernels
participant Accumulators as Force/Hessian Accumulators
SimLoop->>Adjacency: Check for springs, get adjacency info
SimLoop->>Kernels: Launch accumulate_spring_force_and_hessian per color group
Kernels->>Accumulators: Compute and accumulate spring force & Hessian
SimLoop->>Accumulators: Integrate results into solver
sequenceDiagram
participant Test as test_cloth_stitching
participant ClothSim as ClothSim
participant Model as Model
Test->>ClothSim: Initialize with set_up_stitching_experiment
ClothSim->>Model: Configure mesh, springs, materials
Test->>ClothSim: Run simulation
Test->>Model: Assert spring-connected particles are close
Estimated code review effort🎯 4 (Complex) | ⏱️ ~40 minutes Possibly related PRs
Suggested reviewers
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. 📜 Recent review detailsConfiguration used: .coderabbit.yml 📥 CommitsReviewing files that changed from the base of the PR and between 5f05c18d9c5d9dbf0d0993270ccb4d00053010ae and bcd89f6. 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
⏰ Context from checks skipped due to timeout of 900000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
0be87c7 to
5f05c18
Compare
I think I fixed it. |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
newton/solvers/vbd/solver_vbd.py (1)
78-90: Device consistency check can be optimized.The current implementation only checks if the device matches for
v_adj_faces. For better robustness, you should check the device of any array in the struct, or better yet, add adeviceproperty to the struct.Consider adding a device property to the struct:
def to(self, device): - if device == self.v_adj_faces.device: + if device == self.v_adj_faces.device: # Consider adding self.device property instead return self
📜 Review details
Configuration used: .coderabbit.yml
Review profile: CHILL
Plan: Pro
📥 Commits
Reviewing files that changed from the base of the PR and between fddb0f4 and 5f05c18d9c5d9dbf0d0993270ccb4d00053010ae.
📒 Files selected for processing (2)
newton/solvers/vbd/solver_vbd.py(9 hunks)newton/tests/test_cloth.py(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
newton/solvers/vbd/solver_vbd.py (2)
newton/solvers/solver.py (1)
device(170-177)newton/sim/builder.py (2)
spring_count(462-463)color(3314-3355)
🪛 GitHub Actions: Pull Request
newton/tests/test_cloth.py
[error] 1092-1092: NameError: name 'self' is not defined in test_cloth_stitching_vbd_cpu test.
[error] 1092-1092: NameError: name 'self' is not defined in test_cloth_stitching_xpbd_cpu test.
🪛 GitHub Actions: GPU Unit Tests on AWS EC2
newton/tests/test_cloth.py
[error] 1092-1092: NameError: name 'self' is not defined in test_cloth_stitching_vbd_cpu test.
🔇 Additional comments (10)
newton/solvers/vbd/solver_vbd.py (8)
74-76: LGTM! Spring adjacency arrays properly added to struct.The new
v_adj_springsandv_adj_springs_offsetsarrays follow the same pattern as the existing adjacency arrays for faces and edges.
116-125: Spring adjacency access functions look correct.The functions follow the established pattern but note that
get_vertex_num_adjacent_springsdoesn't use bit shifting like the edge/face variants. This is correct since springs only store one ID per adjacency entry, not ID+order pairs.
1917-1952: Spring force and Hessian computation is mathematically sound.The implementation correctly computes elastic spring forces with damping following the VBD convention. The force sign handling and Hessian derivation are accurate.
2663-2682: Spring force integration in no-self-contact simulation is properly implemented.The kernel is correctly launched only when springs exist, using the appropriate color group dimensions.
2840-2859: Spring force integration in collision-aware simulation matches the pattern.Consistent implementation with the no-self-contact version.
2484-2485: Spring adjacency computation follows established patterns correctly.The implementation properly builds vertex-to-spring adjacency data on CPU, following the same pattern as edges and faces, with appropriate handling of empty arrays.
Also applies to: 2559-2593
3074-3107: Spring adjacency kernels are correctly implemented.Both counting and filling kernels properly handle the flattened spring array format and build the adjacency structure as expected.
1954-1994: Ensure graph coloring uses spring adjacencyThe
accumulate_spring_force_and_hessiankernel safely skips atomic operations only if no two particles in the same color share a spring. I couldn’t locate the coloring routine in the Python code—please verify that your graph-coloring step builds the conflict graph usingspring_indices(i.e., adds an edge for each spring between its two endpoint particles).
- Find the function or module that assigns
particle_ids_in_color.- Confirm it iterates over
spring_indiceswhen constructing the color graph to prevent any two connected particles from sharing a color.newton/tests/test_cloth.py (2)
705-772: LGTM! Well-structured stitching experiment setup.The method follows the established pattern of other experiment setups and correctly configures the cloth mesh with two triangles, stitching springs, and appropriate material parameters based on the solver type.
1117-1117: LGTM! Appropriate test integration.The test is correctly integrated into both "xpbd" and "vbd" solver test suites, which aligns with the spring constraint functionality being tested.
Also applies to: 1139-1139
Signed-off-by: Anka Chen <ankachan92@gmail.com>
Signed-off-by: Anka Chen <ankachan92@gmail.com>
Signed-off-by: Anka Chen <ankachan92@gmail.com>
Signed-off-by: Anka Chen <ankachan92@gmail.com>
a107d3d to
bcd89f6
Compare
Signed-off-by: Anka Chen <ankachan92@gmail.com>
# Description Don't attempt to recreate the XR anchor if one already exists. This fixes an error that occurs when multiple XR devices are created, because each one tries to create a prim at the same path so the second one fails. ## Type of change - Bug fix (non-breaking change which fixes an issue) ## Checklist - [x] I have run the [`pre-commit` checks](https://pre-commit.com/) with `./isaaclab.sh --format` - [x] I have made corresponding changes to the documentation - [x] My changes generate no new warnings - [x] I have added tests that prove my fix is effective or that my feature works - [ ] I have updated the changelog and the corresponding version in the extension's `config/extension.toml` file - [x] I have added my name to the `CONTRIBUTORS.md` or my name already exists there
Signed-off-by: Anka Chen <ankachan92@gmail.com>
Implemented Spring Constraint of VBD.
Will compute adjacent springs of each vertex, and call a separate kernel to accumulate the force and hessian of spring forces to
particle_forcesandparticle_hessians.Newton Migration Guide
Please ensure the migration guide for warp.sim users is up-to-date with the changes made in this MR.
docs/migration.rstis up-to dateBefore your PR is "Ready for review"
pre-commit run -aSummary by CodeRabbit