Update document to better reflect proposed and accepted implementations #56

azeey · 2021-09-09T17:49:27Z

This updates the proposal to reflect the accepted implementation in gazebosim/sdformat#589 and the proposed implementation in gazebosim/sdformat#690.

Preview: http://sdformat.org/tutorials?tut=better_pose_proposal&cat=pose_semantics_docs&branch=azeey%2Fbetter_pose_update

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

chapulina

LGTM in general.

I'll just point out that everything that's added to the SDF spec must be implemented by all parsers, not only libSDFormat. It's important to take this into consideration, because the more complex the spec is, the more fragmented the SDF support is in the ecosystem. For example, the SDF parser that we have in JavaScript will probably not support this proposal for a long time.

chapulina · 2021-09-09T20:30:51Z

specify_pose/proposal.md

-This proposal intends to resolve on point (1) by adding `//pose/rotation/@type`
-where degrees can be specified, and *could* address point (2) by structuring
+This proposal intends to resolve on point (1) by adding `//pose/@degrees`
+where degrees can be selected, and *could* address point (2) by structuring
 the element differently (see below).


Should the intro have some sentences about quaternions?

Yes, I think it should. I was hoping that folks who would like to see quaternions supported provide the motivations for doing so. The original proposal did not have a solid motivation for quaternions, so we decided to drop them, but we've had discussions to bring them back. It would be great if we can list out the motivations here, so we can justify the added complexity.

There is some documented support in the following issues for adding quaternion support to URDF in the following issues:

urdf xml should support quaternions ros/urdfdom#13

Add quaternion in URDF attributes ros/urdfdom#123

Add Vector4 and initQuaternions to be able to parse quaternions from URDF files ros/urdfdom_headers#51

In terms of justification, though I find Quaternions less visually inspectable, they are more straightforward to interpret than Euler angles (24 total flavors). This simplifies the portability of quaternions to other pieces of software and algorithms.

If using a camera calibration algorithm that yields Quaternion coefficients (such as doi:10.1109/TIP.2011.2164421), it is most straightforward to directly specify those coefficients in the model file, rather than requiring a user to convert them to roll-pitch-yaw angles, with the risk of calculation errors and precision loss.

shall I suggest text in this PR or open a new pull request with justification for quaternions?

Either way works, but please feel free to make direct changes to this PR.

chapulina · 2021-09-09T20:34:49Z

specify_pose/proposal.md

 angle. This is provided for legacy purposes and ease of conversion.
    * It is not suggested to use this for a text-storage format.
    * Same precision as suggested below for quaternions: Use 17 digits of
    precision, and consider separating each value on a new line.
-* `q_wxyz` - Quaternion as a 4-tuple, represented as  `(w, x, y, z)`, where `w`
+* `@rotation_format="q_wxyz"` - Quaternion as a 4-tuple, represented as  `(w, x, y, z)`, where `w`
 is the real component. This should generally be used when the rotation should be
 machine-generated (e.g. calibration artifacts).


"Should" sounds a bit too opinionated. Some other use cases for quaternions that I can think of are precision and integration with specific tools. I'd recommend rephrasing to be less specific.

I changed it to "This is the recommended format for machine-generated files (e.g. calibration artifacts)." How does that sound?

chapulina · 2021-09-09T20:36:47Z

specify_pose/proposal.md

    * This should be used when the rotation should generally be human-readable.
-* `rpy_radians` - Same as `rpy_degrees`, but with radians as the units for each
+* `@rotation_format="rpy"`, `@degrees="false"` - Same as above, but with radians as the units for each
 angle. This is provided for legacy purposes and ease of conversion.


Saying that this is provided for legacy purposes suggests that this may be deprecated in the future. Is that the case? Aren't there valid use cases for radian RPY?

👍

I would say that it is the default for legacy purposes, not that it is provided for legacy purposes

Changed to "This is the default for legacy purposes" in a258425

scpeters · 2021-09-10T04:57:19Z

specify_pose/proposal.md

    * This should be used when the rotation should generally be human-readable.
