-
Notifications
You must be signed in to change notification settings - Fork 100
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
JointAxis: update calls to use sdf::Errors output #1145
Conversation
Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
b53d849
to
29a7e46
Compare
Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
Codecov Report
@@ Coverage Diff @@
## sdf13 #1145 +/- ##
==========================================
- Coverage 87.51% 87.47% -0.05%
==========================================
Files 126 126
Lines 16248 16363 +115
==========================================
+ Hits 14220 14313 +93
- Misses 2028 2050 +22
... and 2 files with indirect coverage changes Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
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! I've left a few test related comments, but otherwise looks good!
Signed-off-by: Marco A. Gutierrez <[email protected]>
Signed-off-by: Marco A. Gutierrez <[email protected]>
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.
It should be just these 2 lines, thanks!
src/JointAxis.cc
Outdated
limitElem->GetElement("dissipation")->Set<double>(this->Dissipation()); | ||
sdf::ElementPtr dynElem = axisElem->GetElement("dynamics", _errors); | ||
dynElem->GetElement("damping", _errors)->Set<double>( | ||
this->Damping()); |
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.
Sorry I didn't notice this last review, I think you might have missed using the new overloaded function with errors here
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.
Good catch, thanks!
src/JointAxis.cc
Outdated
dynElem->GetElement("damping", _errors)->Set<double>( | ||
this->Damping()); | ||
dynElem->GetElement("friction", _errors)->Set<double>( | ||
this->Friction()); |
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.
Same for this line as well
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.
Good catch, thanks!
Signed-off-by: Marco A. Gutierrez <[email protected]>
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.
final 2 small things, otherwise this looks great!
Signed-off-by: Marco A. Gutierrez <[email protected]>
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.
LGTM! Pending branch update and CI
Signed-off-by: Marco A. Gutierrez [email protected]
🎉 New feature
Work towards #820.
Depends on: #1141.
Summary
Adds missing
Errors
structure parameters in a few methods of the JointAxis class.Test it
Using the JointAxis class should report all errors through
sdf::Errors
if theerrors
parameter is used, it should print them otherwise.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.