Fix ambiguities in spec inertial documentation #990

scpeters · 2022-04-19T01:23:17Z

🎉 New feature

Summary

As noted in gazebosim/sdf_tutorials#70, there are some ambiguities in the definition of inertial properties in the SDFormat specification, particularly the pose and inertia tensor terms. Thanks to @mitiguy, @sherm1, and @EricCousineau-TRI for providing the text of these changes.

These changes should be propagated to earlier versions of the spec, both in the other folders of the sdf12 branch and to earlier supported branches such as sdf9 and sdf6. It also needs to be tested for its rendering on sdformat.org on the following page:

http://sdformat.org/spec?ver=1.9&elem=link#link_inertial

I have marked this as a draft pull request for now. Once the text has been approved, I will propagate the changes to the other files as needed.

Test it

Read the documentation
TODO: test its rendering on sdformat.org

Checklist

Signed all commits for DCO
Added tests
Added example and/or tutorial
Updated documentation (as needed)
Updated migration guide (as needed)
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.

https://reviewable.io/reviews/ignitionrobotics/sdformat/990#-

This change is

This helps resolve ambiguities in the SDF <inertial> tag by clarifying the link’s center of mass location, explicitly states that the inertia is always about the link’s center of mass, clarifies directions associated with moments/products of inertia, and clarifies the ± sign convention for product of inertia. Thanks to Paul Mitiguy and Michael Sherman from Toyota Research Institute for these improvements. Signed-off-by: Steve Peters <[email protected]>

Signed-off-by: Steve Peters <[email protected]>

codecov-commenter · 2022-04-19T02:29:35Z

Codecov Report

Merging #990 (70fa638) into sdf12 (12e7782) will not change coverage.
The diff coverage is n/a.

❗ Current head 70fa638 differs from pull request most recent head 4f9487f. Consider uploading reports for the commit 4f9487f to get more accurate results

@@           Coverage Diff           @@
##            sdf12     #990   +/-   ##
=======================================
  Coverage   65.38%   65.38%           
=======================================
  Files           2        2           
  Lines          26       26           
=======================================
  Hits           17       17           
  Misses          9        9