-* `rpy_radians` - Same as `rpy_degrees`, but with radians as the units for each
+* `@rotation_format="rpy"`, `@degrees="false"` - Same as above, but with radians as the units for each
 angle. This is provided for legacy purposes and ease of conversion.


👍

I would say that it is the default for legacy purposes, not that it is provided for legacy purposes

scpeters · 2021-09-10T04:59:00Z

specify_pose/proposal.md

-<pose rotation_type="rpy_radians">{xyz}  {rpy_radians}</pose>
-<pose rotation_type="rpy_degrees">{xyz}  {rpy_degrees}</pose>
+<pose rotation_format="rpy_radians">{xyz}  {rpy_radians}</pose>
+<pose rotation_format="rpy_degrees">{xyz}  {rpy_degrees}</pose>


if we are using //pose/@degrees, then I would change the @rotation_format values to euler_rpy or q_wxyz / quat_wxyz / quaternion_wxyz

this way the portion before the _ describes the rotation format (quaternion or Euler angles) and the part after the _ describes the component order

I would still only support one form of Euler angles, but ensure that the rotation order is clearly documented

euler_rpy and quat_wxyz sound good to me.

I would still only support one form of Euler angles, but ensure that the rotation order is clearly documented

Agreed. Looking at that link reminded me a recent change to the pose spec (fix in gazebosim/sdformat#698). I don't think we can completely get rid of confusion about Euler angles, but we should do our best to clearly document SDFormat's convention.

Changed to euler_rpy and quat_wxyz in a258425.

scpeters · 2021-09-10T05:01:38Z

specify_pose/proposal.md

    * This should be used when the rotation should generally be human-readable.
-* `rpy_radians` - Same as `rpy_degrees`, but with radians as the units for each
+* `@rotation_format="rpy"`, `@degrees="false"` - Same as above, but with radians as the units for each


the candidate values for @rotation_format given here don't match the values above in Syntax B.

Fixed in a258425.

azeey · 2021-09-10T20:59:10Z

specify_pose/proposal.md

@@ -321,19 +288,17 @@ in code, the order of operations, etc.).

 #### 1.2 Conversion to SDFormat 1.9

-When SDFormat files are converted from SDFormat <=1.8 to 1.9, the `//pose` tags


I removed this section because I think this was a remnant of when this document proposed to deprecate RPY with radians. IMO, we should leave poses in older files intact. This will keep converted from getting verbose. Also, we currently print out only attributes with non-default values. We can have a special case for //pose so that //pose/rotation_format="euler_rpy" and //pose/@degrees=false get printed even though they are the default values, but the added complexity doesn't seem justified.

aaronchongth

Thanks for making the changes! Looks good in general, on a separate note, I found a typo we missed out in the past, coordiantes
https://github.com/ignitionrobotics/sdf_tutorials/blob/azeey/better_pose_update/specify_pose/proposal.md?plain=1#L279

azeey · 2021-09-14T15:42:43Z

Looking at ROS, ROS2 and Ignition messages, it looks like the order xyzw is preferred for quaternions. Even though I like wxyz since it follows the alphabetical order, I propose we change it to xyzw to maintain consistency with tools that will be commonly used with SDFormat.

\cc @EricCousineau-TRI, @FirefoxMetzger

scpeters · 2021-09-14T17:15:15Z

Looking at ROS, ROS2 and Ignition messages, it looks like the order xyzw is preferred for quaternions. Even though I like wxyz since it follows the alphabetical order, I propose we change it to xyzw to maintain consistency with tools that will be commonly used with SDFormat.

\cc @EricCousineau-TRI, @FirefoxMetzger

I am indifferent between wxyz and xyzw; the important thing is that the convention is clear. If we have to choose only one, I think xyzw makes sense to match ROS and ignition.

Separately, would it be confusing for users if we supported both quat_xyzw and quat_wxyz? I don't think it would add too much parser complexity

scpeters · 2021-09-14T17:22:03Z

Separately, would it be confusing for users if we supported both quat_xyzw and quat_wxyz? I don't think it would add too much parser complexity

To answer my own question: if we are only supporting one form of Euler angles, I think we can also pick a single form for quaternions. So I would lean towards quat_xyzw

FirefoxMetzger · 2021-09-14T19:25:40Z

I like the proposal. quart_xyzw (to align with ROS) works for me, too, and I agree with @scpeters that only supporting one version of quaternions for consistency is likely the better choice.

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey · 2021-09-14T22:45:27Z

Okay, I've updated the document to quat_xyzw in 6e54b94.

\cc @aaronchongth

#57) * Add motivation for quaternions as rotation format Signed-off-by: Steve Peters <scpeters@openrobotics.org> Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>

Update document to better reflect proposed and accepted implementations

2bf85d2

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

azeey requested a review from EricCousineau-TRI September 9, 2021 17:49

azeey mentioned this pull request Sep 9, 2021

proposal: Should allow //pose rotation to be specified in degrees? gazebosim/sdformat#252

Closed

4 tasks

azeey requested a review from aaronchongth September 9, 2021 19:26

chapulina approved these changes Sep 9, 2021

View reviewed changes

scpeters reviewed Sep 10, 2021

View reviewed changes

Address reviewer feedback

a258425

azeey commented Sep 10, 2021

View reviewed changes

aaronchongth approved these changes Sep 13, 2021

View reviewed changes

Change quaternion convention to 'xyzw'

6e54b94

Signed-off-by: Addisu Z. Taddese <addisu@openrobotics.org>

scpeters mentioned this pull request Sep 17, 2021

Suggestions for #56: add motivation for quaternions as rotation format #57

Merged

Suggestions for #56: add motivation for quaternions as rotation format (

0473d30

#57) * Add motivation for quaternions as rotation format Signed-off-by: Steve Peters <scpeters@openrobotics.org> Co-authored-by: Addisu Z. Taddese <addisu@openrobotics.org>

scpeters approved these changes Sep 17, 2021

View reviewed changes

azeey merged commit 59b6b20 into master Sep 17, 2021

azeey deleted the azeey/better_pose_update branch September 17, 2021 21:47

azeey mentioned this pull request May 4, 2022

Fix ambiguities in spec inertial documentation gazebosim/sdformat#990

Merged

8 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update document to better reflect proposed and accepted implementations #56

Update document to better reflect proposed and accepted implementations #56

azeey commented Sep 9, 2021 •

edited

Loading

chapulina left a comment

chapulina Sep 9, 2021

azeey Sep 10, 2021

scpeters Sep 14, 2021

scpeters Sep 14, 2021

azeey Sep 14, 2021

chapulina Sep 9, 2021

azeey Sep 10, 2021

chapulina Sep 9, 2021

scpeters Sep 10, 2021

azeey Sep 10, 2021

scpeters Sep 10, 2021

scpeters Sep 10, 2021

azeey Sep 10, 2021

azeey Sep 10, 2021

scpeters Sep 10, 2021

azeey Sep 10, 2021

azeey Sep 10, 2021

aaronchongth left a comment •

edited

Loading

azeey commented Sep 14, 2021

scpeters commented Sep 14, 2021

scpeters commented Sep 14, 2021

FirefoxMetzger commented Sep 14, 2021

azeey commented Sep 14, 2021

		@@ -321,19 +288,17 @@ in code, the order of operations, etc.).

		#### 1.2 Conversion to SDFormat 1.9

		When SDFormat files are converted from SDFormat <=1.8 to 1.9, the `//pose` tags

Update document to better reflect proposed and accepted implementations #56

Update document to better reflect proposed and accepted implementations #56

Conversation

azeey commented Sep 9, 2021 • edited Loading

chapulina left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronchongth left a comment • edited Loading

Choose a reason for hiding this comment

azeey commented Sep 14, 2021

scpeters commented Sep 14, 2021

scpeters commented Sep 14, 2021

FirefoxMetzger commented Sep 14, 2021

azeey commented Sep 14, 2021

azeey commented Sep 9, 2021 •

edited

Loading

aaronchongth left a comment •

edited

Loading