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

Refactor contact models and add tests #260

Merged
merged 13 commits into from
Oct 17, 2024
Merged

Conversation

diegoferigo
Copy link
Member

@diegoferigo diegoferigo commented Oct 9, 2024

I wanted to polish and uniform the great work done in the last few months about contact models. Likely, we need to keep working on them to optimize their performance, I took advantage of going through them more carefully than my previous reviews.

I have some doubts on few details of the RigidContacts and RelaxedRigidContacts. I will comment my edits in this PR to discuss my doubts and proposed changes.


📚 Documentation preview 📚: https://jaxsim--260.org.readthedocs.build//260/

@diegoferigo diegoferigo self-assigned this Oct 9, 2024
Comment on lines 494 to 546
@ jnp.linalg.inv(M_L[link_idx, :3, :3])
* (1 / jnp.diag(M_L[link_idx, 0:3, 0:3]).mean())
Copy link
Member Author

@diegoferigo diegoferigo Oct 9, 2024

Choose a reason for hiding this comment

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

@flferretti I suspect that the usage of the spatial inertia of the link $\mathbb{M}_L$ is wrong here. Accordingly to the following resources, it seems to me that Mujoco initializes it to $\text{diag}(J^\top_{W,C} M^{-1} J_{W,C})[0:3, 0:3]$ computed at $\mathbf{q}_0$ (default configuration from the URDF). It's not a big deal now, let's keep in mind to validate it as soon as possible.

Note that I didn't fix it here in this PR. We should replace M_L with something else after some investigation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Delayed to a next PR.

@diegoferigo diegoferigo force-pushed the refactor_contact_models branch from ed2b8b3 to 598abeb Compare October 9, 2024 13:39
@diegoferigo
Copy link
Member Author

In this PR, I also added some simple test of all contact models so that we at least know that we won't break anything related to them in the future. Regardless, since I touched some internal details of the RigidContacts and RelaxedRigidContacts, please @flferretti @xela-95 during your review make sure that your scripts still work as expected.

@diegoferigo diegoferigo force-pushed the refactor_contact_models branch from 598abeb to d8feb7a Compare October 9, 2024 13:58
Copy link
Collaborator

@flferretti flferretti left a comment

Choose a reason for hiding this comment

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

Just an early comment, would it make sense to include in this PR avoid repeating the contact_params attributes in JaxSimModelData and in ContactModel?

src/jaxsim/rbda/contacts/relaxed_rigid.py Show resolved Hide resolved
@diegoferigo diegoferigo force-pushed the refactor_contact_models branch 3 times, most recently from b0dbdad to 2b6f69d Compare October 10, 2024 07:55
@diegoferigo diegoferigo marked this pull request as ready for review October 10, 2024 07:55
@diegoferigo
Copy link
Member Author

@xela-95 w.r.t. to the original PR, I updated the direction of action of the baumgarte stabilization. Now it produces a force towards the terrain's normal, and this behavior is aligned with RelaxedRigidContacts. While reviewing, do you mind running the walking script with Baumgarte stabilization enabled? I'm curious to see if it works well now.

Just an early comment, would it make sense to include in this PR avoid repeating the contact_params attributes in JaxSimModelData and in ContactModel?

Let me think about it. Having nominal parameters already defined in the contact model itself is quite useful, but the dynamic override from data is not super intuitive. I'm in favor of this change, but I need to play a bit with it before proceeding. I'll try to do it in a new PR.

@diegoferigo diegoferigo changed the title Refactor contact models Refactor contact models and add tests Oct 10, 2024
src/jaxsim/api/contact.py Outdated Show resolved Hide resolved
tests/test_simulations.py Outdated Show resolved Hide resolved
tests/test_simulations.py Outdated Show resolved Hide resolved
src/jaxsim/rbda/contacts/relaxed_rigid.py Outdated Show resolved Hide resolved
@diegoferigo diegoferigo force-pushed the refactor_contact_models branch from 2b6f69d to 85a98da Compare October 10, 2024 15:37
@diegoferigo diegoferigo force-pushed the refactor_contact_models branch 3 times, most recently from 95559f1 to acae469 Compare October 10, 2024 16:27
@diegoferigo diegoferigo force-pushed the refactor_contact_models branch 2 times, most recently from a80cbaf to 2e0b3af Compare October 14, 2024 13:14
@diegoferigo diegoferigo force-pushed the refactor_contact_models branch 3 times, most recently from 822fe18 to 3ec2a87 Compare October 17, 2024 08:01
@diegoferigo diegoferigo requested review from xela-95 and flferretti and removed request for xela-95 and flferretti October 17, 2024 08:02
@diegoferigo diegoferigo force-pushed the refactor_contact_models branch from 3ec2a87 to 7b2fc3b Compare October 17, 2024 08:57
Copy link
Collaborator

@flferretti flferretti left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! I left a couple comments

src/jaxsim/rbda/contacts/relaxed_rigid.py Show resolved Hide resolved
src/jaxsim/rbda/contacts/relaxed_rigid.py Show resolved Hide resolved
src/jaxsim/rbda/contacts/relaxed_rigid.py Show resolved Hide resolved
src/jaxsim/rbda/contacts/relaxed_rigid.py Show resolved Hide resolved
src/jaxsim/rbda/contacts/relaxed_rigid.py Show resolved Hide resolved
src/jaxsim/rbda/contacts/relaxed_rigid.py Show resolved Hide resolved
src/jaxsim/rbda/contacts/rigid.py Show resolved Hide resolved
src/jaxsim/rbda/contacts/rigid.py Show resolved Hide resolved
@diegoferigo diegoferigo force-pushed the refactor_contact_models branch 2 times, most recently from f2e613f to 224f051 Compare October 17, 2024 09:14
@diegoferigo diegoferigo force-pushed the refactor_contact_models branch from 224f051 to 1e5d13f Compare October 17, 2024 09:19
@diegoferigo
Copy link
Member Author

diegoferigo commented Oct 17, 2024

@flferretti @xela-95 I removed from this PR the new logic of the RelaxedRigid contact model, only leaving some minor refactoring. I need some more time to investigate how to properly update it, as there are behaviors that do not sum up. The comodo notebook we were discussing f2f should work with these changes.

There are few PRs that depend on this one. I want to merge these changes and propose again the new logic of the RelaxedRigid contact model in a new one.

@diegoferigo diegoferigo merged commit 92d3e49 into main Oct 17, 2024
24 checks passed
@diegoferigo diegoferigo deleted the refactor_contact_models branch October 17, 2024 09:45
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.

4 participants