-
Notifications
You must be signed in to change notification settings - Fork 290
Add coupler constraint support and example #2115
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
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2115 +/- ##
==========================================
+ Coverage 61.78% 61.88% +0.09%
==========================================
Files 393 394 +1
Lines 33331 33515 +184
Branches 4126 4151 +25
==========================================
+ Hits 20594 20740 +146
- Misses 12737 12775 +38
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
💡 Codex Review
Here are some automated review suggestions for this pull request.
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
|
cc @scpeters |
d063a14 to
45589ba
Compare
Signed-off-by: Aditya Pande <aditya0509951@gmail.com>
Signed-off-by: Aditya Pande <aditya050995@gmail.com>
45589ba to
56246e7
Compare
scpeters
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks for making progress here @jslee02! I'll test it shortly; these are just some comments from reading through the changes and comparing the new constraint files to the Mimic constraint
|
I started testing with gz-physics (see gazebosim/gz-physics#517 (comment)), but don't have it working yet |
|
@scpeters Thanks for reviewing and testing this! This PR isn’t ready yet. I’ll mark it as “Ready for review” once it is. Sorry for the confusion! |
|
@scpeters, quick update now that everything’s in place. thanks again for the detailed review and testing! I’ve pushed changes that address each of your comments: both skeletons now have their constraint impulses cleared before unit impulses, the const_cast usage is centralized via local pointers, every assert is upgraded to DART_ASSERT, and there’s a new COUPLER_CONSTRAINT_APPLY_IMPULSE test so the bilateral bookkeeping stays covered. I also refreshed the GUI example ( Screencast.from.2025-11-18.06-16-25.mp4 |
|
This seems to work functionally, but adding a constraint-type-specific flag directly onto JointAspect feels clunky. Let me spend some time refactoring so we have a more scalable way to plug in future joint constraints without touching the shared aspect. |
|
Let's continue in #2212 |
CouplerConstraintso mimic joints can optionally be enforced bilaterally and ensure the constraint solver instantiates it only whenJoint::setUseCouplerConstraint(true)is setTesting
pixi run test-allpixi run ex coupler_constraintBefore creating a pull request
pixi run test-allto lint, build, and test your changes