Skip to content
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

sdf 1.8: support specifying frames as joint child/parent #304

Merged
merged 12 commits into from
Jul 16, 2020

Conversation

scpeters
Copy link
Member

@scpeters scpeters commented Jun 24, 2020

Resolves #204, replacement for #300.

This changes the definition of the //joint/child and //joint/parent elements in SDFormat 1.8 so that they may specify a frame name instead of only a link name, per the composition proposal. I've added some example models using this behavior in 85ab1c5. The parsing stages used to construct the FrameAttachedToGraph and PoseRelativeToGraph were changed a bit in de80f92 in order to support this feature, as noted in gazebosim/sdf_tutorials@77b0db1 (currently blocked by gazebosim/sdf_tutorials#28).

Methods for resolving the name of a joint's parent and child links are added in 655833c:

  • Joint::ResolveChildLink
  • Joint::ResolveParentLink

I've redacted the deprecations from #301 from this PR to reduce the noise. We can rename API's in a follow-up PR.


This change is Reviewable

@codecov-commenter
Copy link

codecov-commenter commented Jun 24, 2020

Codecov Report

Merging #304 into master will increase coverage by 0.06%.
The diff coverage is 82.02%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #304      +/-   ##
==========================================
+ Coverage   87.73%   87.79%   +0.06%     
==========================================
  Files          59       59              
  Lines        9047     9110      +63     
==========================================
+ Hits         7937     7998      +61     
- Misses       1110     1112       +2     
Impacted Files Coverage Δ
src/parser.cc 78.60% <64.28%> (-0.22%) ⬇️
src/Model.cc 88.46% <66.66%> (-0.29%) ⬇️
src/FrameSemantics.cc 78.43% <100.00%> (+1.21%) ⬆️
src/Joint.cc 98.35% <100.00%> (+0.31%) ⬆️

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 3b4275d...44cab95. Read the comment docs.

Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 14 of 14 files at r1.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)


Migration.md, line 15 at r1 (raw file):

but with improved human-readability..

## SDFormat 9.x to 10.0

Should this be 10.x to 11.0?


src/ign_TEST.cc, line 280 at r1 (raw file):

  }

  // Check an SDF file with the world specified as a parent link.

This comment doesn't match up with the test.


src/ign_TEST.cc, line 291 at r1 (raw file):

  }

  // Check an SDF file with the world specified as a parent link.

The comment doesn't match up with the test.


test/sdf/joint_invalid_self_child.sdf, line 2 at r1 (raw file):

<?xml version="1.0" ?>
<sdf version="1.6">

Is the version 1.6 on purpose?


test/sdf/joint_invalid_self_parent.sdf, line 2 at r1 (raw file):

<?xml version="1.0" ?>
<sdf version="1.6">

Is the version 1.6 on purpose?

Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)


Migration.md, line 15 at r1 (raw file):

Previously, azeey (Addisu Taddese) wrote…

Should this be 10.x to 11.0?

oh, good point. I'm updating the migration guide in #308, and then I'll merge those changes from sdf10 forward to master


test/sdf/joint_invalid_self_child.sdf, line 2 at r1 (raw file):

Previously, azeey (Addisu Taddese) wrote…

Is the version 1.6 on purpose?

it's technically invalid for both 1.6 and 1.8, though for different reasons, but the name of the file is based on the 1.8 reason for invalidity. I'll update the version to 1.8


test/sdf/joint_invalid_self_parent.sdf, line 2 at r1 (raw file):

Previously, azeey (Addisu Taddese) wrote…

Is the version 1.6 on purpose?

I'll update it to 1.8

Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)


src/ign_TEST.cc, line 280 at r1 (raw file):

Previously, azeey (Addisu Taddese) wrote…

This comment doesn't match up with the test.

fixed in 2554d5e


src/ign_TEST.cc, line 291 at r1 (raw file):

Previously, azeey (Addisu Taddese) wrote…

The comment doesn't match up with the test.

fixed in 2554d5e


test/sdf/joint_invalid_self_child.sdf, line 2 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

it's technically invalid for both 1.6 and 1.8, though for different reasons, but the name of the file is based on the 1.8 reason for invalidity. I'll update the version to 1.8

updated in 2554d5e


test/sdf/joint_invalid_self_parent.sdf, line 2 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

I'll update it to 1.8

updated in 2554d5e

Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 14 of 14 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)

Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)


src/ign_TEST.cc, line 280 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

fixed in 2554d5e

I think you missed this one. The comment suggests the parent link is world, but this test is about having a frame specified as a child.


src/ign_TEST.cc, line 291 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

fixed in 2554d5e

I think you missed this one as well.

Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey, @EricCousineau-TRI, and @scpeters)


src/ign_TEST.cc, line 280 at r1 (raw file):

Previously, azeey (Addisu Taddese) wrote…

I think you missed this one. The comment suggests the parent link is world, but this test is about having a frame specified as a child.

you're right I fixed a different incorrect comment

Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)


src/ign_TEST.cc, line 280 at r1 (raw file):

Previously, scpeters (Steve Peters) wrote…

