Remove overly cautious warning (#869)

[yurket]

Remove overly cautious warning

---

Before creating a pull request

• Document new methods and classes (N/A)
• Format new code files using clang-format (N/A)

Before merging a pull request

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

[yurket]

refers to #869

[codecov]

Codecov Report

Merging #1374 into master will decrease coverage by <.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1374      +/-   ##
==========================================
- Coverage   57.27%   57.27%   -0.01%     
==========================================
  Files         366      366              
  Lines       27432    27427       -5     
==========================================
- Hits        15711    15708       -3     
+ Misses      11721    11719       -2

Impacted Files	Coverage Δ
dart/dynamics/detail/GenericJoint.hpp	75.4% <ø> (+0.47%)	⬆️
dart/dynamics/EulerJoint.cpp	69.32% <0%> (-3.69%)	⬇️
dart/dynamics/Skeleton.cpp	66.22% <0%> (+0.16%)	⬆️

[mxgrey]

Thanks for this PR!

I think rather than just removing the warning, we should remove the entire limit check. As it is right now in this PR, the function will quit without allowing the rest position to be set outside the limits. However, there might be a legitimate use case where the rest position is outside of the limits, where the passive dynamics will clamp the joint down onto its joint limit.

For that reason, I would suggest removing all of lines 1470-1474.

[mxgrey]

Also with that change in behavior, I think it would be smart to set up a simple unit test.

The unit test could be as simple as a link attached to the world by a revolute joint where the revolute joint has some lower position limit, and its spring rest position is a lower value than its position limit (note that the spring stiffness should be something positive). Then the unit test can simulate the system for a second or two and check if the link comes to rest at its lower position limit.

[jslee02]

@mxgrey Your suggestions all sound good to me. I think we can merge this PR and address your suggestions as a separate PR.

[mxgrey]

To be honest I think this PR on its own is problematic because it removes a warning without removing behavior that is likely to surprise users. If we merge it in as-is, we risk the possibility that we forget to change this problematic behavior.

[yurket]

Agree with you, guys. Silent return doesn't look good. I'll add a test for the behavior.

[jslee02]

Thank you for your contribution. Closing in favor of #1418 that is a rework of this PR addressing @mxgrey's comments.