URDF->SDF handle links with no inertia or small mass (backport) #1258

aaronchongth · 2023-03-22T05:50:45Z

🦟 Bug fix

Related to #199 and #1007

Summary

cherry picked from commit 6ffe669, URDF->SDF handle links with no inertia or small mass #1238

Checklist

Signed all commits for DCO
Added tests
Updated documentation (as needed)
Updated migration guide (as needed)
Consider updating Python bindings (if the library has them)
codecheck passed (See contributing)
All tests passed (See test coverage)
While waiting for a review on your PR, please help review another open pull request to support the maintainers

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.

codecov · 2023-03-22T06:06:50Z

Codecov Report

Merging #1258 (3189371) into sdf9 (88de84f) will increase coverage by 0.15%.
The diff coverage is 100.00%.

❗ Current head 3189371 differs from pull request most recent head 47d5ce1. Consider uploading reports for the commit 47d5ce1 to get more accurate results

@@            Coverage Diff             @@
##             sdf9    #1258      +/-   ##
==========================================
+ Coverage   88.35%   88.51%   +0.15%     
==========================================
  Files          63       63              
  Lines       10092    10123      +31     
==========================================
+ Hits         8917     8960      +43     
+ Misses       1175     1163      -12

Impacted Files	Coverage Δ
src/parser_urdf.cc	`85.95% <100.00%> (+0.77%)`	⬆️

... and 2 files with indirect coverage changes

* Adding tests that catch warnings when urdf have no inertia element, or if mass is too small Signed-off-by: Aaron Chong <[email protected]> * Promote link inertia and mass related urdf2sdf sdfdbg to sdfwarn with more verbose messages Signed-off-by: Aaron Chong <[email protected]> * Added flag for converting urdf links with small or no mass to frames Signed-off-by: Aaron Chong <[email protected]> * Converts urdf links with small or no mass to frames, added tests Signed-off-by: Aaron Chong <[email protected]> * Adding warning when conversion happens, added tests for small masses, being explicit about epsilon in equal Signed-off-by: Aaron Chong <[email protected]> * Fix cpplint errors Signed-off-by: Aaron Chong <[email protected]> * Adding integration test Signed-off-by: Aaron Chong <[email protected]> * Fix lint Signed-off-by: Aaron Chong <[email protected]> * Use camelCase Signed-off-by: Aaron Chong <[email protected]> * Added URDFMinimumAllowedLinkMass Signed-off-by: Aaron Chong <[email protected]> * Fix tests expecting warnings when converting to frames Signed-off-by: Aaron Chong <[email protected]> * Adding inline contains and notContains to test_utils Signed-off-by: Aaron Chong <[email protected]> * Using RedirectConsoleStream and ScopeExit Signed-off-by: Aaron Chong <[email protected]> * Refactor to use Root::LoadSdfString Signed-off-by: Aaron Chong <[email protected]> * Removing debug message when converted frame is from a root link Signed-off-by: Aaron Chong <[email protected]> * Added attached_to for frames during conversion, using < instead of math::equals Signed-off-by: Aaron Chong <[email protected]> * Update brief of URDFSetConvertLinkWithNoMassToFrame to mention case of no inertial frame Signed-off-by: Aaron Chong <[email protected]> * Remove stale commentted code Signed-off-by: Aaron Chong <[email protected]> * Update comment about the errors we are expecting Signed-off-by: Aaron Chong <[email protected]> * Convert to frame by default, remove minimal mass option, refactor implementation to handle lumping, modified unit tests Signed-off-by: Aaron Chong <[email protected]> * Only convert to frame when parent joint is fixed, attaches and transforms pose to parent link, leaves out the fixed joint Signed-off-by: Aaron Chong <[email protected]> * Rephrased conversion error, modified unit tests Signed-off-by: Aaron Chong <[email protected]> * prints zero mass errors as well when conversion to frame fails Signed-off-by: Aaron Chong <[email protected]> * integration tests that mimic the unit tests Signed-off-by: Aaron Chong <[email protected]> * Added integration test with valid and invalid use of force torque sensor where massless child links occur Signed-off-by: Aaron Chong <[email protected]> * Fix lint Signed-off-by: Aaron Chong <[email protected]> * Fix joint reduction logic, more specific error messages, more targeted unit tests for each case Signed-off-by: Aaron Chong <[email protected]> * Convert joints to frames when converting links, attach them to converted links Signed-off-by: Aaron Chong <[email protected]> * Integration tests with child fixed links as well Signed-off-by: Aaron Chong <[email protected]> * Change sdferr to sdfwarn, no way to use ParserConfig::WarningsPolicy for now Signed-off-by: Aaron Chong <[email protected]> * sdferr to sdfwarn for the case where conversion to frame is attempted Signed-off-by: Aaron Chong <[email protected]> * Removing case within converting to frame, where parent joint turns into revolute joint, that is not covered Signed-off-by: Aaron Chong <[email protected]> * Reduced scope of implementation, more specific warning messages Signed-off-by: Aaron Chong <[email protected]> * Remove mention of parser config in warning, since that is not typically used by workflows Signed-off-by: Aaron Chong <[email protected]> * Integration tests revisited and modified, removed printouts Signed-off-by: Aaron Chong <[email protected]> * Test case showing ParserConfig::URDFSetPreserveFixedJoint(true) overrides gazebo tags for joint lumping, warnings with more information and placeholder url Signed-off-by: Aaron Chong <[email protected]> * Use sdf::testing::contains instead of local contains functions in testing (#1251) Signed-off-by: Aaron Chong <[email protected]> * Remove unused InitSDF, added TODO for warning when joints are converted/dropped Signed-off-by: Aaron Chong <[email protected]> * Cleaned up if else cases Signed-off-by: Aaron Chong <[email protected]> * Using << operator of Errors Signed-off-by: Aaron Chong <[email protected]> * Replacing placeholder url with expected URL for the documentation from sdf_tutorials#88 Signed-off-by: Aaron Chong <[email protected]> * URL to tutorial as a constant, removing checking URL from test, just in case links change, we are not testing for that anyway Signed-off-by: Aaron Chong <[email protected]> * Refactored fixed child joint logic as it is never reached Signed-off-by: Aaron Chong <[email protected]> * Reduce number of if statements, renaming to only check if parent joint is reduced Signed-off-by: Aaron Chong <[email protected]> --------- Signed-off-by: Aaron Chong <[email protected]> (cherry picked from commit 6ffe669) Signed-off-by: Aaron Chong <[email protected]>

azeey

This looks good to me, but we should wait to hear from @j-rivero and @scpeters.

scpeters

this looks good to me; I've tested it with a big URDF that has massless links, and this cleans up the warnings

I see one small behavior change for small masses (less than 1e-6), but I think that's fine

scpeters · 2023-05-09T06:19:50Z

src/parser_urdf.cc

-  // must have an <inertial> block and cannot have zero mass.
-  //  allow det(I) == zero, in the case of point mass geoms.
+  // Links without an <inertial> block will be considered to have zero mass.
+  const bool linkHasZeroMass = !_link->inertial || _link->inertial->mass <= 0;


noting a slight behavior change for 0 < mass <= 1e-6:

before: sdfdbg message if gz::math::equal(mass, 0), which is true for -1e-6 <= mass <= 1e-6

after: errorStream if mass <= 0 (no error for 0 < mass <= 1e-6)

scpeters · 2023-05-09T19:25:33Z

I recommend rebase-and-merge

scpeters · 2023-05-10T02:34:35Z

do we want a review from @j-rivero or should I go ahead and merge?

j-rivero · 2023-05-10T10:34:12Z

do we want a review from @j-rivero or should I go ahead and merge?

Sorry, I have not been following the issues since I created the original PRs. I'm good if you are good ;)

scpeters · 2023-05-10T17:09:06Z

do we want a review from @j-rivero or should I go ahead and merge?

Sorry, I have not been following the issues since I created the original PRs. I'm good if you are good ;)

ok thanks, I'll go ahead and merge. I will actually squash and merge and edit the commit message since it wasn't edited when #1238 was merged

scpeters · 2023-05-10T17:46:09Z

see the edited commit message in 9d9693b; I think it's nice to do this, especially on pull requests with lots of commits

aaronchongth · 2023-05-11T07:44:02Z

got it, thanks! I will keep that in mind next time

aaronchongth requested review from azeey and scpeters as code owners March 22, 2023 05:50

github-actions bot added Gazebo 1️1️ Dependency of Gazebo classic version 11 🏰 citadel Ignition Citadel labels Mar 22, 2023

aaronchongth force-pushed the aaron/urdf-to-sdf-warnings branch from b5fcc36 to ea9b542 Compare March 22, 2023 06:47

aaronchongth mentioned this pull request Mar 24, 2023

Offer an API option to display new warnings on problems with URDF to SDF conversions #1171

Closed

8 tasks

scpeters changed the title ~~URDF->SDF handle links with no inertia or small mass (#1238)~~ URDF->SDF handle links with no inertia or small mass (backport) Mar 27, 2023

azeey approved these changes Mar 31, 2023

View reviewed changes

azeey requested a review from j-rivero April 10, 2023 18:58

scpeters approved these changes May 9, 2023

View reviewed changes

Merge branch 'sdf9' into aaron/urdf-to-sdf-warnings

47d5ce1

scpeters merged commit 9d9693b into sdf9 May 10, 2023

scpeters deleted the aaron/urdf-to-sdf-warnings branch May 10, 2023 17:44

azeey mentioned this pull request May 22, 2023

Merge sdf9 ➡️ sdf12 #1286

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

URDF->SDF handle links with no inertia or small mass (backport) #1258

URDF->SDF handle links with no inertia or small mass (backport) #1258

aaronchongth commented Mar 22, 2023 •

edited

Loading

codecov bot commented Mar 22, 2023 •

edited

Loading

azeey left a comment

scpeters left a comment

scpeters May 9, 2023

scpeters commented May 9, 2023

scpeters commented May 10, 2023

j-rivero commented May 10, 2023

scpeters commented May 10, 2023

scpeters commented May 10, 2023

aaronchongth commented May 11, 2023

URDF->SDF handle links with no inertia or small mass (backport) #1258

URDF->SDF handle links with no inertia or small mass (backport) #1258

Conversation

aaronchongth commented Mar 22, 2023 • edited Loading

🦟 Bug fix

Summary

Checklist

codecov bot commented Mar 22, 2023 • edited Loading

Codecov Report

azeey left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

scpeters May 9, 2023

Choose a reason for hiding this comment

scpeters commented May 9, 2023

scpeters commented May 10, 2023

j-rivero commented May 10, 2023

scpeters commented May 10, 2023

scpeters commented May 10, 2023

aaronchongth commented May 11, 2023

aaronchongth commented Mar 22, 2023 •

edited

Loading

codecov bot commented Mar 22, 2023 •

edited

Loading