you're right I fixed a different incorrect comment

should be fixed by aa5a763


src/ign_TEST.cc, line 291 at r1 (raw file):

Previously, azeey (Addisu Taddese) wrote…

I think you missed this one as well.

aa5a763

Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r3.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @azeey and @EricCousineau-TRI)

Copy link
Collaborator

@azeey azeey left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm:

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @EricCousineau-TRI)

The `//joint/child` and `//joint/parent` elements now accept any
valid frame name instead of only link names.
The migration guide is updated as well.

Closes gazebosim#204.

Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
buildFrameAttachedToGraph: don't create edges from joint to child
until all joint and frame vertices are added, in case the child
is not a link.

buildPoseRelativeToGraph: if //joint/pose/@relative_to is not
specified, don't create edges from joint to child until all joint
and frame vertices are added to the graph, in case the child is
not a link.

Signed-off-by: Steve Peters <[email protected]>
Signed-off-by: Steve Peters <[email protected]>
These methods use the FrameAttachedToGraph to resolve the name
of the parent and child links of a joint, similar to
Frame::ResolveAttachedToBody.

Signed-off-by: Steve Peters <[email protected]>
Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r4.
Reviewable status: :shipit: complete! all files reviewed, all discussions resolved (waiting on @EricCousineau-TRI)

@scpeters
Copy link
Member Author

scpeters commented Jul 5, 2020

ready for review from @EricCousineau-TRI

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 14 files at r1, 2 of 5 files at r4.
Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @scpeters)

a discussion (no related file):
To do:
Make a mention of "stacking joints" via joint implicit frames, and how that will do parallel, not serial, joints.



Migration.md, line 20 at r4 (raw file):

1. **sdf/Joint.hh**
    + Errors ResolveChildLink(std::string&) const

nit Should these be in backticks for code markup?

@EricCousineau-TRI
Copy link
Collaborator

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

To do:
Make a mention of "stacking joints" via joint implicit frames, and how that will do parallel, not serial, joints.

Will write up XML for L1 -> J1 -> J2 -> L2


Signed-off-by: Steve Peters <[email protected]>
Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI and @scpeters)


Migration.md, line 20 at r4 (raw file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

nit Should these be in backticks for code markup?

looking at the rest of the document, we use backpacks when mentioning APIs in prose, but the bullet point lists don't use backticks. If we want to add backpacks everywhere, I would do that in a separate PR

I did notice one missing backpack and added it in d6a2a3c

Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 2 unresolved discussions (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Will write up XML for L1 -> J1 -> J2 -> L2

ok, I'll wait for this xml


Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: 13 of 14 files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI and @scpeters)

a discussion (no related file):

Previously, scpeters (Steve Peters) wrote…

ok, I'll wait for this xml

Sorry for delay!

Here's what I was envisioning:

<?xml version="1.0" ?>
<sdf version="1.7">
  <model name="parallel_but_looks_serial"> <!-- Or broken? -->
    <link name="L1"/>

    <joint name="J1" type="prismitic">
      <parent>L1</parent>
      <child>J2</child>
      <axis>
        <xyz>1 0 0</xyz>
      </axis>
    </joint>

    <joint name="J2" type="prismitic">
      <parent>J1</parent>
      <child>L2</child>
      <axis>
        <xyz>0 1 0</xyz>
      </axis>
    </joint>

    <link name="L2"/>
  </model>
</sdf>

My attempt to draw it out:
https://github.com/EricCousineau-TRI/gist_like_files/blob/66943a3274d66cbf9b81945db55da81c58e351dc/drawings/2020-07-10-sdformat-joint-serial-vs-parallel.svg
(can't embed SVG via ![image](url)?)



Migration.md, line 20 at r4 (raw file):

Previously, scpeters (Steve Peters) wrote…

looking at the rest of the document, we use backpacks when mentioning APIs in prose, but the bullet point lists don't use backticks. If we want to add backpacks everywhere, I would do that in a separate PR

I did notice one missing backpack and added it in d6a2a3c

OK SGTM!

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:lgtm: pending addition of unittest showing the weird L1 -> Jx -> Jy -> L2 setup

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

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @scpeters)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Sorry for delay!

Here's what I was envisioning:

<?xml version="1.0" ?>
<sdf version="1.7">
  <model name="parallel_but_looks_serial"> <!-- Or broken? -->
    <link name="L1"/>

    <joint name="J1" type="prismitic">
      <parent>L1</parent>
      <child>J2</child>
      <axis>
        <xyz>1 0 0</xyz>
      </axis>
    </joint>

    <joint name="J2" type="prismitic">
      <parent>J1</parent>
      <child>L2</child>
      <axis>
        <xyz>0 1 0</xyz>
      </axis>
    </joint>

    <link name="L2"/>
  </model>
</sdf>

My attempt to draw it out:
https://github.com/EricCousineau-TRI/gist_like_files/blob/66943a3274d66cbf9b81945db55da81c58e351dc/drawings/2020-07-10-sdformat-joint-serial-vs-parallel.svg
(can't embed SVG via ![image](url)?)

Er, just needed to use the raw link - d'oh!


Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @scpeters)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Er, just needed to use the raw link - d'oh!

My guess is, just looking at the code, it will produce a cycle in the frame graph, so it should die early.
However, if I were to place myself in grad-student "try random shit til it works", I'd probably end up trying this variant too:

    <joint name="Jx" type="prismatic">
      <!-- Avoid depending on J2 pose that makes for frame graph cycle -->
      <pose relative_to="L1"/>
      <parent>L1</parent>
      <child>Jx</child>
      <axis>
        <xyz>1 0 0</xyz>
      </axis>
    </joint>

    <joint name="Jy" type="prismatic">
      <parent>Jx</parent>
      <child>L2</child>
      <axis>
        <xyz>0 1 0</xyz>
      </axis>
    </joint>

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @scpeters)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

