Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Kinetics and Static Friction pairwise for different materials #128

Closed
wants to merge 37 commits into from

Conversation

antoinebou12
Copy link
Contributor

@antoinebou12 antoinebou12 commented Nov 4, 2024

Description

Kinetics and Static Friction pairwise for different materials

This pull request includes several significant updates to the build configuration and friction collision handling in the IPC Toolkit. The most important changes include adding new pre-commit hooks, updating CMake options, and enhancing the friction collision classes with additional parameters.

Build Configuration Updates:

  • .pre-commit-config.yaml: Added new pre-commit hooks for clang-format and a local hook for building and running unit tests using ccache.

Friction Collision Enhancements:

Documentation:

  • README.md: Added a note about the new pairwise friction handling feature.

Fixes # (issue)

Type of change

  • Enhancement (non-breaking change which improves existing functionality)
  • New feature (non-breaking change which adds functionality)

How Has This Been Tested?

Test Configuration:

  • OS and version: WSL
  • Compile and version:

Checklist:

  • I have followed the project style guide
  • [] My code follows the clang-format style guidelines of this project
  • I have performed a self-review of my code
  • I have commented my code, particularly in hard-to-understand areas
  • I have made corresponding changes to the documentation
  • My changes generate no new warnings
  • I have added tests that prove my fix is effective or that my feature works
  • New and existing unit tests pass locally with my changes
  • Any dependent changes have been merged and published in downstream modules

@zfergus
Copy link
Member

zfergus commented Nov 5, 2024

Thank you for your changes and pull request. This is a useful feature that I thought about adding but never had the time to do. I am glad you are working on it :). I will wait until you mark it as "Ready for review" to give a full review, but here are some initial comments:

  • I like the idea of CUDA being enabled by default, but it needs to be behind a check to see if CUDA is available.
  • I have always wanted to enable SIMD instruction for Eigen but have always run into memory alignment issues and crashes. I left it off by default and marked it as advanced. We can troubleshoot this to see if there is a good solution.
  • I'll make some other individual comments for the CMake changes for now and hold off on source code comments until the PR is close to being finalized.

CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@@ -0,0 +1,68 @@
{
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modify in the final version

@@ -14,11 +14,11 @@ CollisionMesh::CollisionMesh(
const Eigen::MatrixXi& faces,
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Match the ID of the collision mesh with the pairwise friction

@antoinebou12
Copy link
Contributor Author

@zfergus i gonna restart with the new changes of 1.4

@zfergus
Copy link
Member

zfergus commented Nov 11, 2024

Okay, thanks @antoinebou12. Let me know if you run into any confusion with the latest changes.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants