-
Notifications
You must be signed in to change notification settings - Fork 98
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
1.10/joint.sdf: better default limit values #1112
Conversation
* Use `±inf` instead of `±1e16` for position limits * Use `inf` instead of `-1` for effort, velocity limits Signed-off-by: Steve Peters <[email protected]>
Codecov Report
@@ Coverage Diff @@
## sdf13 #1112 +/- ##
=======================================
Coverage 86.70% 86.71%
=======================================
Files 125 125
Lines 15890 15892 +2
=======================================
+ Hits 13778 13781 +3
+ Misses 2112 2111 -1
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
src/JointAxis.cc
Outdated
this->dataPtr->upper = limitElement->Get<double>("upper", 1e16).first; | ||
this->dataPtr->effort = limitElement->Get<double>("effort", -1).first; | ||
const double kInf = std::numeric_limits<double>::infinity(); | ||
this->dataPtr->lower = limitElement->Get<double>("lower", -kInf).first; |
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.
You could pass the variable itself here as the default value, so if it ever changes again, it only needs to be changed on line 64, i.e.
this->dataPtr->lower = limitElement->Get<double>("lower", -kInf).first; | |
this->dataPtr->lower = limitElement->Get<double>("lower", this->dataPtr->lower).first; |
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.
I've deduplicated the default values used in JointAxis::Load
in c7feca9
Signed-off-by: Steve Peters <[email protected]>
The use_parent_model_frame element has already been deprecated and removed, so we can remove the internal variable. Signed-off-by: Steve Peters <[email protected]>
while deduplicating some default value constants, I noticed an unused variable corresponding to the deprecated and removed |
<description>Specifies the upper joint limit (radians for revolute joints, meters for prismatic joints). Omit if joint is continuous.</description> | ||
</element> | ||
<element name="effort" type="double" default="-1" required="0"> | ||
<element name="effort" type="double" default="inf" required="0"> |
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.
The description says "Limit is not enforced if value is negative". Something to
keep in mind when updating gz-physics.
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.
this was already part of the behavior of the DOM object as of #357
Follow up to gazebosim/sdformat#1112 Signed-off-by: Steve Peters <[email protected]>
opened a PR gazebosim/gz-mujoco#106 but its tests are failing |
Follow up to gazebosim/sdformat#1112 Signed-off-by: Steve Peters <[email protected]>
🦟 Bug fix
Part of #169.
Summary
±inf
instead of±1e16
for position limitsinf
instead of-1
for effort, velocity limitsThis will require a small change in gz-physics, as illustrated by the test failure in gazebosim/gz-physics#401.
Checklist
codecheck
passed (See contributing)Note to maintainers: Remember to use Squash-Merge and edit the commit message to match the pull request summary while retaining
Signed-off-by
messages.