My guess is, just looking at the code, it will produce a cycle in the frame graph, so it should die early.
However, if I were to place myself in grad-student "try random shit til it works", I'd probably end up trying this variant too:

    <joint name="Jx" type="prismatic">
      <!-- Avoid depending on J2 pose that makes for frame graph cycle -->
      <pose relative_to="L1"/>
      <parent>L1</parent>
      <child>Jx</child>
      <axis>
        <xyz>1 0 0</xyz>
      </axis>
    </joint>

    <joint name="Jy" type="prismatic">
      <parent>Jx</parent>
      <child>L2</child>
      <axis>
        <xyz>0 1 0</xyz>
      </axis>
    </joint>

Damn, I meant <child>Jy</child>, not <child>Jx</child>.

(One thing I dislike about Reviewable - no easy way to edit comments...)


Return early from ResolveParentLink if "world" is specified
as a joint's parent frame, since "world" is not in a Model's
FrameAttachedToGraph.

Signed-off-by: Steve Peters <[email protected]>
Add test world in which a joint's child frame resolves to the
same value as the parent, which is invalid.
Check for this case in `ign sdf --check`.

Signed-off-by: Steve Peters <[email protected]>
Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 5 of 5 files at r6.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Damn, I meant <child>Jy</child>, not <child>Jx</child>.

(One thing I dislike about Reviewable - no easy way to edit comments...)

I added a test for the parallel_but_looks_serial model you pasted above, and I believe that specific model is invalid because the J1 frame is attached to its child link L2, which means that joint J2's parent and child frames both resolve to link L2 (invalid because a parent and child link must be different). We weren't actually checking that, so I added a check for this to ign sdf --check in 44cab95 and updated the sdf_tutorials PR in gazebosim/sdf_tutorials@85c2a91

I'll try the second example now


Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, scpeters (Steve Peters) wrote…

I added a test for the parallel_but_looks_serial model you pasted above, and I believe that specific model is invalid because the J1 frame is attached to its child link L2, which means that joint J2's parent and child frames both resolve to link L2 (invalid because a parent and child link must be different). We weren't actually checking that, so I added a check for this to ign sdf --check in 44cab95 and updated the sdf_tutorials PR in gazebosim/sdf_tutorials@85c2a91

I'll try the second example now

The second example has the same issue:

  • frame Jy resolves to its child L2 in the FrameAttachedToGraph
  • specifying Jy as the child of Jx means that Jx also resolves to L2
  • specifying Jx as the parent of Jy means that the parent and child frames of Jy both resolve to L2, which is invalid

Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @scpeters)

a discussion (no related file):

Previously, scpeters (Steve Peters) wrote…

The second example has the same issue:

  • frame Jy resolves to its child L2 in the FrameAttachedToGraph
  • specifying Jy as the child of Jx means that Jx also resolves to L2
  • specifying Jx as the parent of Jy means that the parent and child frames of Jy both resolve to L2, which is invalid

Ah, gotcha! Is there a way to encode that variant in a unittest as well?
(or perhaps just a TODO?)


Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, EricCousineau-TRI (Eric Cousineau) wrote…

Ah, gotcha! Is there a way to encode that variant in a unittest as well?
(or perhaps just a TODO?)

ok, I'll add another example sdf with a test


Copy link
Member Author

@scpeters scpeters left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @EricCousineau-TRI)

a discussion (no related file):

Previously, scpeters (Steve Peters) wrote…

ok, I'll add another example sdf with a test

Actually, now that I look at it, those two examples look identical to me, except for the names of the joints and the extra //joint/pose/@relative_to specification. What is the difference supposed to be?


Copy link
Collaborator

@EricCousineau-TRI EricCousineau-TRI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewable status: :shipit: complete! all files reviewed, all discussions resolved

a discussion (no related file):

Previously, scpeters (Steve Peters) wrote…

Actually, now that I look at it, those two examples look identical to me, except for the names of the joints and the extra //joint/pose/@relative_to specification. What is the difference supposed to be?

OK Per f2f, yup, conditions are identical.


@scpeters scpeters merged commit a460035 into gazebosim:master Jul 16, 2020
@scpeters scpeters deleted the joint_parent_child_frame branch July 16, 2020 20:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

joint: should support specifying frames in <child> and <parent>
4 participants