specify_pose: Add "Proposal for a better pose" #37
Conversation
EricCousineau-TRI
force-pushed
the
issue-sdformat-252-pose-degrees
branch
from
04daf5d to
0d9cdd0
Compare
EricCousineau-TRI
changed the title
EricCousineau-TRI
force-pushed
the
issue-sdformat-252-pose-degrees
branch
3 times, most recently
from
d59f2d0 to
933311e
Compare
EricCousineau-TRI
force-pushed
the
issue-sdformat-252-pose-degrees
branch
from
56e0e49 to
27998e7
Compare
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 1 unresolved discussion (waiting on @azeey and @scpeters)
specify_pose/proposal.md, line 146 at r1 (raw file):
is the real component. This should generally be used when the rotation should be machine-generated (e.g. calibration artifacts). * It is encouraged to use 16 digits of precision when possible.
This should be concretely verified reflect the following value:
#include <limits>
std::numeric_limits<double>::max_digits10
// 17
Used a brief cling Binder session:
https://mybinder.org/v2/gh/jupyter-xeus/xeus-cling/e1ebf168aff5d2566dff29b61b14db3a40ca0fac?filepath=notebooks%2Fxcpp.ipynb
FWIW In Python:
import numpy as np
print(np.finfo(np.float64).precision)
# 15
Should find docs reconciling the two
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 5 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)
manifest.xml, line 37 at r1 (raw file):
<tutorial title="Proposal for a better pose" ref='better_pose_proposal' type='proposal'> <markdown version="1.7">specify_pose/proposal.md</markdown>
1.7 -> 1.8?
specify_pose/proposal.md, line 18 at r1 (raw file):
Currently, the text within `//pose` consists of a 6-tuple representing `{xyz} {rpy}`, where `{xyz}` is a 3-tuple representing translation (in meters) and `{rpy}` is a 3-tuple representing translation (in radians).
translation->rotation
specify_pose/proposal.md, line 23 at r1 (raw file):
sometimes hard to visually separate translation from rotation and (2) specifying rotation in radians adds overhead when hand-crafting models because the author must specify common degree values (e.g. 30, 45, 60, 90 degrees) i
i -> in
specify_pose/proposal.md, line 27 at r1 (raw file):
This proposal intends to resolve on point (1) by structuring the pose as `//pose/xyz` and `//pose/rotation`, and point (2) by adding
It seems a bit odd that we would use xyz for position/translation and rotation for rotation. The <xyz> element is also already used to specify a unit vector for joint axis. What do you think about using "position" or "translation" or a short form of those?
specify_pose/proposal.md, line 239 at r1 (raw file):
in the API (or even the documentation). Instead, the author recommends explicitly enumerating this order in a relatively unambiguous way that is shown directly in the specification.
BTW, having been bitten by this many times, I strongly support explicitly stating the order.
|
@sammy-tri Would you be able to review this too? |
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 6 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)
a discussion (no related file):
Per VC, should ensure this proposal covers what happens when poses are serialized to an XML document.
Options:
- Always output quaternion (least loss of precision?)
- Always output quatnerion, unless RPY can represent it to some degree of precision?
- ?
There was a problem hiding this comment.
First pass complete, I need to give the idea some more thought. I don't have any objections yet beyond it feeling slightly... weird? I do agree that the current trend of specifying fractions of pi with arbitrary precision depending on the whims of the author is not ideal. (for example, I rarely use more than 3 significant digits and I wouldn't be surprised if that error adds up to something significant in some cases)
Reviewable status: 0 of 2 files reviewed, 8 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)
specify_pose/proposal.md, line 27 at r1 (raw file):
This proposal intends to resolve on point (1) by structuring the pose as `//pose/xyz` and `//pose/rotation`, and point (2) by adding
BTW The existing SDF documentation for pose describes the "rpy" component as "orientation" not "rotation". I have no idea if one term is more mathematically correct than the other in this context.
specify_pose/proposal.md, line 257 at r1 (raw file):
When converting to roll-pitch-yaw coordiantes, we should try to specify the *exact* math being done. (e.g. a cross-reference to `Quatnerion::Euler()`
nit: typo "quatnerion"
specify_pose/proposal.md, line 285 at r1 (raw file):
It'd be nice to constrain the rotation types to just two. Radians don't seem that useful if you have degrees for RPY, and quaternions for machine-generated
My gut feeling is that rpy_radians is also fairly common for machine-generated rotations as quaternions, but I'll admit to having no data for this.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 10 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)
specify_pose/proposal.md, line 102 at r1 (raw file):
## Proposed changes ### 1. `//pose/xyz` and `//pose/rotation`
I think an alternative to this is not using sub-elements but adding a //pose/@orientation_type attribute that can change the number and interpretation of tuple elements after the first three.
specify_pose/proposal.md, line 239 at r1 (raw file):
Previously, azeey (Addisu Taddese) wrote…
BTW, having been bitten by this many times, I strongly support explicitly stating the order.
👍
specify_pose/proposal.md, line 282 at r1 (raw file):
#### 1.2.2 Deprecate `@type="rpy_radians"` in SDFormat 1.9 This should be deprecated in SDFormat 1.9, and removed in SDFormat 1.10.
I don't think we should deprecate the default rotation type used in old versions of SDFormat and URDF.
There was a problem hiding this comment.
Reviewable status: 0 of 2 files reviewed, 10 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, @sammy-tri, and @scpeters)
manifest.xml, line 37 at r1 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
1.7 -> 1.8?
Done.
specify_pose/proposal.md, line 18 at r1 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
translation->rotation
Done.
specify_pose/proposal.md, line 23 at r1 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
i -> in
Done.
specify_pose/proposal.md, line 27 at r1 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
It seems a bit odd that we would use
xyzfor position/translation androtationfor rotation. The<xyz>element is also already used to specify a unit vector for joint axis. What do you think about using "position" or "translation" or a short form of those?
Done. Trying out translation.
specify_pose/proposal.md, line 27 at r1 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
BTW The existing SDF documentation for pose describes the "rpy" component as "orientation" not "rotation". I have no idea if one term is more mathematically correct than the other in this context.
OK From the brief survey I did, seems like "rotation" is more popular in modeling formats. Only counter I've seen is orientation for ROS's pose messages.
specify_pose/proposal.md, line 102 at r1 (raw file):
Previously, scpeters (Steve Peters) wrote…
I think an alternative to this is not using sub-elements but adding a
//pose/@orientation_typeattribute that can change the number and interpretation of tuple elements after the first three.
Done. (Added as an alternative atm)
specify_pose/proposal.md, line 239 at r1 (raw file):
Previously, scpeters (Steve Peters) wrote…
👍
OK Yeah, when reviewing other formats, it was briefly painful to find the format (or infer it implicitly from what I hope are examples for identity :P)
specify_pose/proposal.md, line 285 at r1 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
My gut feeling is that rpy_radians is also fairly common for machine-generated rotations as quaternions, but I'll admit to having no data for this.
I think that's only a facet of URDF / SDFormat doing those. I'd prefer to have a representation with fewer weird singularities / wraparound, which a quaternion provides.
I could see it as useful for simple text-based upgrades of files, but I dunno how I feel about that if we can just automate the up-conversion process...
May relax it when I revisit this next week.
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2.
Reviewable status: 1 of 2 files reviewed, 13 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @sammy-tri)
specify_pose/proposal.md, line 257 at r1 (raw file):
Previously, sammy-tri (Sam Creasey) wrote…
nit: typo "quatnerion"
👍
specify_pose/proposal.md, line 359 at r2 (raw file):
Some examples: ```xml
I think you need a blank line above this as it's not rendering properly:
specify_pose/proposal.md, line 379 at r2 (raw file):
Some examples: ```json
another blank line needed above this:
specify_pose/proposal.md, line 395 at r2 (raw file):
An example:
blank line needed above this
specify_pose/proposal.md, line 413 at r2 (raw file):
Note: I (Eric) am assuming radians for Euler angles.
does this note apply to the SKEL format only?
There was a problem hiding this comment.
Reviewed 1 of 2 files at r2.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @sammy-tri)
There was a problem hiding this comment.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @sammy-tri)
specify_pose/proposal.md, line 285 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
I think that's only a facet of URDF / SDFormat doing those. I'd prefer to have a representation with fewer weird singularities / wraparound, which a quaternion provides.
I could see it as useful for simple text-based upgrades of files, but I dunno how I feel about that if we can just automate the up-conversion process...
May relax it when I revisit this next week.
Per f2f, will keep it given legacy.
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 11 unresolved discussions (waiting on @azeey, @sammy-tri, and @scpeters)
specify_pose/proposal.md, line 146 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
This should be concretely verified reflect the following value:
#include <limits> std::numeric_limits<double>::max_digits10 // 17Used a brief cling Binder session:
https://mybinder.org/v2/gh/jupyter-xeus/xeus-cling/e1ebf168aff5d2566dff29b61b14db3a40ca0fac?filepath=notebooks%2Fxcpp.ipynbFWIW In Python:
import numpy as np print(np.finfo(np.float64).precision) # 15Should find docs reconciling the two
Done. Didn't reconcile, but just stuck with C++'s limits.
(Assumes a common architecture, but 🤷 )
specify_pose/proposal.md, line 282 at r1 (raw file):
Previously, scpeters (Steve Peters) wrote…
I don't think we should deprecate the default rotation type used in old versions of SDFormat and URDF.
Done.
specify_pose/proposal.md, line 285 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Per f2f, will keep it given legacy.
Done.
specify_pose/proposal.md, line 359 at r2 (raw file):
Previously, scpeters (Steve Peters) wrote…
I think you need a blank line above this as it's not rendering properly:
Done.
specify_pose/proposal.md, line 379 at r2 (raw file):
Previously, scpeters (Steve Peters) wrote…
another blank line needed above this:
Done.
specify_pose/proposal.md, line 395 at r2 (raw file):
Previously, scpeters (Steve Peters) wrote…
blank line needed above this
Done.
specify_pose/proposal.md, line 413 at r2 (raw file):
Previously, scpeters (Steve Peters) wrote…
does this note apply to the SKEL format only?
Done.
There was a problem hiding this comment.
Reviewable status: 1 of 2 files reviewed, 12 unresolved discussions (waiting on @azeey, @sammy-tri, and @scpeters)
a discussion (no related file):
Thinking on this more, perhaps I could be convinced of something like a 4-tuple axis-angle, where axis will be normalized (magnitude always ignored), and degrees will specify the angle.
Then that can make certain rotations (e.g. rotate about normalized([1, 1, 1]) by 45 degrees) a bit easier to express by hand, rather than resorting to a lookup for rpy combos or quaternions.
EricCousineau-TRI
force-pushed
the
issue-sdformat-252-pose-degrees
branch
from
be99233 to
8f9c46d
Compare
There was a problem hiding this comment.
Thanks for the comments! I think I've addressed all outstanding discussion poinst
Reviewable status: 0 of 2 files reviewed, 8 unresolved discussions (waiting on @aaronchongth, @azeey, @EricCousineau-TRI, @sammy-tri, and @scpeters)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
As we've talked about this, I believe we should start in the following fashion:
- For output, provide the option to specify pose output. By default, it will radians (for simplicity).
- For degrees and other representations, we can add an option for a "snap-to-grid" value, e.g. if your value is close to 45 degrees along whatever axes by some threshold (user specified), it'll get replaced.
Done. (encoded in proposal)
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Thinking on this more, perhaps I could be convinced of something like a 4-tuple axis-angle, where axis will be normalized (magnitude always ignored), and degrees will specify the angle.
Then that can make certain rotations (e.g. rotate about
normalized([1, 1, 1])by 45 degrees) a bit easier to express by hand, rather than resorting to a lookup for rpy combos or quaternions.
Done. Ignoring for now.
a discussion (no related file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Should mention:
https://www.ros.org/reps/rep-0103.html
Done.
specify_pose/proposal.md, line 320 at r5 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
I was under the impression that we were not deprecating the existing usage of
//poseper https://reviewable.io/reviews/osrf/sdf_tutorials/37#-MDp2cQm8hCmQTOa7MWc.
Done. oops!
FWIW Also, I think your link was broken; Reviewable hyperlinks can be finicky.
I believe this is the link in GitHub you meant? (though imprecise, as it doesn't pinpoint exact convo)
#37 (review)
specify_pose/proposal.md, line 325 at r5 (raw file):
Previously, aaronchongth (Aaron Chong) wrote…
Would this be 1.9? This would apply to the next line too
Done.
There was a problem hiding this comment.
//pose/translation and //pose/rotation. If we end up not pursuing those, it would be good to update some of the sections and examples showing //pose/@rotation_type.
Reviewed 1 of 2 files at r4, 1 of 1 files at r7.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @aaronchongth, @EricCousineau-TRI, and @sammy-tri)
specify_pose/proposal.md, line 146 at r1 (raw file):
std::numeric_limits<double>::max_digits10
std::numeric_limits<double>::digits10 is 15, which I think is what np.finfo is providing. I'm still trying to understand the difference between max_digits10 and digits10 and its implication on parsed input values. From https://stackoverflow.com/a/56515598/283225
digits10: Given a value in decimal representation, how many decimal digits can be guaranteedly preserved if converted from decimal to a selected binary format and back (with default rounding).
max_digits10: Given a value in binary format, how many decimal digits are needed if value is converted to decimal format and back to original binary format (again, with default rounding) to get the original value unchanged.
So I think using max_digits10 when outputting values makes sense because that's the string representation that captures the full precision of the number being generated.
specify_pose/proposal.md, line 320 at r5 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Done. oops!
FWIW Also, I think your link was broken; Reviewable hyperlinks can be finicky.
I believe this is the link in GitHub you meant? (though imprecise, as it doesn't pinpoint exact convo)
#37 (review)
I'll stop using reviewable hyperlinks. It seems to work for me, but not for others. It must be tied to my session.
specify_pose/proposal.md, line 28 at r7 (raw file):
This proposal intends to resolve on point (1) by adding `//pose/rotation/@type` where degrees can be specified, and *could* address point by structuring the
nit: address point (2)
specify_pose/proposal.md, line 35 at r7 (raw file):
Make a bullet list of all major sections: TBD
nit: Can this TBD be filled out now?
specify_pose/proposal.md, line 50 at r7 (raw file):
```xml <pose>{xyz} {rpy_radians}</pose> <!-- Old format; deprecated. -->
nit: Can you remove "deprecated"?
specify_pose/proposal.md, line 170 at r7 (raw file):
While SDFormat could use attributes for these values like URDF does, it would go against the convention used for other elements (e.g. `//joint/axis/translation`, `//inertia/ixx,...`).
nit: //joint/axis/translation -> //joint/axis/xyz?
There was a problem hiding this comment.
LGTM!
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @EricCousineau-TRI and @sammy-tri)
…2-pose-degrees # Conflicts: # manifest.xml
|
Addressed all points! Unable to publish changes on Reviewable, though (I guess due to migration to |
|
Testing comments for Reviewable |
|
Testing comments again |
|
specify_pose/proposal.md, line 35 at r7 (raw file): Previously, azeey (Addisu Z. Taddese) wrote…
Test reviewable comment. |
|
specify_pose/proposal.md, line 28 at r7 (raw file): Previously, azeey (Addisu Z. Taddese) wrote…
Done. |
|
specify_pose/proposal.md, line 51 at r9 (raw file):
Now that we've pretty much converged on |
|
specify_pose/proposal.md, line 50 at r7 (raw file): Previously, azeey (Addisu Z. Taddese) wrote…
Done. |
There was a problem hiding this comment.
Reviewed 2 of 2 files at r9.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI and @sammy-tri)
specify_pose/proposal.md, line 170 at r7 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
nit:
//joint/axis/translation->//joint/axis/xyz?
Done.
There was a problem hiding this comment.
Dismissed @sammy-tri from a discussion.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)
specify_pose/proposal.md, line 146 at r1 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
std::numeric_limits<double>::max_digits10
std::numeric_limits<double>::digits10is 15, which I think is whatnp.finfois providing. I'm still trying to understand the difference betweenmax_digits10anddigits10and its implication on parsed input values. From https://stackoverflow.com/a/56515598/283225
digits10: Given a value in decimal representation, how many decimal digits can be guaranteedly preserved if converted from decimal to a selected binary format and back (with default rounding).
max_digits10: Given a value in binary format, how many decimal digits are needed if value is converted to decimal format and back to original binary format (again, with default rounding) to get the original value unchanged.So I think using
max_digits10when outputting values makes sense because that's the string representation that captures the full precision of the number being generated.
OK Sweet, thanks for checking! TBH, I'm still a bit confused on why NumPy doesn't present similar information, but perhaps I need to ponder a bit more!
specify_pose/proposal.md, line 257 at r1 (raw file):
Previously, EricCousineau-TRI (Eric Cousineau) wrote…
Done. Oops!
Done.
specify_pose/proposal.md, line 35 at r7 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
nit: Can this TBD be filled out now?
Done.
specify_pose/proposal.md, line 50 at r7 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
nit: Can you remove "deprecated"?
Done.
specify_pose/proposal.md, line 170 at r7 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
nit:
//joint/axis/translation->//joint/axis/xyz?
Done.
There was a problem hiding this comment.
Dismissed @azeey from a discussion.
Reviewable status:complete! all files reviewed, all discussions resolved (waiting on @EricCousineau-TRI)
specify_pose/proposal.md, line 51 at r9 (raw file):
Previously, azeey (Addisu Z. Taddese) wrote…
Now that we've pretty much converged on
//pose/@rotation_type, I feel like this section can be moved to the "Alternatives Considered" section. Thoughts?
OK Per VC, will do this as follow-up PR!
|
Assuming all discussions were resolved via Reviewable, so these should be good to go - merging! |
Towards gazebosim/sdformat#252
Low-priority for review w.r.t. composition at the moment, but I would still love to land this for SDFormat 1.8 in general.
Preview:
http://sdformat.org/tutorials?tut=better_pose_proposal&cat=pose_semantics_docs&branch=issue-sdformat-252-pose-degrees&repo=https%3A%2F%2Fgithub.com%2FEricCousineau-TRI%2Fsdf_tutorials-1
EDIT: Preview is broken, probably due to issue w/ manifest.
This change is