search

ami-iit / jaxsim

A differentiable physics engine and multibody dynamics library for control and robot learning.
https://jaxsim.readthedocs.io/
BSD 3-Clause "New" or "Revised" License
57 stars 9 forks source link

Add `ContactModel` base class and abstract contact handling in `JaxSimModel` and `JaxSimModelData` #178

Closed flferretti closed 2 weeks ago

flferretti commented 3 weeks ago

This pull request refactors the contact models to enhance flexibility and future extensibility. Key changes include:

These changes allow specifying different contact models during instantiation, facilitating the development of additional contact models in the future.

C.C. @xela-95

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

flferretti commented 3 weeks ago

For some reason, CI is failing only in Python 3.10 (https://github.com/ami-iit/jaxsim/actions/runs/9511311149)

diegoferigo commented 3 weeks ago

For some reason, CI is failing only in Python 3.10 (https://github.com/ami-iit/jaxsim/actions/runs/9511311149)

Mmh, since it is related to hash and equality, maybe we can wait to merge #179.

flferretti commented 2 weeks ago

What if we leave the actual soft-contact class defining the algorithm in jaxsim.rbda? If we plan to add multiple contact models, I'm ok in adding a new subpackage jaxsim.rbda.contacts.{soft|...}.

Done in https://github.com/ami-iit/jaxsim/pull/178/commits/29991984118fc7e504f164a2c7e7c5f59ce45b52

flferretti commented 2 weeks ago

Can you double check in our codebase where JaxSimModel.build is called? For sure, the new JaxSimModel.contact_model attribute has to be copied when the model is reduced here.

The only part in which the contact_model is not specified is in the test, yet this is not a problem since the soft contact model is the default value

flferretti commented 2 weeks ago

I added the pass statements and the rest of the suggestions in https://github.com/ami-iit/jaxsim/pull/178/commits/ab7a688aba91545e6372f9dac68436dd4efb5b27

diegoferigo commented 2 weeks ago

Can you double check in our codebase where JaxSimModel.build is called? For sure, the new JaxSimModel.contact_model attribute has to be copied when the model is reduced here.

The only part in which the contact_model is not specified is in the test, yet this is not a problem since the soft contact model is the default value

Ok. Instead, regarding the model reduction, I meant that something to prevent a problem similar to #168 is still missing, right?

flferretti commented 2 weeks ago

Can you double check in our codebase where JaxSimModel.build is called? For sure, the new JaxSimModel.contact_model attribute has to be copied when the model is reduced here.

The only part in which the contact_model is not specified is in the test, yet this is not a problem since the soft contact model is the default value

Ok. Instead, regarding the model reduction, I meant that something to prevent a problem similar to #168 is still missing, right?

It's indeed there https://github.com/ami-iit/jaxsim/blob/dfebf773d924ad6ab67b91891b28cc4281dbc369/src/jaxsim/api/model.py#L369

diegoferigo commented 2 weeks ago

Can you double check in our codebase where JaxSimModel.build is called? For sure, the new JaxSimModel.contact_model attribute has to be copied when the model is reduced here.

The only part in which the contact_model is not specified is in the test, yet this is not a problem since the soft contact model is the default value

Ok. Instead, regarding the model reduction, I meant that something to prevent a problem similar to #168 is still missing, right?

It's indeed there

https://github.com/ami-iit/jaxsim/blob/dfebf773d924ad6ab67b91891b28cc4281dbc369/src/jaxsim/api/model.py#L369

Woops, sorry my bad. All good then, thanks!