Impacted Files	Coverage Δ
src/EmbeddedSdf.cc	`100.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 12e7782...4f9487f. Read the comment docs.

EricCousineau-TRI · 2022-04-19T15:14:07Z

sdf/1.9/inertial.sdf

    </element>
    <element name="ixy" type="double" default="0.0" required="1">
-      <description></description>
+      <description>
+        The link’s product of inertia about Co (the link’s center of mass) for


nit (here and elsewhere) link’s => link's (different character?)

Signed-off-by: Steve Peters <[email protected]>

traversaro · 2022-04-19T20:11:12Z

sdf/1.9/inertial.sdf

+      convention for product of inertia, align Ĉx, Ĉy, Ĉᴢ with principal
+      inertia directions so that all the products of inertia are zero.
+      For more information about this sign convention, see the following
+      Mathworks documentation for working with CAD tools:


Nitpick: The company is tipically indicated with the W capitalized, see https://www.mathworks.com/ .

Suggested change

Mathworks documentation for working with CAD tools:

MathWorks documentation for working with CAD tools:

EricCousineau-TRI

Reviewed 1 of 1 files at r2, all commit messages.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @scpeters)

Signed-off-by: Steve Peters <[email protected]> Co-authored-by: Silvio Traversaro <[email protected]>

scpeters

Reviewable status: 0 of 1 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI and @traversaro)

sdf/1.9/inertial.sdf, line 51 at r2 (raw file):

Previously, traversaro (Silvio Traversaro) wrote…

Nitpick: The company is tipically indicated with the W capitalized, see https://www.mathworks.com/ .
      MathWorks documentation for working with CAD tools:

Ok, thanks! I applied your suggestion in 1c5df36

mitiguy

This looks great. Thank you Silvio, Eric, Steve, Addisu, and all. Much appreciated.

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed (commit messages unreviewed), 1 unresolved discussion (waiting on @traversaro)

scpeters · 2022-04-21T19:04:56Z

It also needs to be tested for its rendering on sdformat.org on the following page:

http://sdformat.org/spec?ver=1.9&elem=link#link_inertial

I just tested it, and I think there are some significant issues with URLs based on how we are rendering the content in https://sdformat.org/spec

They can be seen in the current form by interacting with the //link/pose documentation:

http://sdformat.org/spec?ver=1.9&elem=link#link_pose

That documentation contains a link to http://sdformat.org/tutorials?tut=specify_pose but it is not clickable in my tests. None of the text is even selectable. Attempting to select text will drag the whole block, which drops a link to that portion of the page if you drop it into a text document.

So the text of these new URLs is visible but not clickable or selectable. This should be remedied on the web app code.

scpeters · 2022-04-21T19:10:26Z

quick side-by-side comparison with this branch on the left and the existing display on the right

EricCousineau-TRI · 2022-04-30T01:59:03Z

I think even w/ rendering ~~fix~~ bug, the docs look better.
Perhaps this could merge, then we fix the rendering as a separate task?

scpeters · 2022-05-03T04:47:04Z

I think even w/ rendering ~~fix~~ bug, the docs look better.
Perhaps this could merge, then we fix the rendering as a separate task?

Sure, I'll work on replicating the changes to the other spec folders

@frame

Versions 1.5 - 1.7 will be addressed in a separate commit since those versions have an //inertial/pose/@frame or //inertial/pose/@relative_to attribute that is not present in 1.4, 1.8, or 1.9. Signed-off-by: Steve Peters <[email protected]>

@frame

The changes to inertial.sdf are copied with slight modifications to account to the presence of //inertial/pose/@frame in 1.5, 1.6 and //inertial/pose/@relative_to in 1.7. Signed-off-by: Steve Peters <[email protected]>

scpeters · 2022-05-03T05:14:56Z

Sure, I'll work on replicating the changes to the other spec folders

I've copied the changes back to spec version 1.4 in 43f2f84 and fc0ba0a. I adjusted the wording slightly in spec versions 1.5, 1.6, and 1.7 to account for the presence of //inertial/pose/@frame and //inertial/pose/@relative_to attributes in those versions of the spec that were removed in spec version 1.8.

azeey · 2022-05-04T16:25:28Z

sdf/1.4/inertial.sdf

+      vectors. The subsequent values characterize C's orientation relative to
+      link-frame L as a sequence of Euler rotations
+      (r p y) documented in http://sdformat.org/tutorials?tut=specify_pose,
+      or as a quaternion (x y z w), where w is the scalar component.


We only support quaternions in >= SDFormat 1.9

Please tell me the quaternion is given as (w x y z)!

To be consistent with ROS and Ignition/Gazebo, we went with xyzw gazebosim/sdf_tutorials#56 (comment)

fixed in 2a62ad1

:( but consistency is a good argument. Eigen stores it that way also but was apparently so ashamed that they hid it by having their Quaternion constructor take w,x,y,z!

I hear your pain, but at least the convention is specified in the @rotation_format attribute as quat_xyzw.

azeey · 2022-05-04T16:48:20Z

sdf/1.8/inertial.sdf

+      or as a quaternion (x y z w), where w is the scalar component.
+    </description>
+
+    <attribute name="rotation_format" type="string" default="euler_rpy" required="0">


The attributes rotation_format and degree were added to 1.9. Was it intentional to add them in sdf/1.8/inerital.sdf?

nope, not intentional. removed in 2a62ad1

Signed-off-by: Steve Peters <[email protected]>

scpeters added 3 commits April 17, 2022 02:00

Add missing xml closing tag

9e9ce18

Signed-off-by: Steve Peters <[email protected]>

Add link to Mathworks documentation

0a57cd6

Signed-off-by: Steve Peters <[email protected]>

github-actions bot added the 🏯 fortress Ignition Fortress label Apr 19, 2022

EricCousineau-TRI reviewed Apr 19, 2022

View reviewed changes

scpeters added 2 commits April 19, 2022 12:26

Replace ’ with more standard ' character

77630dd

Signed-off-by: Steve Peters <[email protected]>

Merge branch 'sdf12' into formalize_inertial_spec_doc

a812c25

traversaro reviewed Apr 19, 2022

View reviewed changes

EricCousineau-TRI reviewed Apr 19, 2022

View reviewed changes

EricCousineau-TRI approved these changes Apr 19, 2022

View reviewed changes

Fix MathWorks capitalization

1c5df36

Signed-off-by: Steve Peters <[email protected]> Co-authored-by: Silvio Traversaro <[email protected]>

scpeters commented Apr 19, 2022

View reviewed changes

mitiguy reviewed Apr 20, 2022

View reviewed changes

mitiguy mentioned this pull request Apr 26, 2022

Update SDF <inertial> sub-tags <pose> and <inertia> RobotLocomotion/drake#16935

Closed

scpeters added 3 commits May 2, 2022 22:02

Copy changes to spec versions 1.4, 1.8

43f2f84

Versions 1.5 - 1.7 will be addressed in a separate commit since those versions have an //inertial/pose/@frame or //inertial/pose/@relative_to attribute that is not present in 1.4, 1.8, or 1.9. Signed-off-by: Steve Peters <[email protected]>

Copy changes to spec version 1.5, 1.6, 1.7

fc0ba0a

The changes to inertial.sdf are copied with slight modifications to account to the presence of //inertial/pose/@frame in 1.5, 1.6 and //inertial/pose/@relative_to in 1.7. Signed-off-by: Steve Peters <[email protected]>

Merge branch 'sdf12' into formalize_inertial_spec_doc

4f9487f

scpeters marked this pull request as ready for review May 3, 2022 05:12

scpeters requested a review from azeey as a code owner May 3, 2022 05:12

azeey requested changes May 4, 2022

View reviewed changes

Revert quaternions in older spec

2a62ad1

Signed-off-by: Steve Peters <[email protected]>

azeey approved these changes May 4, 2022

View reviewed changes

Merge branch 'sdf12' into formalize_inertial_spec_doc

0538d73

scpeters merged commit 6424ffa into gazebosim:sdf12 May 6, 2022

scpeters deleted the formalize_inertial_spec_doc branch May 6, 2022 00:58

scpeters mentioned this pull request May 6, 2022

spec: Should fix ambiguities for //inertial/{pose,mass,inertia} gazebosim/sdf_tutorials#70

Closed

2 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix ambiguities in spec inertial documentation #990

Fix ambiguities in spec inertial documentation #990

scpeters commented Apr 19, 2022 •

edited

Loading

codecov-commenter commented Apr 19, 2022 •

edited

Loading

EricCousineau-TRI Apr 19, 2022

scpeters Apr 19, 2022

traversaro Apr 19, 2022

EricCousineau-TRI left a comment

scpeters left a comment

mitiguy left a comment

scpeters commented Apr 21, 2022

scpeters commented Apr 21, 2022

EricCousineau-TRI commented Apr 30, 2022 •

edited

Loading

scpeters commented May 3, 2022

scpeters commented May 3, 2022

azeey May 4, 2022

sherm1 May 4, 2022

azeey May 4, 2022

scpeters May 4, 2022

sherm1 May 4, 2022

scpeters May 4, 2022

azeey May 4, 2022

scpeters May 4, 2022

	Mathworks documentation for working with CAD tools:
	MathWorks documentation for working with CAD tools:

Fix ambiguities in spec inertial documentation #990

Fix ambiguities in spec inertial documentation #990

Conversation

scpeters commented Apr 19, 2022 • edited Loading

🎉 New feature

Summary

Test it

Checklist

codecov-commenter commented Apr 19, 2022 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

EricCousineau-TRI left a comment

Choose a reason for hiding this comment

scpeters left a comment

Choose a reason for hiding this comment

mitiguy left a comment

Choose a reason for hiding this comment

scpeters commented Apr 21, 2022

scpeters commented Apr 21, 2022

EricCousineau-TRI commented Apr 30, 2022 • edited Loading

scpeters commented May 3, 2022

scpeters commented May 3, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

scpeters commented Apr 19, 2022 •

edited

Loading

codecov-commenter commented Apr 19, 2022 •

edited

Loading

EricCousineau-TRI commented Apr 30, 2022 •

edited

Loading