[FEATURE] Enhance SAP Coupler to support self collision between FEM objects #1375
Conversation
| @ti.kernel | ||
| def detection(self, f: ti.i32): | ||
| fem_solver = self.fem_solver | ||
| pairs = ti.static(self.contact_pairs) |
There was a problem hiding this comment.
What does static does here? Why not just use self.contact_pairs directly.
There was a problem hiding this comment.
This is just a syntax sugar, make the code shorter
| def detection(self, f: ti.i32): | ||
| fem_solver = self.fem_solver | ||
| pairs = ti.static(self.contact_pairs) | ||
| sap_info = ti.static(pairs.sap_info) |
|
I have updated the code. Let me know if there's anything else need to be changed. @duburcqa |
* Delay VTK import to mitigate import failure on Linux w/o X11. * Fix 'delete_weld_constraint'. * Fix 'test_convexify'. * Disable broken 'test_query' unit test. * Improve BVH query performance. * Use 'ti.u1' for boolean flags. * Fix dtype handling. * Avoid 'numpy.flatten' for efficiency. * Remove dead code 'RigidGeom.sdf_*' methods.
|
I have pushed the lastest version @duburcqa |
|
I will have a look tomorrow morning and merge this PR if all the comments have been addressed. |
|
Take your time. I think for changes like the self.fem_solver, we should have another PR to make these changes. I have other working branches forked from this one. After I make these changes to this branch, I need to merge this branch into my other working branches which would cause many conflicts. I kind of need to change these multiple times which is quite painful. I think these kind of changes should be done when all my relevant branches are merged then I can only do them in one time. Let me know what you think @duburcqa . For these kind of change, maybe you don't need to mark them and just leave a general note for how to improve them and we can work on that in another PR. |
First, I think this kind of issues are caused by "weaknesses" in your workflow and the way your organise your branches and PRs. Ideally, your workflow should be more resilient to this kind of requests for changes, especially where it only affects fresh new code that you are pushing yourself. We would discuss this together in a video call if you are interested.
It is a bad engineering practice to merge code that is not up-to-standard under the promise that it will be fixed later on. From my own experience, even without deceptive intend from contributors, such promises tend to never be fulfilled in practice, simply because other tasks come on the way so that addressing requests for changes of some already merged PR is no longer a priority. This practice is only recommended when a repository is in a broken state for some reason and a quick-and-dirty solution must be shipped as soon as possible before anything. What being said, I'm ok to make a compromise for this time. |
I get your idea. I will update them in this PR. For issues like this, maybe only comment once and note that it occurs multiple times in the code instead of trying to exhaust them. As you can see the dialogues are already very lengthy and cumbersome to navigate through. |
|
Most comments are resolved except for syntax sugar and compute information kernels. Please see the above replies. @duburcqa |
|
@Libero0809 I will merge once the CI is passing! |
Description
Related Issue
Motivation and Context
How Has This Been / Can This Be Tested?
Added a new test.
Screenshots (if appropriate):
test_self.1.mp4
test_self.mp4
test_dragon.mp4
Checklist:
Submitting Code Changessection of CONTRIBUTING document.