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

Fix incorrect LCP construction in JointConstraint for multi-DOFs joints (#1596) #1597

Merged
merged 8 commits into from
Aug 23, 2021

Conversation

jslee02
Copy link
Member

@jslee02 jslee02 commented Aug 22, 2021


Before creating a pull request

  • Document new methods and classes
  • Format new code files using clang-format

Before merging a pull request

  • Set version target by selecting a milestone on the right side
  • Summarize this change in CHANGELOG.md
  • Add unit test(s) for this change

@jslee02 jslee02 added this to the DART 6.11.1 milestone Aug 22, 2021
@jslee02 jslee02 requested a review from scpeters August 22, 2021 17:03
@jslee02 jslee02 changed the title Fix incorrect LCP construction in JointConstraint for multi-DOFs joints Fix incorrect LCP construction in JointConstraint for multi-DOFs joints (#1596) Aug 22, 2021
Comment on lines +403 to +404
if (!mActive[i])
continue;
Copy link
Member Author

Choose a reason for hiding this comment

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

These two lines are the actual fix for #1596. Without these lines, this function attempted to access invalid indices of the arrays in lcp.

Copy link
Collaborator

Choose a reason for hiding this comment

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

👍

@jslee02 jslee02 marked this pull request as ready for review August 22, 2021 17:23
@scpeters
Copy link
Collaborator

Thanks for the incredibly quick fix and adding the test over. I would make one small adjustment to the test, since I tried reverting the two lines that fix the bug, and the test currently passes without the fix. I've widened the joint limits a bit in the following patch, which I think is an improvement because it makes the test fail on my machine without the fix, while it still passes with the fix:

diff --git a/data/sdf/test/test_issue1596.model b/data/sdf/test/test_issue1596.model
index 793b8467f7..ee7342057b 100644
--- a/data/sdf/test/test_issue1596.model
+++ b/data/sdf/test/test_issue1596.model
@@ -124,8 +124,8 @@
       <axis>
         <xyz>1 0 0</xyz>
         <limit>
-          <lower>-0.1</lower>
-          <upper>0.1</upper>
+          <lower>-1.2</lower>
+          <upper>1.2</upper>
           <stiffness>1e6</stiffness>
         </limit>
         <dynamics>
@@ -135,8 +135,8 @@
       <axis2>
         <xyz>0 1 0</xyz>
         <limit>
-          <lower>-0.1</lower>
-          <upper>0.1</upper>
+          <lower>-1.2</lower>
+          <upper>1.2</upper>
           <stiffness>1e6</stiffness>
         </limit>
         <dynamics>
@@ -155,8 +155,8 @@
       <axis>
         <xyz>1 0 0</xyz>
         <limit>
-          <lower>-0.1</lower>
-          <upper>0.1</upper>
+          <lower>-1.2</lower>
+          <upper>1.2</upper>
         </limit>
         <dynamics>
           <damping>0.1</damping>
@@ -165,8 +165,8 @@
       <axis2>
         <xyz>0 1 0</xyz>
         <limit>
-          <lower>-0.1</lower>
-          <upper>0.1</upper>
+          <lower>-1.2</lower>
+          <upper>1.2</upper>
         </limit>
         <dynamics>
           <damping>0.1</damping>

@jslee02
Copy link
Member Author

jslee02 commented Aug 22, 2021

Interesting, it was the opposite in my case. Narrowing the limits made the difference. I guess it's possible that the gravity I used (i.e., [1, 1, 0]) was different from the one in your test. Using [9.81, 9.81, 0] showed the same results as yours with the widen limits.

Copy link
Collaborator

@scpeters scpeters left a comment

Choose a reason for hiding this comment

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

I left comments about two asserts, but I leave judgment about them in your hands

aside from that, everything looks great to me, so I will mark this as approved

I can re-review if you decide to make further changes

dart/utils/sdf/SdfParser.cpp Outdated Show resolved Hide resolved
dart/utils/sdf/SdfParser.cpp Outdated Show resolved Hide resolved
@jslee02 jslee02 merged commit 763eddd into release-6.11 Aug 23, 2021
@jslee02 jslee02 deleted the fix/issue1596 branch August 23, 2021 16:46
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.

2 participants