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.

Sign up for GitHub

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

Jump to bottom

Reintroduce alternative contact models and streamline changes to the new API #360

Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

Reintroduce alternative contact models and streamline changes to the new API #360

wants to merge 36 commits into from

Conversation

flferretti
Copy link
Collaborator

@flferretti flferretti commented Jan 30, 2025 •
edited by github-actions bot
Loading

This PR restores the alternative contact models of the original API and streamlines changes to the rest of the codebase

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

@flferretti flferretti self-assigned this Jan 30, 2025
@flferretti flferretti force-pushed the sprint/reintroduce_contact_models branch from 5ec7f2b to a7b7150 Compare January 30, 2025 11:28
@flferretti flferretti marked this pull request as ready for review January 30, 2025 11:32
@flferretti flferretti force-pushed the sprint/reintroduce_contact_models branch 3 times, most recently from ca8e2a3 to d4ae223 Compare January 30, 2025 21:08
@flferretti
Copy link
Collaborator Author

Ready for review @ami-iit/darwin. Thanks!

xela-95
xela-95 requested changes Jan 31, 2025
View reviewed changes
Copy link
Member

@xela-95 xela-95 left a comment

Choose a reason for hiding this comment

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

Thanks @flferretti !! I left some minor comments. By the way, remember to remove the hunt_crossley_model function from the relaxed rigid contact model since now we have restored the soft contact model that has this function built in.

@xela-95
Copy link
Member

xela-95 commented Jan 31, 2025

@flferretti did you notice that here we have also the test_box_with_zero_gravity failing?

https://github.com/ami-iit/jaxsim/actions/runs/13061287183/job/36444671783?pr=360#step:11:684

@flferretti
Copy link
Collaborator Author

@flferretti did you notice that here we have also the test_box_with_zero_gravity failing?

https://github.com/ami-iit/jaxsim/actions/runs/13061287183/job/36444671783?pr=360#step:11:684

Yes, but the results are not deterministic, as the test passes if re-run with pytest --lf

xela-95
xela-95 reviewed Jan 31, 2025
View reviewed changes
src/jaxsim/api/contact.py
@@ -37,14 +37,11 @@ def collidable_point_kinematics(
the linear component of the mixed 6D frame velocity.
"""

# Switch to inertial-fixed since the RBDAs expect velocities in this representation.
with data.switch_velocity_representation(VelRepr.Inertial):
Copy link
Member

Choose a reason for hiding this comment

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

We should pay attention to this change of representation for PR #359 in which the property returns data in the active representation (not always inertial)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I'm accessing the attributes, not the property, so I believe it should always return the quantity in inertial representation, right?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I agree, it should be equivalent

Copy link
Member

Choose a reason for hiding this comment

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

yep true. Coming from other languages I don't like to access directly the "private" attributes of a class, is like coupling to the inner implementation of it. If in a next PR we change the representation in which they are stored it will be difficult to backtrace errors here, while if we use the class API we are relying on the class contract.

Copy link
Member

Choose a reason for hiding this comment

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

Good point, we should see them as protected rather than private

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Leaving unresolved for now, thanks for pointing this out

Base automatically changed from sprint/dev to main February 4, 2025 16:01
@flferretti flferretti force-pushed the sprint/reintroduce_contact_models branch from ccb1c2e to 9b2a238 Compare February 10, 2025 10:59
@flferretti flferretti changed the title [Sprint] Reintroduce alternative contact models and streamline changes to the new API Reintroduce alternative contact models and streamline changes to the new API Feb 10, 2025
younik
younik requested changes Feb 10, 2025
View reviewed changes
Copy link
Member

@younik younik left a comment

Choose a reason for hiding this comment

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

Thank you! I added some comments :)

flferretti added a commit that referenced this pull request Feb 10, 2025
@flferretti
Add alternative contact models in update_documentation
2f1a59e 
Requires #360 to avoid warnings
@flferretti flferretti force-pushed the sprint/reintroduce_contact_models branch 4 times, most recently from 6fa0a76 to a2e6a1a Compare February 10, 2025 13:52
@flferretti flferretti requested review from younik and xela-95 February 10, 2025 14:16
flferretti added a commit that referenced this pull request Feb 10, 2025
@flferretti
Add alternative contact models in update_documentation
b85b467 
Requires #360 to avoid warnings
flferretti added a commit that referenced this pull request Feb 10, 2025
@flferretti
Add alternative contact models in update_documentation
5a8e0d1 
Requires #360 to avoid warnings
flferretti added a commit that referenced this pull request Feb 10, 2025
@flferretti
Add alternative contact models in update_documentation
9a85114 
Requires #360 to avoid warnings
flferretti and others added 29 commits March 3, 2025 15:52
@flferretti
Install Gazebo SDF
9aedc95
@flferretti
Skip soft and rigid contacts model tests
662c08e
@flferretti
Pass API token for commenting PRs
9e37572
@flferretti
Ensure GPU utilization and benchmark vectorized computations
66b8c70
@flferretti
Always comment PRs and add permissions for GH pages
33628bb
@flferretti
Reintroduce soft, viscoelastic and rigid contact models
05111af 
This reverts commit 9408f74.
@flferretti
Streamline new API changes to alternative contact models
29f9c61
@flferretti
Remove viscoelastic contact model
8eef5a2
@flferretti
Convert base post impact velocity to inertial before setting
65ac30c
@flferretti
Use polymorphism for estimating good contact parameters
1d0b47c
@flferretti
Use polymorphism to update the contact state during the step
8822416
@flferretti
Refactor initialization of contact_state
e008f0d
@flferretti
Use polymorphism to update the post impact velocity
8e6c3b5
@flferretti
Make build_default_from_jaxsim_model compatible with all models
d3b8404
@flferretti
Unify api.contact_model in api.contact
6853fe3
@flferretti
Rename aux_dict to old_contact_state
39f9989
@flferretti
Make static methods functions in RigidContacts
6716a41
@flferretti
Make system_acceleration return the contact state derivative
f3ee2b6
@flferretti
Parametrize contact tests on integrators
4683e81
@flferretti
Allow replacing the contact state in JaxSimModelData
71235cc
@flferretti
Clarify that the gravity should be passed as positive constant
4f0b7df
@flferretti
Remove JIT compilation from inequality bounds function
510a20d
@flferretti
Remove unused methods in soft contacts
2f6c231
@flferretti
Speed up rigid contact model
49f062e
@flferretti
Prevent recompilation on enabled collidable points
383a15b
@flferretti
Remove duplicated Hunt-Crossley model
c853c4f
@flferretti
Fix link forces representation in rigid contacts
1dd113d
@flferretti
Move test_collidable_point_jacobians to test_api_contact
ee5d8c8
@flferretti
Lint code and remove duplications
59a2a7b
@flferretti flferretti force-pushed the sprint/reintroduce_contact_models branch from 3005e41 to 59a2a7b Compare March 3, 2025 14:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@github-actions github-actions[bot] github-actions[bot] left review comments

@CarlottaSartore CarlottaSartore CarlottaSartore requested changes

@younik younik younik approved these changes

@xela-95 xela-95 xela-95 approved these changes

Assignees

@flferretti flferretti

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@flferretti @xela-95 @CarlottaSartore @younik